Re: [PATCH 2/2] trace-cmd: Enhance "trace-cmd clear" to be instance aware

[Tzvetomir Stoyanov <tz.stoyanov@gmail.com>]

On Fri, Jul 17, 2020 at 5:06 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 17 Jul 2020 11:03:26 +0300
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
> > --- /dev/null
> > +++ b/tracecmd/trace-clear.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: LGPL-2.1
> > +/*
> > + * Copyright (C) 2008, 2009, 2010 Red Hat Inc, Steven Rostedt <srostedt@redhat.com>
> > + *
> > + * Updates:
> > + * Copyright (C) 2020, VMware, Tzvetomir Stoyanov <tz.stoyanov@gmail.com>
> > + *
> > + */
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <getopt.h>
> > +
> > +#include "tracefs.h"
> > +#include "trace-local.h"
> > +
> > +struct instances_list {
> > +     struct instances_list *next;
> > +     struct tracefs_instance *instance;
> > +};
> > +
> > +static int add_new_instance(struct instances_list **list, char *name)
> > +{
> > +     struct instances_list *new;
> > +
> > +     new = calloc(1, sizeof(*new));
> > +     if (!new)
> > +             return -1;
> > +     new->instance = tracefs_instance_alloc(name);
> > +     if (!new->instance) {
> > +             free(new);
> > +             return -1;
> > +     }
> > +
> > +     new->next = *list;
> > +     *list = new;
> > +     return 0;
> > +}
> > +
> > +static int add_instance_walk(const char *name, void *data)
> > +{
> > +     return add_new_instance((struct instances_list **)data, (char *)name);
> > +}
> > +
> > +static void clear_list(struct instances_list *list)
> > +{
> > +     struct instances_list *del;
> > +
> > +     while (list) {
> > +             del = list;
> > +             list = list->next;
> > +             tracefs_instance_free(del->instance);
> > +             free(del);
> > +     }
> > +}
>
> BTW, why create this list? (see below).
>
> > +
> > +static void clear_instance_trace(struct tracefs_instance *instance)
> > +{
> > +     FILE *fp;
> > +     char *path;
> > +
> > +     /* reset the trace */
> > +     path = tracefs_instance_get_file(instance, "trace");
> > +     fp = fopen(path, "w");
> > +     if (!fp)
> > +             die("writing to '%s'", path);
> > +     tracefs_put_tracing_file(path);
> > +     fwrite("0", 1, 1, fp);
> > +     fclose(fp);
> > +}
> > +
> > +static void clear_trace(struct instances_list *instances)
> > +{
> > +     if (instances) {
> > +             while (instances) {
> > +                     clear_instance_trace(instances->instance);
> > +                     instances = instances->next;
> > +             }
> > +     } else
> > +             clear_instance_trace(NULL);
> > +}
> > +
> > +void trace_clear(int argc, char **argv)
> > +{
> > +     struct instances_list *instances = NULL;
> > +     bool all = false;
> > +     int c;
> > +
> > +     for (;;) {
> > +             int option_index = 0;
> > +             static struct option long_options[] = {
> > +                     {"all", no_argument, NULL, 'a'},
> > +                     {"help", no_argument, NULL, '?'},
> > +                     {NULL, 0, NULL, 0}
> > +             };
> > +
> > +             c = getopt_long (argc-1, argv+1, "+haB:",
> > +                              long_options, &option_index);
> > +             if (c == -1)
> > +                     break;
> > +             switch (c) {
> > +             case 'B':
> > +                     if (add_new_instance(&instances, optarg))
> > +                             die("Failed to allocate instance %s", optarg);
> > +                     break;
> > +             case 'a':
> > +                     all = true;
>
> All you need to do is set the flag, and don't create the list here.
>
> > +                     if (tracefs_instances_walk(add_instance_walk, &instances))
> > +                             die("Failed to add all instances");
> > +                     break;
> > +             case 'h':
> > +             case '?':
> > +             default:
> > +                     usage(argv);
> > +                     break;
> > +             }
> > +     }
> > +
> > +     clear_trace(instances);
> > +     if (all)
>
> Then here, you would just do:
>
>                 tracefs_instances_iterate(clear_instance_iter, NULL);
>
> Where we have:
>
> static int clear_instance_iter(const char *name, void *data)
> {
>         tracefs_clear(name);
> }
>
> No need for the creating of the list.

There are  two reasons for creating the list:
 - For consistency with the other flow, the"-B" option.
 - The tracefs APIs require "struct tracefs_instance" as input
parameter to identify the instance, do not work with instance name
The list is not needed for "--all", but still needs to create "struct
tracefs_instance" and to pass it to tracefs_instance_get_file() API


>
> -- Steve
>
>
> > +             clear_trace(NULL);
> > +     clear_list(instances);
> > +     exit(0);
> > +}
> > +
> > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> > index bd004574..5009eaa1 100644
> > --- a/tracecmd/trace-record.c
> > +++ b/tracecmd/trace-record.c
> > @@ -835,21 +835,6 @@ static void clear_trace_instances(void)
> >               __clear_trace(instance);
> >  }
> >
> > -static void clear_trace(void)
> > -{
> > -     FILE *fp;
> > -     char *path;
> > -
> > -     /* reset the trace */
> > -     path = tracefs_get_tracing_file("trace");
> > -     fp = fopen(path, "w");
> > -     if (!fp)
> > -             die("writing to '%s'", path);
> > -     tracefs_put_tracing_file(path);
> > -     fwrite("0", 1, 1, fp);
> > -     fclose(fp);
> > -}
> > -
> >  static void reset_max_latency(struct buffer_instance *instance)
> >  {
> >       tracefs_instance_file_write(instance->tracefs,
> > @@ -6704,15 +6689,6 @@ void trace_profile(int argc, char **argv)
> >       exit(0);
> >  }
> >
> > -void trace_clear(int argc, char **argv)
> > -{
> > -     if (argc > 2)
> > -             usage(argv);
> > -     else
> > -             clear_trace();
> > -     exit(0);
> > -}
> > -
> >  void trace_record(int argc, char **argv)
> >  {
> >       struct common_record_context ctx;
> > diff --git a/tracecmd/trace-usage.c b/tracecmd/trace-usage.c
> > index 3f0b2d07..79610bd3 100644
> > --- a/tracecmd/trace-usage.c
> > +++ b/tracecmd/trace-usage.c
> > @@ -180,7 +180,9 @@ static struct usage_help usage_help[] = {
> >       {
> >               "clear",
> >               "clear the trace buffers",
> > -             " %s clear\n"
> > +             " %s clear [-B buf][-a]\n"
> > +             "          -B clear the given buffer (may specify multiple -B)\n"
> > +             "          -a clear all existing buffers, including the top level one\n"
> >       },
> >       {
> >               "report",
>


-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center