Re: [PATCH v17 04/18] trace-cmd: Add new library APIs for ftrace instances.

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

On Wed, Dec 4, 2019 at 6:17 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
[ ... ]
> > +     do {
> > +             r = read(fd, buffer, BUFSIZ);
> > +             if (r <= 0)
> > +                     continue;
> > +             if (size)
> > +                     buf = realloc(buf, size+r+1);
> > +             else
> > +                     buf = malloc(r+1);
>
> I know this is only cut and pasted from what the original was, but we
> should probably (either in this patch, or perhaps a separate one) fix
> the above. As realloc(NULL, x) is equivalent to malloc(x), we can just
> have:
>
>                 buf = realloc(buf, size+r+1);
>
>
> > +             if (!buf)
> > +                     die("Failed to allocate instance file buffer");
>
> As this is becoming a library function, we should remove "die()", as
> that is only allowed in the application. Not library calls.
>
> That would also mean we need to clean up anything we allocated on
> failure, and that the realloc would need to be:
>
>                 new_buf = realloc(buf, size + r + 1);
>
>                 if (!new_buf) {
>                         free(buf);
>                         return NULL;
>
> All the functions that are being moved into the library needs to have
> their die() calls removed. This is OK to do in a separate patch.
>
> Preferably right after this patch (so reviewers know it's happening).
>
I fixed most of the comments, except those die() call. There are a lot
of things that
should be fixed in the existing libtracecmd code, in order to become a
real library.
Also, I started to work on a new library providing APIs for
interaction with files
from tracefs, libtraceftrace (this is only a work name, we should
think about more suitable one).
I would suggest changes from this patch set to be considered only as
part of the time stamp sync
feature. There will be at least two library patch sets - for cleaning
up the existing libtracecmd code and
for the new ftrace library.

>
> > +             memcpy(buf+size, buffer, r);
> > +             size += r;
> > +     } while (r);
> > +
> > +     buf[size] = '\0';
> > +     if (psize)
> > +             *psize = size;
> > +     return buf;
> > +}
> > +
> > +/**
> > + * tracecmd_set_clock - Set the clock of ftrace event's timestamps, per instance.
> > + * @instance: Pointer to ftrace instance, containing the desired clock.
> > + * @old_clock: Optional, return the newly allocated string with the old clock.
>
> This looks to be just a copy of the old code and forced into a library
> function. As a library function, it should return an "int" and be
> passed the clock name directly. Not via the instance->clock.

The idea of this API is to apply the clock, already set to the
instance. This follows
the existing logic of configuring the instance clock, that's why I put
the "char *clock"
in the new "struct tracecmd_instance". We can remove the clock from
the structure
and pass it as an argument to tracecmd_set_clock(), so the  "struct
tracecmd_instance"
will hold only the instance name ?

>
> -- Steve
>
> > + *
> > + */
> > +void tracecmd_set_clock(struct tracecmd_instance *instance, char **old_clock)
> > +{
> > +     char *content;
> > +     char *str;
> > +
> > +     if (!instance->clock)
> > +             return;
> > +
> > +     /* The current clock is in brackets, reset it when we are done */
> > +     content = tracecmd_read_instance_file(instance, "trace_clock", NULL);
> > +
> > +     /* check if first clock is set */
> > +     if (*content == '[')
> > +             str = strtok(content+1, "]");
> > +     else {
> > +             str = strtok(content, "[");
> > +             if (!str)
> > +                     die("Can not find clock in trace_clock");
> > +             str = strtok(NULL, "]");
> > +     }
> > +     if (old_clock)
> > +             *old_clock = strdup(str);
> > +
> > +     free(content);
> > +     tracecmd_write_instance_file(instance,
> > +                                  "trace_clock", instance->clock, "clock");
> > +}
> >

Thanks !

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