Selon Mathieu Desnoyers <[email protected]>:

> * Vincent Attard ([email protected]) wrote:
> > This new plugin prints a formated output of each events in a trace. The
> > output format is defined as a parameter with printf like syntax.It provides
> > a default format easy to read and the original textDump format for backward
> > compatibility.
> > Updated: Use glib functions for solving buffer overflow problem
> >
> > Signed-off-by: Vincent Attard <[email protected]>
> > Reviewed-by: Yannick Brosseau <[email protected]>
> > ---
> >  lttv/modules/text/Makefile.am    |    3 +-
> >  lttv/modules/text/formatedDump.c |  352
> ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 354 insertions(+), 1 deletions(-)
> >  create mode 100644 lttv/modules/text/formatedDump.c
> >
> > diff --git a/lttv/modules/text/Makefile.am b/lttv/modules/text/Makefile.am
> > index 29e8990..d95a4a0 100644
> > --- a/lttv/modules/text/Makefile.am
> > +++ b/lttv/modules/text/Makefile.am
> > @@ -3,7 +3,7 @@ AM_LDFLAGS = $(MODULE_LDFLAGS)
> >
> >  libdir = ${lttvplugindir}
> >
> > -lib_LTLIBRARIES = libtextDump.la libbatchAnalysis.la libtextFilter.la
> libprecomputeState.la libdepanalysis.la libsync_chain_batch.la
> > +lib_LTLIBRARIES = libtextDump.la libbatchAnalysis.la libtextFilter.la
> libprecomputeState.la libdepanalysis.la libsync_chain_batch.la
> libformatedDump.la
> >
> >  libtextDump_la_SOURCES = textDump.c
> >  libbatchAnalysis_la_SOURCES = batchAnalysis.c
> > @@ -11,6 +11,7 @@ libtextFilter_la_SOURCES = textFilter.c
> >  libprecomputeState_la_SOURCES = precomputeState.c
> >  libdepanalysis_la_SOURCES = depanalysis.c sstack.c
> >  libsync_chain_batch_la_SOURCES = sync_chain_batch.c
> > +libformatedDump_la_SOURCES = formatedDump.c
> >
> >  noinst_HEADERS = \
> >     batchanalysis.h \
> > diff --git a/lttv/modules/text/formatedDump.c
> b/lttv/modules/text/formatedDump.c
> > new file mode 100644
> > index 0000000..58ccdb8
> > --- /dev/null
> > +++ b/lttv/modules/text/formatedDump.c
> > @@ -0,0 +1,352 @@
> > +/* This file is part of the Linux Trace Toolkit viewer
> > + * Copyright (C) 2003-2004 Michel Dagenais
> > + *               2005 Mathieu Desnoyers
> > + *               2011 Vincent Attard
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License Version 2 as
> > + * published by the Free Software Foundation;
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place - Suite 330, Boston,
> > + * MA 02111-1307, USA.
> > + */
> > +
> > +/* Formated dump plugin prints a formated output of each events in a
> trace. The
>
>
> Please use:
>
> /*
>  * blah blah
>  * blah
>  */
>
> rather than
>
> /* blah
>  * blah
>  */
>
> for multi-line comments.
>

Sorry for my wrong indentation and for my english spelling mistakes, problem
solved!

