Re: [PATCH v3 02/15] Save trace into CTF format

Doug Evans
2013-03-14T17:49:45Z

Hi. Just some nits.

Yao Qi writes:
> 2013-03-13 Hui Zhu <hui_zhu@mentor.com>
> Yao Qi <yao@codesourcery.com>
>
> * Makefile.in (REMOTE_OBS): Add ctf.o.
> (SFILES): Add ctf.c.
> (HFILES_NO_SRCDIR): Add ctf.h.
> * ctf.c, ctf.h: New files.
> * tracepoint.c : Include 'ctf.h'.
> (collect_pseudocommand): Remove static.
> (trace_save_command): Parse option "-ctf".
> Produce different trace file writers per option.
> Adjust output message.
> (trace_save_tfile, trace_save_ctf): New.
> * tracepoint.h (trace_save_tfile, trace_save_ctf): Declare.
> * mi/mi-main.c: Include 'ctf.h'.
> (mi_cmd_trace_save): Handle option '-ctf'. Call either
> trace_save_tfile or trace_save_ctf.

> +/* Write a unsigned 32-bit integer to datastream file represented by
> + HANDLER. */
> +
> +#define ctf_save_write_uint32(HANDLER, U32) \
> + ctf_save_write (HANDLER, (gdb_byte *) &U32, 4)

Technically speaking, HANDLE and U32 should be in parens.
Also, I think convention is that they be lowercase
(still uppercase in the comment).
[One might also just want to make this a function instead of a macro,
but either way is fine with me at least for now.]

> +/* This is the implementation of trace_file_write_ops method
> + start. It creates the directory DIRNAME, metadata and datastream
> + in the directory. */
> +
> +static void
> +ctf_start (struct trace_file_writer *self, const char *dirname)
> +{
> + char *file_name;
> + struct cleanup *old_chain;
> + struct ctf_trace_file_writer *writer
> + = (struct ctf_trace_file_writer *) self;
> + int i;
> +
> + /* Create DIRNAME. */
> + if (mkdir (dirname, S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH)
> + && errno != EEXIST)
> + error (_("Unable to open directory '%s' for saving trace data (%s)"),
> + dirname, safe_strerror (errno));

I see remote-fileio.c doesn't assume S_IXGRP,et.al. are portable.
Probably want to do the same here.