Re: [PATCH v12 1/4] perf check: introduce check subcommand

[Namhyung Kim <namhyung@kernel.org>]

Hello,

On Wed, Jul 03, 2024 at 04:17:09PM +0530, Aditya Gupta wrote:
> Hi Namhyung,
> 
> > > <...snip...>
> > >
> > > > > +static const char *check_usage[] = {
> > > > > +	"perf check [<subcommand>] [<options>]",
> > > > 
> > > > You can leave this NULL and parse_options_subcommand() will fill the
> > > > first element automatically using check_subcommands[].
> > > > 
> > > > Please see other commands like 'perf sched' how to handle this.
> > > 
> > > It doesn't seem to be working, hence added that check_usage in v12
> > > itself.
> > > 
> > >     $ ./perf check
> > > 	Usage: (null)
> > > 
> > >     -q, --quiet           do not show any warnings or messages
> > > 
> > >     $ ./perf sched
> > > 	Usage: (null)
> > > 
> > >     -D, --dump-raw-trace  dump raw trace in ASCII
> > >     -f, --force           don't complain, do it
> > >     -i, --input <file>    input file name
> > >     -v, --verbose         be more verbose (show symbol address, etc)
> > > 
> > > Debugging it further, this behaviour was changed in
> > > 
> > >     commit 230a7a71f9221: libsubcmd: Fix parse-options memory leak
> > > 
> > > Where the generated usage string is deallocated, and usage[0] string is
> > > reassigned as NULL.
> > 
> > Ok, thanks for the investigation.  It's a bug then.
> > 
> > > 
> > > If expected behaviour was allocation of the usage string, it should be
> > > okay for the buffer to not get deallocated for the entirety of the perf
> > > process's lifetime right ?
> > 
> > Right, it should not deallocate it in the parse_options_subcommand().
> > I think we need to change the exit path of the commands to free the
> > usage[0] manually.
> 
> Sure, I can implement that if you have things in hand, or I am fine with
> you implementing it since you will already have an idea what to do.
> 
> Maybe `free(command_usage[0])` or something like
> `parse_options_free_subcommand()` ?

It seems we don't have a place to mark whether it needs to free or not.
Let's go with free(command_usage[0]) for now.

Thanks,
Namhyung