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

Tom Tromey
2013-03-01T19:22:50Z

>>>>> "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