Re: [RFA/RFC] Add dump and load command to process record and replay

[Eli Zaretskii <eliz@gnu.org>]

> From: Hui Zhu <teawater@gmail.com>
> Date: Sat, 1 Aug 2009 15:30:38 +0800
> Cc: Michael Snyder <msnyder@vmware.com>, Eli Zaretskii <eliz@gnu.org>
> 
> 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.<process_id>'."),
> +               &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.