> > + * output format is defined as a parameter. It provides a default format
> easy to
> > + * read and the original textDump format for backward compatibility.
> > + */
> > +
> > +#ifdef HAVE_CONFIG_H
> > +#include <config.h>
> > +#endif
> > +
> > +#include <lttv/lttv.h>
> > +#include <lttv/option.h>
> > +#include <lttv/module.h>
> > +#include <lttv/hook.h>
> > +#include <lttv/attribute.h>
> > +#include <lttv/iattribute.h>
> > +#include <lttv/stats.h>
> > +#include <lttv/filter.h>
> > +#include <lttv/print.h>
> > +#include <ltt/ltt.h>
> > +#include <ltt/event.h>
> > +#include <ltt/trace.h>
> > +#include <stdio.h>
> > +#include <inttypes.h>
> > +#include <string.h>
> > +#include <stdlib.h>
> > +
> > +static gboolean a_noevent;
> > +static gboolean a_no_field_names;
> > +static gboolean a_state;
> > +static gboolean a_text;
> > +
> > +static char *a_file_name = NULL;
> > +static char *a_format = NULL;
> > +
> > +static LttvHooks *before_traceset;
> > +static LttvHooks *event_hook;
> > +
> > +static char default_format[] =
> > +           "tracefile:%f envent:%e timestamp:%t elapse:%l cpu:%c pid:%d 
> > tgpid:%g "\
> > +           "process:%p brand:%b ppid:%i state:%a";
>
> static const char for the above.
>
> > +static char textDump_format[] =
> > +           "%f.%e: %s.%n (%r/%f_%c), %d, %g, %p, %b, %i, %y, %a { %m }";
>
> same here.
>
> > +
> > +static FILE *a_file;
> > +
> > +static GString *a_string;
> > +
> > +static gboolean open_output_file(void *hook_data, void *call_data)
> > +{
> > +  g_info("Open the output file");
> > +
> > +  if (a_file_name == NULL) a_file = stdout;
>
> never put code on the same line as a if ().
>
> > +    else a_file = fopen(a_file_name, "w");
>
> Indentation is wrong.
>
> > +
> > +     if (a_file == NULL) g_error("cannot open file %s", a_file_name);
> > +      return FALSE;
>
> here too.
>
> > +}
> > +
> > +static int write_event_content(void *hook_data, void *call_data)
> > +{
> > +   gboolean result;
> > +
> > +   LttvIAttribute *attributes = LTTV_IATTRIBUTE(lttv_global_attributes());
> > +
> > +   LttvTracefileContext *tfc = (LttvTracefileContext *)call_data;
> > +
> > +   LttvTracefileState *tfs = (LttvTracefileState *)call_data;
> > +
> > +   LttEvent *e;
> > +
> > +   LttvAttributeValue value_filter;
> > +
> > +   LttvFilter *filter;
> > +
> > +   guint cpu = tfs->cpu;
> > +   LttvTraceState *ts = (LttvTraceState*)tfc->t_context;
> > +   LttvProcessState *process = ts->running_process[cpu];
> > +
> > +   if (a_noevent)
> > +           return FALSE;
> > +
> > +   e = ltt_tracefile_get_event(tfc->tf);
> > +
> > +   result = lttv_iattribute_find_by_path(attributes, "filter/lttv_filter",
> > +                   LTTV_POINTER, &value_filter);
> > +   g_assert(result);
> > +   filter = (LttvFilter*)*(value_filter.v_pointer);
> > +
> > +   /*
> > +    * call to the filter if available
> > +    */
> > +   if (filter->head != NULL)
> > +           if (!lttv_filter_tree_parse(filter->head,e,tfc->tf,
> > +                           tfc->t_context->t,tfc,NULL,NULL))
> > +                   return FALSE;
> > +
> > +   lttv_event_to_string(e, a_string, TRUE, !a_no_field_names, tfs);
> > +
> > +   if (a_state) {
> > +           g_string_append_printf(a_string, "%s ",
> > +                           g_quark_to_string(process->state->s));
> > +   }
> > +
> > +
> > +   g_string_append_printf(a_string,"\n");
> > +
> > +   fputs(a_string->str, a_file);
> > +   return FALSE;
> > +}
> > +
> > +void lttv_event_to_string(LttEvent *e, GString *string_buffer, gboolean
> mandatory_fields,
> > +           gboolean field_names, LttvTracefileState *tfs)
> > +{
>
> There is an extra whitespace at the end of this line.
>
> > +   struct marker_field *field;
> > +   struct marker_info *info;
> > +   LttTime time;
> > +   static LttTime time_prev = {0,0};
>
> This static value will be hard to port into a librarized "print.c"-like
> version of the formatted output (usable by the GUI). It should be added
> into state.c instead, and reset each time we do a seek.
>

I let a 'TODO' for it in my code because I don't want to make a mistake in
state.c but if it is quite simple I can try it.

> > +   LttTime elapse;
> > +
> > +
> > +   guint cpu = tfs->cpu;
> > +   LttvTraceState *ts = (LttvTraceState*)tfs->parent.t_context;
> > +   LttvProcessState *process = ts->running_process[cpu];
> > +
> > +   info = marker_get_info_from_id(tfs->parent.tf->mdata, e->event_id);
> > +   if (mandatory_fields) {
> > +           time = ltt_event_time(e);
> > +   /* Calculate elapsed time between current and previous event */
> > +           if (time_prev.tv_sec == 0 && time_prev.tv_nsec == 0) {
> > +                   time_prev = ltt_event_time(e);
>
> The time delta for the first event is 0. We have to keep in mind that
> we should add the ability to restore the previous event time to state.c
> if we want to reuse this code into the GUI.
>

I did the same thing for this one.

> > +                   elapse.tv_sec = 0;
> > +                   elapse.tv_nsec = 0;
> > +           } else {
> > +                   elapse = ltt_time_sub(time, time_prev);
> > +                   time_prev = time;
> > +           }
> > +   }
> > +
> > +   char * fmt;
>
> char *fmt;
>
> for consistency.
>
> > +   int i;
> > +
> > +   if (a_text) {
> > +           /* textDump format (used with -T command option) */
> > +           fmt = textDump_format;
> > +   }
> > +
> > +   else if (!a_format) {
> > +           /* Default format (used if no option) */
> > +           fmt = default_format;
> > +   } else {
> > +           /* formatedDump format
> > +            * (used with -F command option following by the desired 
> > format) */
> > +           fmt = a_format;
> > +   }
> > +
> > +   g_string_set_size(string_buffer, 0);
> > +   /* Switch case:
>
> Please use
>
> /*
>  * ...
>  */
>
> comment style.
>
> > +    * all '%-' are replaced by the desired value in 'string_buffer' */
> > +   for (i = 0; i < strlen(fmt); i++) {
>
> You recompute the strlen of fmt for each loop ? You should compute it
> once and store the result in a local variable instead.
>
> > +           if (fmt[i] == '%') {
> > +                   switch (fmt[++i]) {
> > +                   case 't':
> > +                           g_string_append_printf(string_buffer, 
> > "%ldh%02ldm%02lds%09ldns",
> > +                                           time.tv_sec/3600, 
> > (time.tv_sec%3600)/60, time.tv_sec%60,
> > +                                           time.tv_nsec);
> > +                           break;
> > +                   case 'f':
> > +                           g_string_append(string_buffer,
> > +                                           
> > g_quark_to_string(ltt_tracefile_name(tfs->parent.tf)));
> > +                           break;
> > +                   case 'e':
> > +                           g_string_append(string_buffer,
> > +                                           g_quark_to_string(info->name));
> > +                           break;
> > +                   case 'd':
> > +                           g_string_append_printf(string_buffer, "%u", 
> > process->pid);
> > +                           break;
> > +                   case 's':
> > +                           g_string_append_printf(string_buffer, "%ld", 
> > time.tv_sec);
> > +                           break;
> > +                   case 'n':
> > +                           g_string_append_printf(string_buffer, "%ld", 
> > time.tv_nsec);
> > +                           break;
> > +                   case 'i':
> > +                           g_string_append_printf(string_buffer, "%u", 
> > process->ppid);
> > +                           break;
> > +                   case 'g':
> > +                           g_string_append_printf(string_buffer, "%u", 
> > process->tgid);
> > +                           break;
> > +                   case 'p':
> > +                           g_string_append(string_buffer,
> > +                                           
> > g_quark_to_string(process->name));
> > +                           break;
> > +                   case 'b':
> > +                           g_string_append(string_buffer,
> > +                                           
> > g_quark_to_string(process->brand));
> > +                           break;
> > +                   case 'c':
> > +                           g_string_append_printf(string_buffer, "%u", 
> > cpu);
> > +                           break;
> > +                   case 'l':
> > +                           g_string_append_printf(string_buffer, 
> > "%lds%09ldns",
> > +                                           elapse.tv_sec, elapse.tv_nsec);
> > +                           break;
> > +                   case 'a':
> > +                           g_string_append(string_buffer,
> > +                                           
> > g_quark_to_string(process->state->t));
> > +                           break;
> > +                   case 'm':
> > +                           {
> > +                                   /* Get and print markers and 
> > tracepoints fields into 'marker' */
> > +                                   if (marker_get_num_fields(info) == 0) 
> > break;
>
> break on following line.
>
> > +                                   for (field = marker_get_field(info, 0);
> > +                                                   field != 
> > marker_get_field(info, marker_get_num_fields(info));
>
> Here you call marker_get_field() for each loop iteration -- you should
> save the result instead.
>

I don't know if it would optimize something, I let it like found in print.c

> > +                                                   field++) {
> > +                                           if (field != 
> > marker_get_field(info, 0)) {
>
> Same here for marker_get_field(info, 0).
>
> > +                                                   
> > g_string_append(string_buffer, ", ");
> > +                                           }
>
> the { } are maybe a bit much for a single line.
>
> > +                                           lttv_print_field(e, field, 
> > string_buffer, field_names, tfs);
> > +                                   }
> > +                           }
> > +                           break;
> > +                   case 'r':
> > +                           g_string_append(string_buffer, 
> > g_quark_to_string(
> > +                                           
> > ltt_trace_name(ltt_tracefile_get_trace(tfs->parent.tf))));
> > +                           break;
> > +                   case '%':
> > +                           g_string_append_c(string_buffer, '%');
> > +                           break;
> > +                   case 'y':
> > +                           g_string_append_printf(string_buffer, "0x%" 
> > PRIx64,
> > +                                           process->current_function);
> > +                           break;
> > +
> > +                   }
> > +           }
> > +           else
> > +           {
>
> Please use the
>
> } else {
>
> kernel coding style.
>
> > +                   /* Copy every character if not equals to '%' */
> > +                   g_string_append_c(string_buffer, fmt[i]);
> > +           }
> > +   }
> > +}
> > +
> > +static void init()
> > +{
> > +   gboolean result;
> > +
> > +   LttvAttributeValue value;
> > +
> > +   LttvIAttribute *attributes = LTTV_IATTRIBUTE(lttv_global_attributes());
> > +
> > +   g_info("Init formatedDump.c");
> > +
> > +   a_string = g_string_new("");
> > +
> > +   a_file_name = NULL;
> > +   lttv_option_add("output", 'o',
> > +                   "output file where the text is written",
> > +                   "file name",
> > +                   LTTV_OPT_STRING, &a_file_name, NULL, NULL);
> > +
> > +   a_format= NULL;
> > +   lttv_option_add("format", 'F',
> > +                   "output the desired format\n\
> > +                   FORMAT controls the output.  Interpreted sequences 
> > are:\n\
> > +                   \n\
> > +                   %f   channel name\n\
> > +                   %p   process name\n\
> > +                   %e   event name\n\
> > +                   %r   path to trace\n\
> > +                   %t   timestamp  (e.g., 2h08m54s025684145ns)\n\
> > +                   %s   seconds\n\
> > +                   %n   nanoseconds\n\
> > +                   %l   elapsed time with the previous event\n\
> > +                   %d   pid\n\
> > +                   %i   ppid\n\
> > +                   %g   tgid\n\
> > +                   %c   cpu\n\
> > +                   %b   brand\n\
> > +                   %a   state\n\
> > +                   %y   memory address\n\
> > +                   %m   markers and tracepoints fields\n",
> > +                   "format string",
> > +                   LTTV_OPT_STRING, &a_format, NULL, NULL);
> > +
> > +   a_text = FALSE;
> > +   lttv_option_add("text", 'T',
> > +                   "output the textDump format",
> > +                   "",
> > +                   LTTV_OPT_NONE, &a_text, NULL, NULL);
> > +
> > +   result = lttv_iattribute_find_by_path(attributes, "hooks/event",
> > +                   LTTV_POINTER, &value);
> > +   g_assert(result);
> > +   event_hook = *(value.v_pointer);
> > +   g_assert(event_hook);
> > +   lttv_hooks_add(event_hook, write_event_content, NULL, 
> > LTTV_PRIO_DEFAULT);
> > +
> > +   result = lttv_iattribute_find_by_path(attributes,
> "hooks/traceset/before",
> > +                   LTTV_POINTER, &value);
> > +   g_assert(result);
> > +   before_traceset = *(value.v_pointer);
> > +   g_assert(before_traceset);
> > +   lttv_hooks_add(before_traceset, open_output_file, NULL,
> > +                   LTTV_PRIO_DEFAULT);
> > +
> > +}
> > +
> > +static void destroy()
> > +{
> > +   g_info("Destroy formatedDump");
> > +
> > +   lttv_option_remove("format");
> > +
> > +   lttv_option_remove("output");
> > +
> > +   lttv_option_remove("text");
> > +
> > +   g_string_free(a_string, TRUE);
> > +
> > +   lttv_hooks_remove_data(event_hook, write_event_content, NULL);
> > +
> > +   lttv_hooks_remove_data(before_traceset, open_output_file, NULL);
> > +
> > +}
> > +
> > +
> > +LTTV_MODULE("formatedDump", "Print events with desired format in a file",
> \
>
> formatedDump -> formattedDump (all across the file)
>

I am really sorry for that! But now it's done.

> Thanks,
>
> Mathieu
>
> > +           "Produce a detailed formated text printout of a trace", \
> > +           init, destroy, "batchAnalysis", "option")
> > +
> > --
> > 1.7.0.4
> >
> >
> > _______________________________________________
> > ltt-dev mailing list
> > [email protected]
> > http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev
> >
>
> --
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com
>


--
Vincent Attard


_______________________________________________
ltt-dev mailing list
[email protected]
http://lists.casi.polymtl.ca/cgi-bin/mailman/listinfo/ltt-dev

Reply via email to