On Tue, Mar 19, 2019 at 8:21 PM Matt Helsley <mhelsley@xxxxxxxxxx> wrote:
>
>
>
> > On Mar 19, 2019, at 8:55 AM, Tzvetomir Stoyanov <tstoyanov@xxxxxxxxxx> wrote:
> >
> > In order to reuse the code inside the trace-cmd application, few
> > functions from trace-record.c are refactored:
> > - make_instances() and tracecmd_remove_instances() are splited.
> > New ones are created: tracecmd_make_instance() and tracecmd_remove_instance(),
> > which are visible outside the trace-record.c
> > - Following functions are made non-static: tracecmd_init_instance()
> > get_instance_dir(), write_instance_file(), write_tracing_on(),
> > tracecmd_set_clock()
> > - New function is implemented: tracecmd_local_cpu_count(), an internal
> > API to get local_cpu_count.
> >
> > Signed-off-by: Tzvetomir Stoyanov <tstoyanov@xxxxxxxxxx>
> > ---
> > tracecmd/include/trace-local.h | 9 ++++
> > tracecmd/trace-record.c | 87 +++++++++++++++++++---------------
> > 2 files changed, 58 insertions(+), 38 deletions(-)
> >
> > diff --git a/tracecmd/include/trace-local.h b/tracecmd/include/trace-local.h
> > index d7bdb1f..8413054 100644
> > --- a/tracecmd/include/trace-local.h
> > +++ b/tracecmd/include/trace-local.h
> > @@ -235,6 +235,15 @@ void update_first_instance(struct buffer_instance *instance, int topt);
> > void show_instance_file(struct buffer_instance *instance, const char *name);
> >
> > int count_cpus(void);
> > +void write_tracing_on(struct buffer_instance *instance, int on);
> > +char *get_instance_dir(struct buffer_instance *instance);
> > +int write_instance_file(struct buffer_instance *instance,
> > + const char *file, const char *str, const char *type);
> > +void tracecmd_init_instance(struct buffer_instance *instance);
> > +void tracecmd_make_instance(struct buffer_instance *instance);
> > +int tracecmd_local_cpu_count(void);
> > +void tracecmd_set_clock(struct buffer_instance *instance);
> > +void tracecmd_remove_instance(struct buffer_instance *instance);
> >
> > /* No longer in event-utils.h */
> > void __noreturn die(const char *fmt, ...); /* Can be overriden */
> > diff --git a/tracecmd/trace-record.c b/tracecmd/trace-record.c
> > index 177060d..3950242 100644
> > --- a/tracecmd/trace-record.c
> > +++ b/tracecmd/trace-record.c
> >
>
> <snipped>
>
> > -static void set_clock(struct buffer_instance *instance)
> > +void tracecmd_set_clock(struct buffer_instance *instance)
> > {
> > char *path;
> > char *content;
> > @@ -4316,49 +4314,57 @@ static void clear_func_filters(void)
> > }
> > }
> >
> > -static void make_instances(void)
> > +void tracecmd_make_instance(struct buffer_instance *instance)
> > {
> > - struct buffer_instance *instance;
> > struct stat st;
> > char *path;
> > int ret;
> >
> > + path = get_instance_dir(instance);
> > + ret = stat(path, &st);
> > + if (ret < 0) {
> > + ret = mkdir(path, 0777);
>
> Here is the mkdir ...
>
> > + if (ret < 0)
> > + die("mkdir %s", path);
> > + } else
> > + /* Don't delete instances that already exist */
> > + instance->flags |= BUFFER_FL_KEEP;
> > + tracecmd_put_tracing_file(path);
> > +
> > +}
> > +
> > +static void make_instances(void)
> > +{
> > + struct buffer_instance *instance;
> > +
> > for_each_instance(instance) {
> > if (is_guest(instance))
> > continue;
> > + tracecmd_make_instance(instance);
> > + }
> > +}
> >
> > - path = get_instance_dir(instance);
> > - ret = stat(path, &st);
> > - if (ret < 0) {
> > - ret = mkdir(path, 0777);
> > - if (ret < 0)
> > - die("mkdir %s", path);
> > - } else
> > - /* Don't delete instances that already exist */
> > - instance->flags |= BUFFER_FL_KEEP;
> > - tracecmd_put_tracing_file(path);
> > +void tracecmd_remove_instance(struct buffer_instance *instance)
> > +{
> > + char *path;
> > +
> > + if (instance->tracing_on_fd > 0) {
> > + close(instance->tracing_on_fd);
> > + instance->tracing_on_fd = 0;
> > }
> > + path = get_instance_dir(instance);
> > + tracecmd_put_tracing_file(path);
> > }
> >
> > void tracecmd_remove_instances(void)
> > {
> > struct buffer_instance *instance;
> > - char *path;
> > - int ret;
> >
> > for_each_instance(instance) {
> > /* Only delete what we created */
> > if (is_guest(instance) || (instance->flags & BUFFER_FL_KEEP))
> > continue;
> > - if (instance->tracing_on_fd > 0) {
> > - close(instance->tracing_on_fd);
> > - instance->tracing_on_fd = 0;
> > - }
> > - path = get_instance_dir(instance);
> > - ret = rmdir(path);
>
> But this rmdir is not moved to tracecmd_remove_instance() — it’s lost. This looks like it’s introducing a bug.
Thanks Matt, this one is from v1 of the patchset, I didn't notice it.
I'll add the rmdir() call in the next version.
There is some general problem with deleting instances, or (most
probably) I cannot figure out what it the proper way
to delete a ftrace instance - even after the call to rmdir() in
tracecmd_remove_instance(), the instance dir is still there.
I tried to run "rm -rf path_to_instance_dir" from the shell (at root),
after the trace-cmd is executed, got a lot of
"Operation not permitted" errors and the instance dir is not deleted.
>
> > - if (ret < 0)
> > - die("rmdir %s", path);
> > - tracecmd_put_tracing_file(path);
> > + tracecmd_remove_instance(instance);
> > }
> > }
>
> <snipped>
>
> The rest looks fine.
>
> Cheers,
> -Matt Helsley
>
--
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center
|