Re: [PATCH 2/5] Save trace into CTF format

[Tom Tromey <tromey@redhat.com>]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> +/* Handler of writing trace of CTF format.  */
Yao> +
Yao> +struct trace_write_handler

I don't think that comment is very useful.
Perhaps it could just say that this is the state kept while writing a
CTF file.  Assuming that is what it is.

Yao> +  if (fwrite (buf, size, 1, fd) != 1)
Yao> +    error (_("Unable to write file for saving trace data (%s)"),
Yao> +	   safe_strerror (errno));

We had this whole thread about using ui-out with patches and everything.
Upthread you said that it was abandoned due to lack of error checking on
write.  But I thought that was addressed in the previous thread?

I suppose I don't really care enough to press it.
I don't recall offhand why I thought it was important; maybe some code
duplication, but I don't really see it any more.

Yao> +static void
Yao> +ctf_save_fwrite_format_1 (FILE *fd, const char *format, va_list args)
Yao> +{
Yao> +  char *linebuffer;
Yao> +  struct cleanup *old_cleanups;
Yao> +
Yao> +  linebuffer = xstrvprintf (format, args);
Yao> +  old_cleanups = make_cleanup (xfree, linebuffer);
Yao> +  ctf_save_fwrite (fd, linebuffer, strlen (linebuffer));
Yao> +  do_cleanups (old_cleanups);
Yao> +}
Yao> +
Yao> +/* Write data in FORMAT to FD.  */
Yao> +
Yao> +static void
Yao> +ctf_save_fwrite_format (FILE *fd, const char *format, ...)
Yao> +{
Yao> +  va_list args;
Yao> +
Yao> +  va_start (args, format);
Yao> +  ctf_save_fwrite_format_1 (fd, format, args);
Yao> +  va_end (args);
Yao> +}

Why not just use vfprintf directly and drop ctf_save_fwrite_format_1 and
the cleanups and the rest?

Yao> +static int
Yao> +ctf_save_fseek (struct trace_write_handler *handler, long offset,
Yao> +		int whence)
Yao> +{
Yao> +  if (fseek (handler->datastream_fd, offset, whence))
Yao> +    error (_("Unable to seek file for saving trace data (%s)"),
Yao> +	   safe_strerror (errno));
Yao> +
Yao> +  if (whence == SEEK_CUR)
Yao> +    handler->content_size += offset;

Why only update the size in this one case?
If it is because the callers only use it in specific ways, then I
suggest assertions to cover this, like maybe:

    gdb_assert (whence != SEEK_END);
    gdb_assert (whence != SEEK_SET || offset < handler->content_size);

Yao> +/* Change the datastream file position to align on ALIGN_SIZE,
Yao> +   and Write BUF to datastream file.  The size of BUF is SIZE.  */

Lowercase "w" in "write".

Yao> +  if (handler->metadata_fd)
Yao> +    fclose (handler->metadata_fd);
Yao> +
Yao> +  if (handler->datastream_fd)
Yao> +    fclose (handler->datastream_fd);

Use the "!= NULL" form.

Yao> +  /* Create DIRNAME.  */
Yao> +  file_name = alloca (strlen (dirname) + 1
Yao> +		      + strlen (CTF_DATASTREAM_NAME) + 1);

I think this assignment is dead.

Yao> +  if (!data->tcs.datastream_fd)

Use "!= NULL".
I think there are some other cases I didn't point out.

Yao> +  /* Tracepoint number.  */
Yao> +  ctf_save_write (&data->tcs, (gdb_byte *) &tpnum, 2);

Does endianness matter?

Yao> +  /* Step 2: Write event "frame".  */
Yao> +  /* Event Id.  */
Yao> +  ctf_save_align_write (&data->tcs, (gdb_byte *) &two, 4, 4);

The comments Andreas and Pedro made apply here.

Yao> +  /* Event Id.  */
Yao> +  ctf_save_align_write (&data->tcs, (gdb_byte *) &event_id, 4, 4);
Yao> +
Yao> +  /* Address.  */
Yao> +  ctf_save_align_write (&data->tcs, (gdb_byte *) &addr, 8, 8);

Here too I think.
There are more spots as well.

Yao> +/* Operations to write various types of trace frames into CTF
Yao> +   format.  */
Yao> +
Yao> +static struct trace_frame_write_ops ctf_write_frame_ops =

Why not const?

Yao> +static struct trace_file_write_ops ctf_write_ops =

Likewise.

Yao> +extern struct trace_file_writer *ctf_trace_file_writer (void);
Yao> +#endif

Newline between these two.

Yao>  mi_cmd_trace_save (char *command, char **argv, int argc)
Yao>  {
Yao>    int target_saves = 0;
Yao> +  int generate_ctf = 0;
Yao>    char *filename;

Yao>    if (argc != 1 && argc != 2)
Yao> -    error (_("Usage: -trace-save [-r] filename"));
Yao> +    error (_("Usage: -trace-save [-r] [-ctf] filename"));

Yao>    if (argc == 2)
Yao>      {
Yao>        filename = argv[1];
Yao>        if (strcmp (argv[0], "-r") == 0)
Yao>  	target_saves = 1;
Yao> +      if (strcmp (argv[0], "-ctf") == 0)
Yao> +	generate_ctf = 1;

I wonder why this doesn't use mi_getopt.
(Not your problem unless you feel like cleaning it up.)

Yao>    add_com ("tsave", class_trace, trace_save_command, _("\
Yao>  Save the trace data to a file.\n\
Yao>  Use the '-r' option to direct the target to save directly to the file,\n\
Yao> +Use the '-ctf' option to save the data to CTF format,\n\
Yao>  using its own filesystem."));

This reads strangely after the change.
Does -ctf direct the target to use its own filesystem?
Perhaps rewriting all this into a more "--help" style would be better;
or otherwise at least repeating the final clause.

Tom