On Wed, 24 Mar 2021 17:38:37 +0200
Tzvetomir Stoyanov <tz.stoyanov@xxxxxxxxx> wrote:
> On Wed, Mar 24, 2021 at 5:20 PM Steven Rostedt <rostedt@xxxxxxxxxxx> 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 ;-)
-- Steve
|