[PATCH] tools lib subcmd: Use array data to save built usage string

[PATCH] tools lib subcmd: Use array data to save built usage string

[Yang Jihong]

commit 230a7a71f922 ("libsubcmd: Fix parse-options memory leak")
free built usage string to solve the memory leak problem,
which causes perf tool to not print subcommands when outputting usage help,
reducing the friendliness of the information.

If use original method of dynamically allocating memory, caller needs to
free the memory in appropriate place. In order to avoid it, use an array
to save built usage string.
Currently, only perf tool uses this function, 128 bytes are enough.

Before:
  # 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)

After:
  # 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)

Fixes: 230a7a71f922 ("libsubcmd: Fix parse-options memory leak")
Signed-off-by: Yang Jihong <yangjihong@bytedance.com>
---
 tools/lib/subcmd/parse-options.c | 27 +++++++++++++++------------
 tools/lib/subcmd/parse-options.h | 10 ++++++++++
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/tools/lib/subcmd/parse-options.c b/tools/lib/subcmd/parse-options.c
index 4b60ec03b0bb..1e41a204864a 100644
--- a/tools/lib/subcmd/parse-options.c
+++ b/tools/lib/subcmd/parse-options.c
@@ -633,20 +633,26 @@ 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]) {
-		astrcatf(&buf, "%s %s [<options>] {", subcmd_config.exec_name, argv[0]);
+		int n;
+		static char buf[USAGESTR_BUF_SIZE];
+		int buf_size = sizeof(buf);
 
-		for (int i = 0; subcommands[i]; i++) {
-			if (i)
-				astrcat(&buf, "|");
-			astrcat(&buf, subcommands[i]);
+		n = scnprintf(buf, buf_size, "%s %s [<options>] {",
+			      subcmd_config.exec_name, argv[0]);
+
+		for (int i = 0; subcommands[i] && n < buf_size - 1; i++) {
+			n += scnprintf(buf + n, buf_size - n, "%s%s",
+				       i ? "|" : "", subcommands[i]);
 		}
-		astrcat(&buf, "}");
+		if (n < buf_size - 1)
+			n += scnprintf(buf + n, buf_size - n, "}");
 
-		usagestr[0] = buf;
+		/* only provided if a complete string is built */
+		if (n < buf_size - 1)
+			usagestr[0] = buf;
 	}
 
 	parse_options_start(&ctx, argc, argv, flags);
@@ -678,10 +684,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/lib/subcmd/parse-options.h b/tools/lib/subcmd/parse-options.h
index 8e9147358a28..654ba0a2201c 100644
--- a/tools/lib/subcmd/parse-options.h
+++ b/tools/lib/subcmd/parse-options.h
@@ -174,6 +174,16 @@ extern int parse_options(int argc, const char **argv,
                          const struct option *options,
                          const char * const usagestr[], int flags);
 
+/* parse_options_subcommand() will filter out the processed options
+ * and subcommands, leave the non-option argments in argv[].
+ * If usagestr is empty, it will build usage string based on subcommands
+ * in format of "exec_name argv[0] [<options>] {subcommand1|subcomman2|...}".
+ *
+ * NOTE: In order to avoid the caller needing to free memory,
+ * use an 128-bytes array to store the built usage string.
+ * If need to expand the array size, please modify USAGESTR_BUF_SIZE macro.
+ */
+#define USAGESTR_BUF_SIZE 128
 extern int parse_options_subcommand(int argc, const char **argv,
 				const struct option *options,
 				const char *const subcommands[],
-- 
2.25.1

Re: [PATCH] tools lib subcmd: Use array data to save built usage string

[kernel test robot]

Hi Yang,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.11-rc6 next-20240903]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yang-Jihong/tools-lib-subcmd-Use-array-data-to-save-built-usage-string/20240903-164426
base:   linus/master
patch link:    https://lore.kernel.org/r/20240903084048.766892-1-yangjihong%40bytedance.com
patch subject: [PATCH] tools lib subcmd: Use array data to save built usage string
config: x86_64-randconfig-101-20240903 (https://download.01.org/0day-ci/archive/20240904/202409040112.BKj8VlQT-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240904/202409040112.BKj8VlQT-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409040112.BKj8VlQT-lkp@intel.com/

All errors (new ones prefixed by >>):

   scripts/genksyms/parse.y: warning: 9 shift/reduce conflicts [-Wconflicts-sr]
   scripts/genksyms/parse.y: warning: 5 reduce/reduce conflicts [-Wconflicts-rr]
   scripts/genksyms/parse.y: note: rerun with option '-Wcounterexamples' to generate conflict counterexamples
   /usr/bin/ld: tools/objtool/libsubcmd/libsubcmd.a(libsubcmd-in.o): in function `parse_options_subcommand':
>> tools/lib/subcmd/parse-options.c:643: undefined reference to `scnprintf'
>> /usr/bin/ld: tools/lib/subcmd/parse-options.c:647: undefined reference to `scnprintf'
>> /usr/bin/ld: tools/lib/subcmd/parse-options.c:647: undefined reference to `scnprintf'
   /usr/bin/ld: tools/lib/subcmd/parse-options.c:651: undefined reference to `scnprintf'
   clang: error: linker command failed with exit code 1 (use -v to see invocation)
   make[4]: *** [Makefile:75: tools/objtool/objtool] Error 1
   make[3]: *** [Makefile:72: objtool] Error 2
   make[2]: *** [Makefile:1360: tools/objtool] Error 2
   make[2]: Target 'prepare' not remade because of errors.
   make[1]: *** [Makefile:224: __sub-make] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:224: __sub-make] Error 2
   make: Target 'prepare' not remade because of errors.


vim +643 tools/lib/subcmd/parse-options.c

   631	
   632	int parse_options_subcommand(int argc, const char **argv, const struct option *options,
   633				const char *const subcommands[], const char *usagestr[], int flags)
   634	{
   635		struct parse_opt_ctx_t ctx;
   636	
   637		/* build usage string if it's not provided */
   638		if (subcommands && !usagestr[0]) {
   639			int n;
   640			static char buf[USAGESTR_BUF_SIZE];
   641			int buf_size = sizeof(buf);
   642	
 > 643			n = scnprintf(buf, buf_size, "%s %s [<options>] {",
   644				      subcmd_config.exec_name, argv[0]);
   645	
   646			for (int i = 0; subcommands[i] && n < buf_size - 1; i++) {
 > 647				n += scnprintf(buf + n, buf_size - n, "%s%s",
   648					       i ? "|" : "", subcommands[i]);
   649			}
   650			if (n < buf_size - 1)
   651				n += scnprintf(buf + n, buf_size - n, "}");
   652	
   653			/* only provided if a complete string is built */
   654			if (n < buf_size - 1)
   655				usagestr[0] = buf;
   656		}
   657	
   658		parse_options_start(&ctx, argc, argv, flags);
   659		switch (parse_options_step(&ctx, options, usagestr)) {
   660		case PARSE_OPT_HELP:
   661			exit(129);
   662		case PARSE_OPT_DONE:
   663			break;
   664		case PARSE_OPT_LIST_OPTS:
   665			while (options->type != OPTION_END) {
   666				if (options->long_name)
   667					printf("--%s ", options->long_name);
   668				options++;
   669			}
   670			putchar('\n');
   671			exit(130);
   672		case PARSE_OPT_LIST_SUBCMDS:
   673			if (subcommands) {
   674				for (int i = 0; subcommands[i]; i++)
   675					printf("%s ", subcommands[i]);
   676			}
   677			putchar('\n');
   678			exit(130);
   679		default: /* PARSE_OPT_UNKNOWN */
   680			if (ctx.argv[0][1] == '-')
   681				astrcatf(&error_buf, "unknown option `%s'",
   682					 ctx.argv[0] + 2);
   683			else
   684				astrcatf(&error_buf, "unknown switch `%c'", *ctx.opt);
   685			usage_with_options(usagestr, options);
   686		}
   687	
   688		return parse_options_end(&ctx);
   689	}
   690	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki