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

[Namhyung Kim <namhyung@kernel.org>]

On Sun, Jun 30, 2024 at 05:07:10PM +0530, Aditya Gupta wrote:
> Hi Namhyung,
> 
> > > +static const char * const check_subcommands[] = { "feature", NULL };
> > > +static struct option check_options[] = {
> > > +	OPT_BOOLEAN('q', "quiet", &quiet, "do not show any warnings or messages"),
> > > +	OPT_END()
> > > +};
> > > +static struct option check_feature_options[] = { OPT_END() };
> > 
> > It should be OPT_PARENT(check_options) instead of OPT_END().
> 
> I did not know that. Thank you.
> 
> > 
> > > +
> > > +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.

Thanks,
Namhyung