Re: [PATCH bpf-next v5 2/3] selftests/bpf: support array presets in veristat

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2025-06-25 at 17:59 +0100, Mykyta Yatsenko wrote:
> From: Mykyta Yatsenko <yatsenko@meta.com>
> 
> Implement support for presetting values for array elements in veristat.
> For example:
> ```
> sudo ./veristat set_global_vars.bpf.o -G "arr[3] = 1"
> ```
> Arrays of structures and structure of arrays work, but each individual
> scalar value has to be set separately: `foo[1].bar[2] = value`.
> 
> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> ---

This looks great, I have a few minor nits but let's land this patch-set.
Maybe fix error reporting below as a follow-up.
New array offset computation logic is much simpler to grasp.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

---

Nit: this signals an error:

        $ ./veristat -G "   arr[   fff  ]      = 1" set_global_vars.bpf.o
        Processing 'set_global_vars.bpf.o'...
        Can't resolve enum value fff
        Failed to set global variables -3
        Failed to process 'set_global_vars.bpf.o': -3
      
     but this does not:

        $ ./veristat -G "   arr[   11111111111111111111111111111  ]      = 1" set_global_vars.bpf.o
        Failed to parse value '11111111111111111111111111111'
        Processing 'set_global_vars.bpf.o'...
        File                   Program           Verdict  Duration (us)  Insns  States  Program size  Jited size
        ---------------------  ----------------  -------  -------------  -----  ------  ------------  ----------
        set_global_vars.bpf.o  test_set_globals  success             27     64       0            82           0
        ---------------------  ----------------  -------  -------------  -----  ------  ------------  ----------
        Done. Processed 1 files, 0 programs. Skipped 1 files, 0 programs.


>  tools/testing/selftests/bpf/veristat.c | 239 +++++++++++++++++++------
>  1 file changed, 189 insertions(+), 50 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index 483442c08ecf..9c67adcf0a33 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c

[...]

> @@ -1670,10 +1706,16 @@ static int append_var_preset(struct var_preset **presets, int *cnt, const char *
>  	memset(cur, 0, sizeof(*cur));
>  	(*cnt)++;
>  
> -	if (sscanf(expr, "%s = %s %n", var, val, &n) != 2 || n != strlen(expr)) {
> +	if (sscanf(expr, " %[][a-zA-Z0-9_. ] = %s %n", var, val, &n) != 2 || n != strlen(expr)) {
>  		fprintf(stderr, "Failed to parse expression '%s'\n", expr);
>  		return -EINVAL;
>  	}
> +	/* Remove trailing spaces from var, as scanf may add those */
> +	for (i = strlen(var) - 1; i > 0; --i) {
> +		if (!isspace(var[i]))
> +			break;
> +		var[i] = '\0';
> +	}

Nit: It appears, this sequence is needed only for nicer error reporting,
     I'd just drop it.

>  
>  	err = parse_rvalue(val, &cur->value);
>  	if (err)

[...]

> @@ -3164,11 +3296,18 @@ int main(int argc, char **argv)
>  	free(env.deny_filters);
>  	for (i = 0; i < env.npresets; ++i) {
>  		free(env.presets[i].full_name);
> -		for (j = 0; j < env.presets[i].atom_count; ++j)
> -			free(env.presets[i].atoms[j].name);
> +		for (j = 0; j < env.presets[i].atom_count; ++j) {
> +			switch (env.presets[i].atoms[j].type) {
> +			case FIELD_NAME:
> +				free(env.presets[i].atoms[j].name);
> +				break;
> +			case ARRAY_INDEX:
> +				if (env.presets[i].atoms[j].index.type == ENUMERATOR)
> +					free(env.presets[i].atoms[j].index.svalue);
> +				break;
> +			}
> +		}

Nit: removing union in `struct field_access` would simplify this loop to:

		for (j = 0; j < env.presets[i].atom_count; ++j) {
			free(env.presets[i].atoms[j].name);
			free(env.presets[i].atoms[j].index.svalue);
		}

     (assuming zero initialization).

>  		free(env.presets[i].atoms);
> -		if (env.presets[i].value.type == ENUMERATOR)
> -			free(env.presets[i].value.svalue);
>  	}
>  	free(env.presets);
>  	return -err;