On Wed, Dec 4, 2019 at 6:17 PM Steven Rostedt <rostedt@xxxxxxxxxxx> 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
|