Re: [PATCH v3 06/23] trace-cmd: Add new trace-cmd clock tsc2nsec

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

On Wed, Mar 24, 2021 at 6:22 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 24 Mar 2021 17:38:37 +0200
> Tzvetomir Stoyanov <tz.stoyanov@gmail.com> wrote:
>
> > On Wed, Mar 24, 2021 at 5:20 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> > >> > > @@ -5904,7 +5965,15 @@ static void parse_record_options(int argc,
> > > >                       break;
> > > >               case 'C':
> > > >                       check_instance_die(ctx->instance, "-C");
> > > > -                     ctx->instance->clock = optarg;
> > > > +                     if (!strncmp(optarg, TSCNSEC_CLOCK, strlen(TSCNSEC_CLOCK))) {
> > >
> > > Hmm, why the strncmp()? Shouldn't it be a full match, not a partial one?
> > >
> > > > +                             ret = get_tsc_nsec(&ctx->tsc2nsec.shift,
> > > > +                                                &ctx->tsc2nsec.mult);
> > > > +                             if (ret || !clock_is_supported(NULL, TSC_CLOCK))
> > > > +                                     die("Clock %s is not supported", optarg);
> > > > +                             ctx->instance->flags |= BUFFER_FL_TSC2NSEC;
> > > > +                             ctx->instance->clock = strdup(TSC_CLOCK);
> > >
> > > Hmm, why the strdup? below we have clock = optarg, one of them is wrong. If
> > > we free instance->clock, then we can't have it set to optarg. As that was
> > > the way it was before, it looks to be a separate bug that probably needs
> > > its own patch.
> >
> > Actually, instance->clock is never freed - because there is no
> > function to free an instance. There is allocate_instance(), but no API
>
> Bah, I just noticed that I was confusing this with tracefs_instance :-p
>
>
> > to free. That's why both are valid. I was wondering if to use strdup
> > or simple assignment, and decided to allocate a memory. One day we may
> > implement free_instance(), there are a lot of resources in an instance
> > that should be freed.
>
> Perhaps that "one day" should be this week ;-)

That means to implement a cleanup after each trace-cmd subcommand ?
Currently, trace-cmd just exits and not freeing that memory is not a
big problem. I was thinking many times to implement free_instance(),
but there is no sense for it if there is no cleanup logic at the end
of each command.

>
> -- Steve



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