Re: [PATCH v2 6/7] perf trace: Add beautifier script for fsmount flags

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The replacement of STRARRAY_FLAGS with a custom formatter initialization silently drops the `.strtoul` handler and `.show_zero` property for the `fsmount` flags argument.
- [Low] The newly generated `fsmount_attr_arrays.c` file is not removed during `make clean`.
- [Low] The commit message falsely claims that the source file `fsmount.c` was renamed.
--

commit 3df6eed17e58d1e1ddb3e75a7cd7fc70c8f3735f
Author: Namhyung Kim <namhyung@kernel.org>

perf trace: Add beautifier script for fsmount flags

This commit updates the fsmount beautifier to generate fsmount_flags[]
directly from mount.h. It also splits out the attribute flags generation into
a separate fsmount_attr.sh script.

> And rename the existing one to fsmount_attr.{sh,c} to be more precise.

This isn't a bug, but the commit message says fsmount.c was renamed to
fsmount_attr.c, while the patch only modifies fsmount.c without renaming it.

Should the commit message be updated to reflect this?

> diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
> --- a/tools/perf/Makefile.perf
> +++ b/tools/perf/Makefile.perf
> @@ -564,6 +564,12 @@ fsmount_tbls := $(srctree)/tools/perf/trace/beauty/fsmount.sh
>  $(fsmount_arrays): $(beauty_uapi_linux_dir)/mount.h $(fsmount_tbls)
>  	$(Q)$(SHELL) '$(fsmount_tbls)' $(beauty_uapi_linux_dir) > $@
>  
> +fsmount_attr_arrays := $(beauty_outdir)/fsmount_attr_arrays.c
> +fsmount_attr_tbls := $(srctree)/tools/perf/trace/beauty/fsmount_attr.sh
> +
> +$(fsmount_attr_arrays): $(beauty_uapi_linux_dir)/mount.h $(fsmount_attr_tbls)
> +	$(Q)$(SHELL) '$(fsmount_attr_tbls)' $(beauty_uapi_linux_dir) > $@

Does the clean target in Makefile.perf need to be updated to remove the new
fsmount_attr_arrays.c file? Without it, the generated file might be left
behind during a make clean operation.

> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
[ ... ]
> @@ -1202,7 +1197,7 @@ static const struct syscall_fmt syscall_fmts[] = {
>  	{ .name     = "fsconfig",
>  	  .arg = { [1] = STRARRAY(cmd, fsconfig_cmds), }, },
>  	{ .name     = "fsmount",
> -	  .arg = { [1] = STRARRAY_FLAGS(flags, fsmount_flags),
> +	  .arg = { [1] = { .scnprintf = SCA_FSMOUNT_FLAGS, /* fsmount_flags */ },
>  		   [2] = { .scnprintf = SCA_FSMOUNT_ATTR_FLAGS, /* attr_flags */ }, }, },

Does replacing STRARRAY_FLAGS drop the .strtoul parser and .show_zero
properties?

The original STRARRAY_FLAGS macro assigned .strtoul = STUL_STRARRAY_FLAGS and
.show_zero = true. By dropping these, does perf trace lose the ability to
parse symbolic names for the fsmount flags argument, such as during filter
expressions like -e 'fsmount(flags==CLOEXEC)'?

Additionally, without .show_zero = true, will a flags value of 0 be skipped in
the output, causing positional ambiguity with the remaining trailing
arguments?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260507072632.37152-1-namhyung@kernel.org?part=6