This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]