Re: [PATCH v2 3/5] trace-cmd: New libtracefs APIs for ftrace instances

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

On Wed, Jan 8, 2020 at 9:37 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon,  6 Jan 2020 17:40:56 +0200
> "Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:
>
> > The functionality related to ftrace instances is moved from
> > trace-cmd application to libtracefs. The following new
> > library APIs are introduced:
> >     struct tracefs_instance;
> >     struct tracefs_instance *tracefs_alloc_instance(const char *name);
> >     int tracefs_free_instance(struct tracefs_instance *instance);
> >     int tracefs_make_instance(struct tracefs_instance *instance);
> >     int tracefs_remove_instance(struct tracefs_instance *instance);
>
> I'm thinking that "make" should be "create" and "remove" should be
> "destroy". The "make" and "remove" does match "mkdir" and "rmdir" but
> that's just the behind-the-scenes of what the OS does. But from an
> application point of view, "create" and "destroy" seem more
> appropriate.
>
> What do you think?
>
>
> >     char *tracefs_get_instance_name(struct tracefs_instance *instance);
> >     char *tracefs_get_instance_file(struct tracefs_instance *instance, const char *file);
> >     char *tracefs_get_instance_dir(struct tracefs_instance *instance);
> >     int tracefs_write_instance_file(struct tracefs_instance *instance,
> >                                const char *file, const char *str, const char *type);
> >     char *tracefs_read_instance_file(struct tracefs_instance *instance, char *file, int *psize);
>
> Also, another naming convention used commonly, is when you have a set
> of functions working on the same thing, is to use the name of what it
> is working on first. That is, instead of the above, we should have:
>
>  tracefs_instance_alloc()
>  tracefs_instance_free()
>  tracefs_instance_create() (instead of "make")
>  tracefs_instance_destroy() (instead of "remove")
>  tracefs_instance_get_name()
>  tracefs_instance_get_file()
>  tracefs_instance_file_read()
>  tracefs_instance_file_write()
>
> That way it is easier to find these functions with just searching for
> "tracefs_instance"
>
> The "get_*" is ok to keep, but as I showed above, I think "_file_read"
> and "_file_write" is a better name.
>
> [ more below ]
>
> >
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> > ---
> >  include/tracefs/tracefs.h           |  17 ++
> >  lib/tracefs/Makefile                |   1 +
> >  lib/tracefs/include/tracefs-local.h |   1 +
> >  lib/tracefs/tracefs-instance.c      | 267 ++++++++++++++++++++++++++++
> >  lib/tracefs/tracefs-utils.c         |  43 +++++
> >  tracecmd/include/trace-local.h      |   5 +-
> >  tracecmd/trace-list.c               |   2 +-
> >  tracecmd/trace-record.c             | 264 +++++++++------------------
> >  tracecmd/trace-show.c               |   2 +
> >  tracecmd/trace-stat.c               |  21 ++-
> >  10 files changed, 431 insertions(+), 192 deletions(-)
> >  create mode 100644 lib/tracefs/tracefs-instance.c
> >
> > diff --git a/include/tracefs/tracefs.h b/include/tracefs/tracefs.h
> > index e844c75..6b27ff7 100644
> > --- a/include/tracefs/tracefs.h
> > +++ b/include/tracefs/tracefs.h
> > @@ -17,4 +17,21 @@ const char *tracefs_get_tracing_dir(void);
> >  /* tracefs_find_tracing_dir must be freed */
> >  char *tracefs_find_tracing_dir(void);
> >
> > +/* ftarce instances */
> > +struct tracefs_instance;
> > +
> > +struct tracefs_instance *tracefs_alloc_instance(const char *name);
> > +int tracefs_free_instance(struct tracefs_instance *instance);
> > +int tracefs_make_instance(struct tracefs_instance *instance);
> > +int tracefs_remove_instance(struct tracefs_instance *instance);
> > +char *tracefs_get_instance_name(struct tracefs_instance *instance);
> > +char *
> > +tracefs_get_instance_file(struct tracefs_instance *instance, const char *file);
> > +char *tracefs_get_instance_dir(struct tracefs_instance *instance);
> > +int tracefs_write_instance_file(struct tracefs_instance *instance,
> > +                             const char *file, const char *str,
> > +                             const char *type);
> > +char *tracefs_read_instance_file(struct tracefs_instance *instance,
> > +                              char *file, int *psize);
> > +
> >  #endif /* _TRACE_FS_H */
> > diff --git a/lib/tracefs/Makefile b/lib/tracefs/Makefile
> > index 86d7845..4030272 100644
> > --- a/lib/tracefs/Makefile
> > +++ b/lib/tracefs/Makefile
> > @@ -8,6 +8,7 @@ DEFAULT_TARGET = $(bdir)/libtracefs.a
> >
> >  OBJS =
> >  OBJS += tracefs-utils.o
> > +OBJS += tracefs-instance.o
> >
> >  OBJS := $(OBJS:%.o=$(bdir)/%.o)
> >  DEPS := $(OBJS:$(bdir)/%.o=$(bdir)/.%.d)
> > diff --git a/lib/tracefs/include/tracefs-local.h b/lib/tracefs/include/tracefs-local.h
> > index 231edd1..fe327a0 100644
> > --- a/lib/tracefs/include/tracefs-local.h
> > +++ b/lib/tracefs/include/tracefs-local.h
> > @@ -8,5 +8,6 @@
> >
> >  /* Can be overridden */
> >  void warning(const char *fmt, ...);
> > +int str_read_file(const char *file, char **buffer);
> >
> >  #endif /* _TRACE_FS_LOCAL_H */
> > diff --git a/lib/tracefs/tracefs-instance.c b/lib/tracefs/tracefs-instance.c
> > new file mode 100644
> > index 0000000..14e8eed
> > --- /dev/null
> > +++ b/lib/tracefs/tracefs-instance.c
> > @@ -0,0 +1,267 @@
> > +// SPDX-License-Identifier: LGPL-2.1
> > +/*
> > + * Copyright (C) 2008, 2009, 2010 Red Hat Inc, Steven Rostedt <srostedt@redhat.com>
> > + *
> > + * Updates:
> > + * Copyright (C) 2019, VMware, Tzvetomir Stoyanov <tz.stoyanov@gmail.com>
> > + *
> > + */
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <errno.h>
> > +#include <sys/stat.h>
> > +#include <fcntl.h>
> > +
> > +#include "tracefs.h"
> > +#include "tracefs-local.h"
> > +
> > +struct tracefs_instance {
> > +     char *name;
> > +};
> > +
> > +/**
> > + * tracefs_alloc_instance - allocate a new ftrace instance
> > + * @name: The name of the instance (instance will point to this)
> > + *
> > + * Returns a newly allocated instance, or NULL in case of an error.
> > + */
> > +struct tracefs_instance *tracefs_alloc_instance(const char *name)
> > +{
> > +     struct tracefs_instance *instance;
> > +
> > +     instance = calloc(1, sizeof(*instance));
> > +     if (!instance)
> > +             return NULL;
> > +     if (name)
> > +             instance->name = strdup(name);
>
> We can remove make this a little simpler (and with the name check):
>
>         instance = calloc(1, sizeof(*instance));
>         if (instance && name) {
>                 instance->name = strdup(name);
>                 if (!instance->name) {
>                         free(instance);
>                         instance = NULL;
>                 }
>         }
>
>         return instance;
>
> > +
> > +     return instance;
> > +}
> > +
> > +/**
> > + * tracefs_free_instance - Free an instance, previously allocated by
> > +                        tracefs_alloc_instance()
> > + * @instance: Pointer to the instance to be freed
> > + *
> > + * Returns -1 in case of an error, or 0 otherwise.
>
> I don't think the free should return a value. Make it void.
>
> > + */
> > +int tracefs_free_instance(struct tracefs_instance *instance)
> > +{
> > +     if (!instance)
> > +             return -1;
> > +
> > +     free(instance->name);
> > +     free(instance);
> > +     return 0;
> > +}
> > +
> > +/**
> > + * tracefs_make_instance - Create a new ftrace instance
> > + * @instance: Pointer to the instance to be created
> > + *
> > + * Returns 1 if the instance already exist, 0 if the instance
> > + * is created successful or -1 in case of an error
> > + */
> > +int tracefs_make_instance(struct tracefs_instance *instance)
> > +{
> > +     struct stat st;
> > +     char *path;
> > +     int ret;
> > +
> > +     path = tracefs_get_instance_dir(instance);
> > +     ret = stat(path, &st);
> > +     if (ret < 0) {
> > +             ret = mkdir(path, 0777);
> > +             if (ret < 0)
> > +                     return ret;
>
> Remove the check of ret here, as the return skips the freeing of path.
>
> Should be just:
>
>         if (ret < 0)
>                 ret = mkdir(path, 0777);
>         else
>                 ret = 1;
>
> > +
> > +     } else
> > +             ret = 1;
> > +     tracefs_put_tracing_file(path);
> > +     return ret;
> > +}
> > +
> > +/**
> > + * tracefs_remove_instance - Remove a ftrace instance
> > + * @instance: Pointer to the instance to be removed
> > + *
> > + * Returns -1 in case of an error, or 0 otherwise.
> > + */
> > +int tracefs_remove_instance(struct tracefs_instance *instance)
> > +{
> > +     char *path;
> > +     int ret;
> > +
> > +     if (!instance || !instance->name) {
> > +             warning("Cannot remove top instance");
> > +             return -1;
> > +     }
> > +
> > +     path = tracefs_get_instance_dir(instance);
>
> path could be NULL here due to failed allocation. Need to check that
> and return an error.
>
> > +     ret = rmdir(path);
> > +     tracefs_put_tracing_file(path);
> > +
> > +     return ret;
> > +}
> > +
> > +/**
> > + * tracefs_get_instance_file - return the path to an instance file.
> > + * @instance: ftrace instance, can be NULL for the top instance
> > + * @file: name of file to return
> > + *
> > + * Returns the path of the @file for the given @instance, or NULL in
> > + * case of an error.
> > + *
> > + * Must use tracefs_put_tracing_file() to free the returned string.
> > + */
> > +char *
> > +tracefs_get_instance_file(struct tracefs_instance *instance, const char *file)
> > +{
> > +     char *path;
> > +     char *buf;
> > +     int ret;
> > +
> > +     if (instance && instance->name) {
> > +             ret = asprintf(&buf, "instances/%s/%s", instance->name, file);
> > +             if (ret < 0) {
> > +                     warning("Failed to allocate name for %s/%s",
> > +                              instance->name, file);
>
> For this being in a library, we should probably remove warnings.
>
> > +                     return NULL;
> > +             }
> > +             path = tracefs_get_tracing_file(buf);
> > +             free(buf);
> > +     } else
> > +             path = tracefs_get_tracing_file(file);
> > +
> > +     return path;
> > +}
> > +
> > +/**
> > + * tracefs_get_instance_dir - return the path to the instance directory.
> > + * @instance: ftrace instance, can be NULL for the top instance
> > + *
> > + * Returns the full path to the instance directory
> > + *
> > + * Must use tracefs_put_tracing_file() to free the returned string.
> > + */
> > +char *tracefs_get_instance_dir(struct tracefs_instance *instance)
> > +{
> > +     char *buf;
> > +     char *path;
> > +     int ret;
> > +
> > +     if (instance && instance->name) {
> > +             ret = asprintf(&buf, "instances/%s", instance->name);
> > +             if (ret < 0) {
> > +                     warning("Failed to allocate path for instance %s",
> > +                              instance->name);
> > +                     return NULL;
> > +             }
> > +             path = tracefs_get_tracing_file(buf);
> > +             free(buf);
> > +     } else
> > +             path = tracefs_find_tracing_dir();
> > +
> > +     return path;
> > +}
> > +
> > +/**
> > + * tracefs_get_instance_name - return the name of an instance
> > + * @instance: ftrace instance
> > + *
> > + * Returns the name of the given @instance.
> > + * The returned string must *not* be freed.
> > + */
> > +char *tracefs_get_instance_name(struct tracefs_instance *instance)
> > +{
> > +     if (instance)
> > +             return instance->name;
> > +     return NULL;
> > +}
> > +
> > +static int write_file(const char *file, const char *str, const char *type)
> > +{
> > +     char buf[BUFSIZ];
> > +     int ret;
> > +     int fd;
> > +
> > +     fd = open(file, O_WRONLY | O_TRUNC);
> > +     if (fd < 0) {
> > +             warning("Failed to open '%s'", file);
> > +             return -1;
> > +     }
> > +     ret = write(fd, str, strlen(str));
> > +     close(fd);
> > +     if (ret < 0 && type) {
> > +             /* write failed */
> > +             fd = open(file, O_RDONLY);
> > +             if (fd < 0) {
> > +                     warning("Failed to write in '%s'", file);
> > +                     return -1;
> > +             }
> > +
> > +             while ((ret = read(fd, buf, BUFSIZ)) > 0)
> > +                     fprintf(stderr, "%.*s", ret, buf);
> > +             warning("Failed %s of %s\n", type, file);
>
> The library function should not bother reading the file on error. This
> is only applicable for some (one or two) files in the instance
> directory. Thus remove this code.
>
> > +             close(fd);
> > +     }
> > +     return ret;
> > +}
> > +
> > +
> > +/**
> > + * tracefs_write_instance_file - Write in trace file of specific instance.
> > + * @instance: ftrace instance, can be NULL for the top instance
> > + * @file: name of the file
> > + * @str: nul terminated string, that will be written in the file.
> > + * @type: nul terminated string, describing the current write operation.
> > + *     Used for logging purposes.
> > + *
> > + * Returns the number of written bytes, or -1 in case of an error
> > + */
> > +int tracefs_write_instance_file(struct tracefs_instance *instance,
> > +                              const char *file, const char *str,
> > +                              const char *type)
>
> Let's get rid of the "type", it's confusing for a library function. If
> the output should be read, that should be for a different operation.
>
>
> > +{
> > +     struct stat st;
> > +     char *path;
> > +     int ret;
> > +
> > +     path = tracefs_get_instance_file(instance, file);
>
> Need to check for path == NULL.
>
> > +     ret = stat(path, &st);
> > +     if (ret == 0)
> > +             ret = write_file(path, str, type);
> > +     tracefs_put_tracing_file(path);
> > +
> > +     return ret;
> > +}
> > +
> > +/**
> > + * tracefs_read_instance_file - Read from a trace file of specific instance.
> > + * @instance: ftrace instance, can be NULL for the top instance
> > + * @file: name of the file
> > + * @psize: returns the number of bytes read
> > + *
> > + * Returns a pointer to a nul terminated string, read from the file, or NULL in
> > + * case of an error.
> > + * The return string must be freed by free()
> > + */
> > +char *tracefs_read_instance_file(struct tracefs_instance *instance,
> > +                               char *file, int *psize)
> > +{
> > +     char *buf = NULL;
> > +     int size = 0;
> > +     char *path;
> > +
> > +     path = tracefs_get_instance_file(instance, file);
>
> Need to check for path == NULL.
>
> > +
> > +     size = str_read_file(path, &buf);
> > +
> > +     tracefs_put_tracing_file(path);
> > +     if (buf && psize)
> > +             *psize = size;
> > +
> > +     return buf;
> > +}
> > diff --git a/lib/tracefs/tracefs-utils.c b/lib/tracefs/tracefs-utils.c
> > index 0ef16fc..bdab130 100644
> > --- a/lib/tracefs/tracefs-utils.c
> > +++ b/lib/tracefs/tracefs-utils.c
> > @@ -10,6 +10,9 @@
> >  #include <sys/mount.h>
> >  #include <sys/stat.h>
> >  #include <linux/limits.h>
> > +#include <sys/stat.h>
> > +#include <fcntl.h>
> > +#include <unistd.h>
> >
> >  #include "tracefs.h"
> >
> > @@ -181,3 +184,43 @@ void tracefs_put_tracing_file(char *name)
> >  {
> >       free(name);
> >  }
> > +
> > +int str_read_file(const char *file, char **buffer)
> > +{
> > +     char stbuf[BUFSIZ];
> > +     char *buf = NULL;
> > +     int size = 0;
> > +     char *nbuf;
> > +     int fd;
> > +     int r;
> > +
> > +     fd = open(file, O_RDONLY);
> > +     if (fd < 0) {
> > +             warning("File %s not found", file);
> > +             return -1;
> > +     }
> > +
> > +     do {
> > +             r = read(fd, stbuf, BUFSIZ);
> > +             if (r <= 0)
> > +                     continue;
> > +             nbuf = realloc(buf, size+r+1);
> > +             if (!nbuf) {
> > +                     warning("Failed to allocate file buffer");
>
> Again, we should remove warnings from the library function. But this
> can be done later. I just wanted to document that we need to do that.
>
> > +                     size = -1;
> > +                     break;
> > +             }
> > +             buf = nbuf;
> > +             memcpy(buf+size, stbuf, r);
> > +             size += r;
> > +     } while (r);
>
> Hmm, this probably should be:
>
>         } while (r > 0);
>
> (I know this was broken before this patch)
>
> > +
> > +     close(fd);
> > +     if (size > 0) {
>
> Hmm, if r is less than zero here, we have a bug. Perhaps this needs to
> be:
>
>         if (r == 0 && size > 0) {
>
> > +             buf[size] = '\0';
> > +             *buffer = buf;
> > +     } else
> > +             free(buf);
> > +
> > +     return size;
> > +}
>
>
Thanks Steve! It makes sense, will include these changes in the next version.


> -- Steve



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