From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12204 invoked by alias); 1 Aug 2009 09:57:43 -0000 Received: (qmail 12190 invoked by uid 22791); 1 Aug 2009 09:57:43 -0000 X-SWARE-Spam-Status: No, hits=3.0 required=5.0 tests=AWL,BAYES_00,BOTNET,RCVD_IN_JMF_BL,SPF_SOFTFAIL X-Spam-Check-By: sourceware.org Received: from mtaout1.012.net.il (HELO mtaout1.012.net.il) (84.95.2.1) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 01 Aug 2009 09:57:35 +0000 Received: from conversion-daemon.i-mtaout1.012.net.il by i-mtaout1.012.net.il (HyperSendmail v2007.08) id <0KNO00500Z7IS000@i-mtaout1.012.net.il> for gdb-patches@sourceware.org; Sat, 01 Aug 2009 12:57:31 +0300 (IDT) Received: from HOME-C4E4A596F7 ([77.126.255.147]) by i-mtaout1.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0KNO00I9UZNVKZ90@i-mtaout1.012.net.il>; Sat, 01 Aug 2009 12:57:31 +0300 (IDT) Date: Sat, 01 Aug 2009 09:57:00 -0000 From: Eli Zaretskii Subject: Re: [RFA/RFC] Add dump and load command to process record and replay In-reply-to: To: Hui Zhu Cc: gdb-patches@sourceware.org, msnyder@vmware.com Reply-to: Eli Zaretskii Message-id: <83my6jq2k3.fsf@gnu.org> References: X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-08/txt/msg00003.txt.bz2 > From: Hui Zhu > Date: Sat, 1 Aug 2009 15:30:38 +0800 > Cc: Michael Snyder , Eli Zaretskii > > This patch add command record dump and record load to make prec can > dump execution log to a file and load it later. And load can work > with core file. Thanks. > + /* Default corefile name is "rec.PID". */ I suggest the default name to be "gdb_record.PID". > + if (write (recfd, &magic, 4) != 4) > + error (_("Failed to write '%s' for dump."), recfilename); I suggest to change the text (here and elsewhere in your patch) to: "Failed to write dump of execution records to `%s'." I also suggest to replace the numerous fragments like this: > + if (write (recfd, &tmpu8, 1) != 1) > + error (_("Failed to write '%s' for dump."), recfilename); with calls to a function which tries to write and errors out of it cannot. That would leave only one "Failed to write ..." string, instead of having gobs of them. > + fprintf_filtered (gdb_stdout, "Saved recfile %s.\n", recfilename); I suggest to replace the message text with this: "Saved dump of execution records to `%s'." > + if (recfd < 0) > + error (_("Failed to open '%s' for load."), args); A better text would be "Failed to open `%s' for loading execution records: %s" The second %s should get the result of strerror (errno), to describe the reason of the failure > + if (read (recfd, &magic, 4) != 4) > + error (_("Failed to read '%s' for load."), args); "Failed reading dump of execution records in `%s'" Again, I suggest to have a single function that reads and errors out on error. > + if (magic != RECORD_FILE_MAGIC) > + error (_("'%s' is not a record dump."), args); "`%s' is not a valid dump of execution records." > + c = add_cmd ("dump", class_obscure, cmd_record_dump, > + _("Dump the execution log to a file.\n\ "Dump the execution records to a file." > +Argument is optional filename. Default filename is 'rec.'."), > + &record_cmdlist); > + set_cmd_completer (c, filename_completer); > + c = add_cmd ("load", class_obscure, cmd_record_load, > + _("Load the execution log from a file. Argument is filename."), "Load previously dumped execution records from a file given as argument." Note that you cannot have more than one period on the first line, because the help commands assume there's only one there. > +@kindex record dump > +@kindex rec dump > +@item record dump [@var{file}] > +@itemx rec dump [@var{file}] > +Produce a record execution log of the inferior process. The optional ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Dump the execution records of the inferior process to a file. > +argument @var{file} specifies the file name where to put the record dump. > +If not specified, the file name defaults to @file{rec.@var{pid}}, where > +@var{pid} is the inferior process ID. ^^^^^^^^^^^^^^^^^^^^^^^^^^ "is the PID of the inferior process." > +Load process record execution log from @var{file}. Load previously-dumped execution records from @var{file}. > +It can work with @code{core-file}. This needs further explanation. Or maybe just delete it, if it silently does The Right Thing.