| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20100223211700.GA1952@jolsa.redhat.com>
Date: Tue, 23 Feb 2010 22:17:00 +0100
From: Jiri Olsa <jolsa@...hat.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH][trace-cmd] minor fixies
On Tue, Feb 23, 2010 at 11:37:57AM -0500, Steven Rostedt wrote:
> On Tue, 2010-02-23 at 16:56 +0100, Jiri Olsa wrote:
> > hi,
> >
> > I was looking through the sources and spot few bugs.
>
> Well, these are mostly clean ups, not bugs. Bugs are where things do not
> work. Clean ups are where you make it look nicer. ;-)
I'll be more accurate next time.. ;)
>
> Also, just so you know, some of these files are under LGPL and some
> under GPL, the changes to the LGPL will stay LGPL. If that's alright
> with you, otherwise, they can't be accepted.
no problem
wbr,
jirka
>
> >
> > wbr,
> > jirka
> >
> >
> > Signed-off-by: Jiri Olsa <jolsa@...hat.com>
> > ---
> > parse-events.c | 11 ++++-------
> > trace-cmd.c | 35 ++++++++++++++++++++---------------
> > trace-filter.c | 14 ++++++++------
> > trace-input.c | 2 +-
> > trace-output.c | 3 ---
> > trace-record.c | 3 ---
> > trace-util.c | 3 ---
> > 7 files changed, 33 insertions(+), 38 deletions(-)
> >
> > diff --git a/parse-events.c b/parse-events.c
> > index b483e85..43adc76 100644
> > --- a/parse-events.c
> > +++ b/parse-events.c
> > @@ -68,8 +68,6 @@ struct print_arg *alloc_arg(void)
> > struct print_arg *arg;
> >
> > arg = malloc_or_die(sizeof(*arg));
> > - if (!arg)
> > - return NULL;
>
> OK, I wasn't consistent here. I added this, because malloc_or_die() can
> be overloaded and actually return. But I probably should not let that
> happen, and if it does return, then buyer beware.
>
>
> > memset(arg, 0, sizeof(*arg));
> >
> > return arg;
> > @@ -570,12 +568,13 @@ static void add_event(struct pevent *pevent, struct event_format *event)
> >
> > if (!pevent->events)
> > pevent->events = malloc_or_die(sizeof(event));
> > - else
> > + else {
> > pevent->events =
> > realloc(pevent->events, sizeof(event) *
> > (pevent->nr_events + 1));
> > - if (!pevent->events)
> > - die("Can not allocate events");
> > + if (!pevent->events)
> > + die("Can not allocate events");
> > + }
>
> I don't like this change. Yes malloc_or_die() should not return on
> failure, but I think it looks cleaner the original way. This just
> unnecessarily adds '{' and '}' and indents when not needed.
>
>
> >
> > for (i = 0; i < pevent->nr_events; i++) {
> > if (pevent->events[i]->id > event->id)
> > @@ -3855,8 +3854,6 @@ int pevent_parse_event(struct pevent *pevent,
> > init_input_buf(buf, size);
> >
> > event = alloc_event();
> > - if (!event)
> > - return -ENOMEM;
>
> I don't like this either, this code is just being robust. No harm
> keeping it.
>
> >
> > event->name = event_read_name();
> > if (!event->name)
> > diff --git a/trace-cmd.c b/trace-cmd.c
> > index e89362d..01cf563 100644
> > --- a/trace-cmd.c
> > +++ b/trace-cmd.c
> > @@ -226,7 +226,7 @@ static int set_ftrace(int set)
> > int fd;
> > char *val = set ? "1" : "0";
> >
> > - /* if ftace_enable does not exist, simply ignore it */
> > + /* if ftrace_enable does not exist, simply ignore it */
> > fd = stat(path, &buf);
> > if (fd < 0)
> > return -ENODEV;
> > @@ -356,8 +356,6 @@ static char *get_tracing_file(const char *name)
> > }
> >
> > file = malloc_or_die(strlen(tracing) + strlen(name) + 2);
> > - if (!file)
> > - return NULL;
> >
> > sprintf(file, "%s/%s", tracing, name);
> > return file;
> > @@ -446,6 +444,22 @@ static void show_options(void)
> > fclose(fp);
> > }
> >
> > +static void show_stats(void)
> > +{
> > + int cpu;
> > + struct trace_seq s;
> > +
> > + printf("Buffer statistics:\n\n");
> > + for (cpu = 0; cpu < cpu_count; cpu++) {
> > + trace_seq_init(&s);
> > + trace_seq_printf(&s, "CPU: %d\n", cpu);
> > + tracecmd_stat_cpu(&s, cpu);
> > + trace_seq_do_printf(&s);
> > + printf("\n");
> > + }
> > +
> > +}
> > +
> > static void set_option(const char *option)
> > {
> > FILE *fp;
> > @@ -1096,9 +1110,10 @@ void usage(char **argv)
> > " Disables the tracer (may reset trace file)\n"
> > " Used in conjunction with start\n"
> > "\n"
> > - " %s report [-i file] [--cpu cpu] [-e][-f][-l][-P][-E][-F filter][-v]\n"
> > + " %s report [-i file] [--cpu cpu] [-e][-p][-f][-l][-P][-E][-F filter][-v]\n"
> > " -i input file [default trace.dat]\n"
> > " -e show file endianess\n"
> > + " -p show page size\n"
> > " -f show function list\n"
> > " -P show printk list\n"
> > " -E show event files stored\n"
> > @@ -1137,7 +1152,6 @@ int main (int argc, char **argv)
> > const char *option;
> > struct event_list *event;
> > struct event_list *last_event;
> > - struct trace_seq s;
> > int disable = 0;
> > int plug = 0;
> > int events = 0;
> > @@ -1147,7 +1161,6 @@ int main (int argc, char **argv)
> > int run_command = 0;
> > int neg_event = 0;
> > int fset;
> > - int cpu;
> >
> > int c;
> >
> > @@ -1385,6 +1398,7 @@ int main (int argc, char **argv)
> > sleep(10);
> > }
> >
> > + show_stats();
> > disable_tracing();
>
> This is wrong, the stats needs to be done after disable tracing, and
> after stop_threads. This shows what was left in the buffer.
>
> -- Steve
>
> > }
> >
> > @@ -1393,15 +1407,6 @@ int main (int argc, char **argv)
> > record_data();
> > delete_thread_data();
> >
> > - printf("Buffer statistics:\n\n");
> > - for (cpu = 0; cpu < cpu_count; cpu++) {
> > - trace_seq_init(&s);
> > - trace_seq_printf(&s, "CPU: %d\n", cpu);
> > - tracecmd_stat_cpu(&s, cpu);
> > - trace_seq_do_printf(&s);
> > - printf("\n");
> > - }
> > -
> > exit(0);
> >
> > return 0;
> > diff --git a/trace-filter.c b/trace-filter.c
> > index faf1e89..c214d25 100644
> > --- a/trace-filter.c
> > +++ b/trace-filter.c
> > @@ -2015,10 +2015,11 @@ static void add_system_str(gchar ***systems, char *system, int count)
> >
> > if (!count)
> > *systems = malloc_or_die(sizeof(char *) * 2);
> > - else
> > + else {
> > *systems = realloc(*systems, sizeof(char *) * (count + 2));
> > - if (!*systems)
> > - die("Can't allocate systems");
> > + if (!*systems)
> > + die("Can't allocate systems");
> > + }
> >
> > (*systems)[count] = system;
> > (*systems)[count+1] = NULL;
> > @@ -2031,10 +2032,11 @@ static void add_event_int(gint **events, gint event, int count)
> >
> > if (!count)
> > *events = malloc_or_die(sizeof(gint) * 2);
> > - else
> > + else {
> > *events = realloc(*events, sizeof(gint) * (count + 2));
> > - if (!*events)
> > - die("Can't allocate events");
> > + if (!*events)
> > + die("Can't allocate events");
> > + }
> >
> > (*events)[count] = event;
> > (*events)[count+1] = -1;
> > diff --git a/trace-input.c b/trace-input.c
> > index ecf86e4..cb8a64d 100644
> > --- a/trace-input.c
> > +++ b/trace-input.c
> > @@ -92,7 +92,7 @@ static int do_read(struct tracecmd_input *handle, void *data, int size)
> > int r;
> >
> > do {
> > - r = read(handle->fd, data, size - tot);
> > + r = read(handle->fd, data + tot, size - tot);
> > tot += r;
> >
> > if (!r)
> > diff --git a/trace-output.c b/trace-output.c
> > index a59099d..4386b5b 100644
> > --- a/trace-output.c
> > +++ b/trace-output.c
> > @@ -175,9 +175,6 @@ static char *get_tracing_file(struct tracecmd_output *handle, const char *name)
> > return NULL;
> >
> > file = malloc_or_die(strlen(tracing) + strlen(name) + 2);
> > - if (!file)
> > - return NULL;
> > -
> > sprintf(file, "%s/%s", tracing, name);
> > return file;
> > }
> > diff --git a/trace-record.c b/trace-record.c
> > index 8317320..cbcfff9 100644
> > --- a/trace-record.c
> > +++ b/trace-record.c
> > @@ -64,9 +64,6 @@ struct tracecmd_recorder *tracecmd_create_recorder(const char *file, int cpu)
> > int ret;
> >
> > recorder = malloc_or_die(sizeof(*recorder));
> > - if (!recorder)
> > - return NULL;
> > -
> > recorder->cpu = cpu;
> >
> > /* Init to know what to free and release */
> > diff --git a/trace-util.c b/trace-util.c
> > index 6854977..435c05d 100644
> > --- a/trace-util.c
> > +++ b/trace-util.c
> > @@ -270,9 +270,6 @@ char *tracecmd_find_tracing_dir(void)
> > }
> >
> > tracing_dir = malloc_or_die(strlen(debugfs) + 9);
> > - if (!tracing_dir)
> > - return NULL;
> > -
> > sprintf(tracing_dir, "%s/tracing", debugfs);
> >
> > return tracing_dir;
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists