Re: [PATCH 1/2] perf c2c: Fix error masking, OOM, and unchecked caller errors in hpp_list__parse()

Thank you for your contribution! Sashiko AI review found 1 potential issue(=
s) to consider:
- [High] The `__hpp_list__parse()` function fails to break out of the strin=
g tokenization loop upon encountering an error, silently overwriting error =
codes and returning false success.
--

--- Patch [1]: [PATCH 1/2] perf c2c: Fix error masking, OOM, and unchecked =
caller errors in hpp_list__parse() ---
commit 9193c764390834e98068c032e1076f1f5f739d5a
Author: Arnaldo Carvalho de Melo <[email protected]>

perf c2c: Fix error masking, OOM, and unchecked caller errors in hpp_list__=
parse()

This commit refactors hpp_list__parse() to correctly propagate errors from
tokenization and OOM situations. It replaces the PARSE_LIST macro with a
helper function to preserve and return the first encountered error.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index c9584dbedf77a..0f2fb13e705e3 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
[ ... ]
> @@ -2063,26 +2065,34 @@ static int c2c_hists__init_sort(struct perf_hpp_l=
ist *hpp_list, char *name, stru
[ ... ]
> +static int __hpp_list__parse(struct perf_hpp_list *hpp_list, char *_list=
, struct perf_env *env,
> +			     int (*_fn)(struct perf_hpp_list *hpp_list, char *name, struct pe=
rf_env *env))
> +{
> +	char *tmp, *tok;
> +	int ret =3D 0;
> +
> +	if (!_list)
> +		return 0;
> +
> +	for (tok =3D strtok_r(_list, ", ", &tmp); tok; tok =3D strtok_r(NULL, "=
, ", &tmp)) {
> +		ret =3D _fn(hpp_list, tok, env);
> +		switch (ret) {
> +		case 0:
> +			continue;
> +		case -EINVAL:
> +			pr_err("Invalid --fields key: `%s'", tok);
> +			break;
> +		case -ESRCH:
> +			pr_err("Unknown --fields key: `%s'", tok);
> +			break;
> +		default:
> +			pr_err("%m for --fields key: `%s'", tok);
> +			break;
> +		}

[Severity: High]
Does this actually break out of the strtok loop on error? In C, a break
statement inside a switch block only exits the switch, not the enclosing
for loop.

If an invalid field is followed by a valid one (e.g.,
--fields invalid_field,valid_field), won't the loop continue, causing _fn()
to return 0 for the valid field and silently overwrite the error code in re=
t?

> +	}
> +
> +	return ret;
> +}

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803120452.1812=
[email protected]?part=3D1