* [RFC PATCH] tools/lib/subcmd: Don't free the usage string
@ 2024-07-17 10:33 Aditya Gupta
2024-07-18 3:12 ` Namhyung Kim
0 siblings, 1 reply; 3+ messages in thread
From: Aditya Gupta @ 2024-07-17 10:33 UTC (permalink / raw)
To: linux-perf-users
Cc: Arnaldo Carvalho de Melo, Jiri Olsa, Ian Rogers, Namhyung Kim
Currently, commands which depend on 'parse_options_subcommand()' don't
show the usage string, and instead show '(null)'
$ ./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)
'parse_options_subcommand()' is generally expected to initialise the usage
string, with information in the passed 'subcommands[]' array
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.
As discussed in [1], free the allocated usage string in the main function
itself, and don't reset usage string to NULL in parse_options_subcommand
With this change, the behaviour is restored.
$ ./perf sched
Usage: perf sched [<options>] {record|latency|map|replay|script|timehist}
-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)
[1]: https://lore.kernel.org/linux-perf-users/htq5vhx6piet4nuq2mmhk7fs2bhfykv52dbppwxmo3s7du2odf@styd27tioc6e/
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Fixes: 230a7a71f922 ("libsubcmd: Fix parse-options memory leak")
Suggested-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
---
tools/lib/subcmd/parse-options.c | 8 +++-----
tools/perf/builtin-kmem.c | 2 ++
tools/perf/builtin-kvm.c | 3 +++
tools/perf/builtin-kwork.c | 3 +++
tools/perf/builtin-lock.c | 3 +++
tools/perf/builtin-mem.c | 3 +++
tools/perf/builtin-sched.c | 3 +++
7 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/tools/lib/subcmd/parse-options.c b/tools/lib/subcmd/parse-options.c
index 4b60ec03b0bb..eb896d30545b 100644
--- a/tools/lib/subcmd/parse-options.c
+++ b/tools/lib/subcmd/parse-options.c
@@ -633,10 +633,11 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
const char *const subcommands[], const char *usagestr[], int flags)
{
struct parse_opt_ctx_t ctx;
- char *buf = NULL;
/* build usage string if it's not provided */
if (subcommands && !usagestr[0]) {
+ char *buf = NULL;
+
astrcatf(&buf, "%s %s [<options>] {", subcmd_config.exec_name, argv[0]);
for (int i = 0; subcommands[i]; i++) {
@@ -678,10 +679,7 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
astrcatf(&error_buf, "unknown switch `%c'", *ctx.opt);
usage_with_options(usagestr, options);
}
- if (buf) {
- usagestr[0] = NULL;
- free(buf);
- }
+
return parse_options_end(&ctx);
}
diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
index 6fd95be5032b..6b88b8b40784 100644
--- a/tools/perf/builtin-kmem.c
+++ b/tools/perf/builtin-kmem.c
@@ -2058,6 +2058,8 @@ int cmd_kmem(int argc, const char **argv)
out_delete:
perf_session__delete(session);
+ /* free usage string allocated by parse_options_subcommand */
+ free((void *)kmem_usage[0]);
return ret;
}
diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 71165036e4ca..988bef73bd09 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -2187,5 +2187,8 @@ int cmd_kvm(int argc, const char **argv)
else
usage_with_options(kvm_usage, kvm_options);
+ /* free usage string allocated by parse_options_subcommand */
+ free((void *)kvm_usage[0]);
+
return 0;
}
diff --git a/tools/perf/builtin-kwork.c b/tools/perf/builtin-kwork.c
index 56e3f3a5e03a..fd53838b5a78 100644
--- a/tools/perf/builtin-kwork.c
+++ b/tools/perf/builtin-kwork.c
@@ -2520,5 +2520,8 @@ int cmd_kwork(int argc, const char **argv)
} else
usage_with_options(kwork_usage, kwork_options);
+ /* free usage string allocated by parse_options_subcommand */
+ free((void *)kwork_usage[0]);
+
return 0;
}
diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 0253184b3b58..b25d50716e63 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -2713,6 +2713,9 @@ int cmd_lock(int argc, const char **argv)
usage_with_options(lock_usage, lock_options);
}
+ /* free usage string allocated by parse_options_subcommand */
+ free((void *)lock_usage[0]);
+
zfree(&lockhash_table);
return rc;
}
diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
index 863fcd735dae..b7c1cf6d0e5a 100644
--- a/tools/perf/builtin-mem.c
+++ b/tools/perf/builtin-mem.c
@@ -517,5 +517,8 @@ int cmd_mem(int argc, const char **argv)
else
usage_with_options(mem_usage, mem_options);
+ /* free usage string allocated by parse_options_subcommand */
+ free((void *)mem_usage[0]);
+
return 0;
}
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 8750b5f2d49b..d6acc53ae89e 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -3805,5 +3805,8 @@ int cmd_sched(int argc, const char **argv)
usage_with_options(sched_usage, sched_options);
}
+ /* free usage string allocated by parse_options_subcommand */
+ free((void *)sched_usage[0]);
+
return 0;
}
--
2.45.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC PATCH] tools/lib/subcmd: Don't free the usage string
2024-07-17 10:33 [RFC PATCH] tools/lib/subcmd: Don't free the usage string Aditya Gupta
@ 2024-07-18 3:12 ` Namhyung Kim
2024-07-18 5:19 ` Aditya Gupta
0 siblings, 1 reply; 3+ messages in thread
From: Namhyung Kim @ 2024-07-18 3:12 UTC (permalink / raw)
To: Aditya Gupta
Cc: linux-perf-users, Arnaldo Carvalho de Melo, Jiri Olsa, Ian Rogers
On Wed, Jul 17, 2024 at 04:03:51PM +0530, Aditya Gupta wrote:
> Currently, commands which depend on 'parse_options_subcommand()' don't
> show the usage string, and instead show '(null)'
>
> $ ./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)
>
> 'parse_options_subcommand()' is generally expected to initialise the usage
> string, with information in the passed 'subcommands[]' array
>
> 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.
>
> As discussed in [1], free the allocated usage string in the main function
> itself, and don't reset usage string to NULL in parse_options_subcommand
>
> With this change, the behaviour is restored.
>
> $ ./perf sched
> Usage: perf sched [<options>] {record|latency|map|replay|script|timehist}
>
> -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)
>
> [1]: https://lore.kernel.org/linux-perf-users/htq5vhx6piet4nuq2mmhk7fs2bhfykv52dbppwxmo3s7du2odf@styd27tioc6e/
>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Ian Rogers <irogers@google.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Fixes: 230a7a71f922 ("libsubcmd: Fix parse-options memory leak")
> Suggested-by: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Aditya Gupta <adityag@linux.ibm.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks,
Namhyung
> ---
> tools/lib/subcmd/parse-options.c | 8 +++-----
> tools/perf/builtin-kmem.c | 2 ++
> tools/perf/builtin-kvm.c | 3 +++
> tools/perf/builtin-kwork.c | 3 +++
> tools/perf/builtin-lock.c | 3 +++
> tools/perf/builtin-mem.c | 3 +++
> tools/perf/builtin-sched.c | 3 +++
> 7 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/tools/lib/subcmd/parse-options.c b/tools/lib/subcmd/parse-options.c
> index 4b60ec03b0bb..eb896d30545b 100644
> --- a/tools/lib/subcmd/parse-options.c
> +++ b/tools/lib/subcmd/parse-options.c
> @@ -633,10 +633,11 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
> const char *const subcommands[], const char *usagestr[], int flags)
> {
> struct parse_opt_ctx_t ctx;
> - char *buf = NULL;
>
> /* build usage string if it's not provided */
> if (subcommands && !usagestr[0]) {
> + char *buf = NULL;
> +
> astrcatf(&buf, "%s %s [<options>] {", subcmd_config.exec_name, argv[0]);
>
> for (int i = 0; subcommands[i]; i++) {
> @@ -678,10 +679,7 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
> astrcatf(&error_buf, "unknown switch `%c'", *ctx.opt);
> usage_with_options(usagestr, options);
> }
> - if (buf) {
> - usagestr[0] = NULL;
> - free(buf);
> - }
> +
> return parse_options_end(&ctx);
> }
>
> diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
> index 6fd95be5032b..6b88b8b40784 100644
> --- a/tools/perf/builtin-kmem.c
> +++ b/tools/perf/builtin-kmem.c
> @@ -2058,6 +2058,8 @@ int cmd_kmem(int argc, const char **argv)
>
> out_delete:
> perf_session__delete(session);
> + /* free usage string allocated by parse_options_subcommand */
> + free((void *)kmem_usage[0]);
>
> return ret;
> }
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index 71165036e4ca..988bef73bd09 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -2187,5 +2187,8 @@ int cmd_kvm(int argc, const char **argv)
> else
> usage_with_options(kvm_usage, kvm_options);
>
> + /* free usage string allocated by parse_options_subcommand */
> + free((void *)kvm_usage[0]);
> +
> return 0;
> }
> diff --git a/tools/perf/builtin-kwork.c b/tools/perf/builtin-kwork.c
> index 56e3f3a5e03a..fd53838b5a78 100644
> --- a/tools/perf/builtin-kwork.c
> +++ b/tools/perf/builtin-kwork.c
> @@ -2520,5 +2520,8 @@ int cmd_kwork(int argc, const char **argv)
> } else
> usage_with_options(kwork_usage, kwork_options);
>
> + /* free usage string allocated by parse_options_subcommand */
> + free((void *)kwork_usage[0]);
> +
> return 0;
> }
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index 0253184b3b58..b25d50716e63 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -2713,6 +2713,9 @@ int cmd_lock(int argc, const char **argv)
> usage_with_options(lock_usage, lock_options);
> }
>
> + /* free usage string allocated by parse_options_subcommand */
> + free((void *)lock_usage[0]);
> +
> zfree(&lockhash_table);
> return rc;
> }
> diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
> index 863fcd735dae..b7c1cf6d0e5a 100644
> --- a/tools/perf/builtin-mem.c
> +++ b/tools/perf/builtin-mem.c
> @@ -517,5 +517,8 @@ int cmd_mem(int argc, const char **argv)
> else
> usage_with_options(mem_usage, mem_options);
>
> + /* free usage string allocated by parse_options_subcommand */
> + free((void *)mem_usage[0]);
> +
> return 0;
> }
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index 8750b5f2d49b..d6acc53ae89e 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -3805,5 +3805,8 @@ int cmd_sched(int argc, const char **argv)
> usage_with_options(sched_usage, sched_options);
> }
>
> + /* free usage string allocated by parse_options_subcommand */
> + free((void *)sched_usage[0]);
> +
> return 0;
> }
> --
> 2.45.2
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC PATCH] tools/lib/subcmd: Don't free the usage string
2024-07-18 3:12 ` Namhyung Kim
@ 2024-07-18 5:19 ` Aditya Gupta
0 siblings, 0 replies; 3+ messages in thread
From: Aditya Gupta @ 2024-07-18 5:19 UTC (permalink / raw)
To: Namhyung Kim
Cc: linux-perf-users, Arnaldo Carvalho de Melo, Jiri Olsa, Ian Rogers
> <...snip...>
> Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks for the Ack, Namhyung !
- Aditya Gupta
> Thanks,
> Namhyung
>
>> ---
>> tools/lib/subcmd/parse-options.c | 8 +++-----
>> tools/perf/builtin-kmem.c | 2 ++
>> tools/perf/builtin-kvm.c | 3 +++
>> tools/perf/builtin-kwork.c | 3 +++
>> tools/perf/builtin-lock.c | 3 +++
>> tools/perf/builtin-mem.c | 3 +++
>> tools/perf/builtin-sched.c | 3 +++
>> 7 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/lib/subcmd/parse-options.c b/tools/lib/subcmd/parse-options.c
>> index 4b60ec03b0bb..eb896d30545b 100644
>> --- a/tools/lib/subcmd/parse-options.c
>> +++ b/tools/lib/subcmd/parse-options.c
>> @@ -633,10 +633,11 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
>> const char *const subcommands[], const char *usagestr[], int flags)
>> {
>> struct parse_opt_ctx_t ctx;
>> - char *buf = NULL;
>>
>> /* build usage string if it's not provided */
>> if (subcommands && !usagestr[0]) {
>> + char *buf = NULL;
>> +
>> astrcatf(&buf, "%s %s [<options>] {", subcmd_config.exec_name, argv[0]);
>>
>> for (int i = 0; subcommands[i]; i++) {
>> @@ -678,10 +679,7 @@ int parse_options_subcommand(int argc, const char **argv, const struct option *o
>> astrcatf(&error_buf, "unknown switch `%c'", *ctx.opt);
>> usage_with_options(usagestr, options);
>> }
>> - if (buf) {
>> - usagestr[0] = NULL;
>> - free(buf);
>> - }
>> +
>> return parse_options_end(&ctx);
>> }
>>
>> diff --git a/tools/perf/builtin-kmem.c b/tools/perf/builtin-kmem.c
>> index 6fd95be5032b..6b88b8b40784 100644
>> --- a/tools/perf/builtin-kmem.c
>> +++ b/tools/perf/builtin-kmem.c
>> @@ -2058,6 +2058,8 @@ int cmd_kmem(int argc, const char **argv)
>>
>> out_delete:
>> perf_session__delete(session);
>> + /* free usage string allocated by parse_options_subcommand */
>> + free((void *)kmem_usage[0]);
>>
>> return ret;
>> }
>> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
>> index 71165036e4ca..988bef73bd09 100644
>> --- a/tools/perf/builtin-kvm.c
>> +++ b/tools/perf/builtin-kvm.c
>> @@ -2187,5 +2187,8 @@ int cmd_kvm(int argc, const char **argv)
>> else
>> usage_with_options(kvm_usage, kvm_options);
>>
>> + /* free usage string allocated by parse_options_subcommand */
>> + free((void *)kvm_usage[0]);
>> +
>> return 0;
>> }
>> diff --git a/tools/perf/builtin-kwork.c b/tools/perf/builtin-kwork.c
>> index 56e3f3a5e03a..fd53838b5a78 100644
>> --- a/tools/perf/builtin-kwork.c
>> +++ b/tools/perf/builtin-kwork.c
>> @@ -2520,5 +2520,8 @@ int cmd_kwork(int argc, const char **argv)
>> } else
>> usage_with_options(kwork_usage, kwork_options);
>>
>> + /* free usage string allocated by parse_options_subcommand */
>> + free((void *)kwork_usage[0]);
>> +
>> return 0;
>> }
>> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
>> index 0253184b3b58..b25d50716e63 100644
>> --- a/tools/perf/builtin-lock.c
>> +++ b/tools/perf/builtin-lock.c
>> @@ -2713,6 +2713,9 @@ int cmd_lock(int argc, const char **argv)
>> usage_with_options(lock_usage, lock_options);
>> }
>>
>> + /* free usage string allocated by parse_options_subcommand */
>> + free((void *)lock_usage[0]);
>> +
>> zfree(&lockhash_table);
>> return rc;
>> }
>> diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
>> index 863fcd735dae..b7c1cf6d0e5a 100644
>> --- a/tools/perf/builtin-mem.c
>> +++ b/tools/perf/builtin-mem.c
>> @@ -517,5 +517,8 @@ int cmd_mem(int argc, const char **argv)
>> else
>> usage_with_options(mem_usage, mem_options);
>>
>> + /* free usage string allocated by parse_options_subcommand */
>> + free((void *)mem_usage[0]);
>> +
>> return 0;
>> }
>> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
>> index 8750b5f2d49b..d6acc53ae89e 100644
>> --- a/tools/perf/builtin-sched.c
>> +++ b/tools/perf/builtin-sched.c
>> @@ -3805,5 +3805,8 @@ int cmd_sched(int argc, const char **argv)
>> usage_with_options(sched_usage, sched_options);
>> }
>>
>> + /* free usage string allocated by parse_options_subcommand */
>> + free((void *)sched_usage[0]);
>> +
>> return 0;
>> }
>> --
>> 2.45.2
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-07-18 5:19 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-17 10:33 [RFC PATCH] tools/lib/subcmd: Don't free the usage string Aditya Gupta
2024-07-18 3:12 ` Namhyung Kim
2024-07-18 5:19 ` Aditya Gupta
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).