On Wed, 2025-06-25 at 17:59 +0100, Mykyta Yatsenko wrote:
> From: Mykyta Yatsenko <yatsenko@xxxxxxxx>
>
> 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@xxxxxxxx>
> ---
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@xxxxxxxxx>
---
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;
