From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13627 invoked by alias); 1 Mar 2013 19:22:50 -0000 Received: (qmail 13617 invoked by uid 22791); 1 Mar 2013 19:22:50 -0000 X-SWARE-Spam-Status: No, hits=-6.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_RV,TW_TR,TW_VF X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 01 Mar 2013 19:22:45 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r21JMhR3008503 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 1 Mar 2013 14:22:43 -0500 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r21JMfRI031856 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 1 Mar 2013 14:22:42 -0500 From: Tom Tromey To: Yao Qi Cc: Subject: Re: [PATCH 2/5] Save trace into CTF format References: <1361931459-3953-1-git-send-email-yao@codesourcery.com> <1361931459-3953-3-git-send-email-yao@codesourcery.com> <512F38CE.5030802@codesourcery.com> Date: Fri, 01 Mar 2013 19:22:00 -0000 In-Reply-To: <512F38CE.5030802@codesourcery.com> (Yao Qi's message of "Thu, 28 Feb 2013 19:00:30 +0800") Message-ID: <87621avsam.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2.92 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2013-03/txt/msg00041.txt.bz2 >>>>> "Yao" == Yao Qi 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