On Fri, Jul 17, 2020 at 5:06 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Fri, 17 Jul 2020 11:03:26 +0300
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@xxxxxxxxx> 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@xxxxxxxxxx>
> > + *
> > + * Updates:
> > + * Copyright (C) 2020, VMware, Tzvetomir Stoyanov <tz.stoyanov@xxxxxxxxx>
> > + *
> > + */
> > +#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
|