On Tue, Jun 3, 2025 at 7:15 AM Mykyta Yatsenko
<mykyta.yatsenko5@xxxxxxxxx> 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 "struct1[2].struct2[1].u.var_u8[2] = 3" -G "arr[3] = 9"
> ```
> 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>
> ---
> .../selftests/bpf/prog_tests/test_veristat.c | 9 +--
> .../selftests/bpf/progs/set_global_vars.c | 12 ++--
> tools/testing/selftests/bpf/veristat.c | 63 +++++++++++++++++--
> 3 files changed, 70 insertions(+), 14 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_veristat.c b/tools/testing/selftests/bpf/prog_tests/test_veristat.c
> index 47b56c258f3f..1af5d02bb2d0 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_veristat.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_veristat.c
> @@ -60,12 +60,13 @@ static void test_set_global_vars_succeeds(void)
> " -G \"var_s8 = -128\" "\
> " -G \"var_u8 = 255\" "\
> " -G \"var_ea = EA2\" "\
> - " -G \"var_eb = EB2\" "\
> - " -G \"var_ec = EC2\" "\
> + " -G \"var_eb = EB2\" "\
> + " -G \"var_ec=EC2\" "\
What was the problem previously with not handling this case?
> " -G \"var_b = 1\" "\
> - " -G \"struct1.struct2.u.var_u8 = 170\" "\
> + " -G \"struct1[2].struct2[1].u.var_u8[2]=170\" "\
> " -G \"union1.struct3.var_u8_l = 0xaa\" "\
> " -G \"union1.struct3.var_u8_h = 0xaa\" "\
> + " -G \"arr[2] = 0xaa\" " \
> "-vl2 > %s", fix->veristat, fix->tmpfile);
>
> read(fix->fd, fix->output, fix->sz);
[...]
> @@ -81,8 +80,9 @@ int test_set_globals(void *ctx)
> a = var_eb;
> a = var_ec;
> a = var_b;
> - a = struct1.struct2.u.var_u8;
> + a = struct1[2].struct2[1].u.var_u8[2];
> a = union1.var_u16;
> + a = arr[3];
>
let's add tests for at least:
a) multi-dimensional arrays
b) arrays of pointers (that's unlikely to happen in practice, but
still, we should avoid just messing stuff up silently). We can also
explicitly error out on pointers, I suppose.
c) what about using enums as indices?
d) can we have some typedef'ed types both with array-based and
direct accesses (to validate we skipped typedefs where appropriate)
I'd suggest splitting selftests from veristat changes. They are in the
same selftests/bpf directory, but conceptually they are tool vs tests
patches, so better kept separate, IMO.
pw-bot: cr
> return a;
> }
> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index b2bb20b00952..79c5ea6476ca 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c
> @@ -1379,7 +1379,7 @@ 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;
> }
> @@ -1486,6 +1486,39 @@ static bool is_preset_supported(const struct btf_type *t)
> return btf_is_int(t) || btf_is_enum(t) || btf_is_enum64(t);
> }
>
> +static int adjust_array_secinfo(const struct btf *btf, const struct btf_type *t,
> + struct btf_var_secinfo *sinfo, const char *var)
> +{
> + struct btf_array *barr;
> + const struct btf_type *type;
> + char arr[64], idx[64];
> + int i = 0, tid;
> +
> + if (!btf_is_array(t))
> + return 0;
this shouldn't be called for non-array, no? if yes, then we should
either drop unnecessary safety check or error out
> +
> + barr = btf_array(t);
> + tid = btf__resolve_type(btf, barr->type);
> + type = btf__type_by_id(btf, tid);
> +
> + /* var may contain chained expression e.g.: foo[1].bar */
this feels a bit hacky that we re-parse those string specifiers...
maybe we should parse them once, prepare some structured
representation such that the rest of the code can easily check whether
each access is array or non-array, and have parsed index number for
arrays
> + if (sscanf(var, "%[a-zA-Z0-9_][%[a-zA-Z0-9]]", arr, idx) != 2) {
> + fprintf(stderr, "Could not parse array expression %s\n", var);
> + return -EINVAL;
> + }
> + errno = 0;
> + i = strtol(idx, NULL, 0);
> + if (errno || i < 0 || i >= barr->nelems) {
> + fprintf(stderr, "Preset index %s is invalid or out of bounds [0, %d]\n",
> + idx, barr->nelems);
> + return -EINVAL;
> + }
> + sinfo->size = type->size;
hm... what if type is another array? we need to calculate the size
properly, not just take type->size directly. Or what if it's a pointer
type and type->size is actually type->type?
> + sinfo->type = tid;
> + sinfo->offset += i * type->size;
> + return 0;
> +}
> +
> const int btf_find_member(const struct btf *btf,
> const struct btf_type *parent_type,
> __u32 parent_offset,
> @@ -1493,7 +1526,7 @@ const int btf_find_member(const struct btf *btf,
> int *member_tid,
> __u32 *member_offset)
> {
> - int i;
> + int i, err;
>
> if (!btf_is_composite(parent_type))
> return -EINVAL;
> @@ -1511,8 +1544,12 @@ const int btf_find_member(const struct btf *btf,
> member_type = btf__type_by_id(btf, tid);
> if (member->name_off) {
> const char *name = btf__name_by_offset(btf, member->name_off);
> + int name_len = strlen(name);
>
> - if (strcmp(member_name, name) == 0) {
> + if (strcmp(member_name, name) == 0 ||
> + (btf_is_array(member_type) &&
> + strncmp(name, member_name, name_len) == 0 &&
> + member_name[name_len] == '[')) {
It looks like you are trying to do too much here at the same time...
Why not handle array case separately and explicitly? And we can
provide a nice message if array vs non-array access is detected
Let's also restructure btf_find_member loop logic a bit to reduce nestedness:
const char *name = ...;
if (name[0] == '\0' && btf_is_composite(...)) { /* anon struct/union */
/* recur btf_find_member in hope to find a match */
...
} else if (name[0] && btf_is_array(member_type)) {
/* if member_name is *NOT* blah[something] - nice error, exit with
-EINVAL */
... new array element-specific logic
} else if (name[0]) {
/* if member_name *IS* blah[something] - nice error, exit with -EINVAL */
... old logic ...
}
BTW, we should probably use -ESRCH to specify "we didn't find match"
(and that's ok, we keep backtracking) vs -EINVAL due to array vs
non-array mismatch or bitfield usage. The latter are real error
conditions, while the former should be silently ignored.
> if (btf_member_bitfield_size(parent_type, i) != 0) {
> fprintf(stderr, "Bitfield presets are not supported %s\n",
> name);
> @@ -1520,6 +1557,16 @@ const int btf_find_member(const struct btf *btf,
> }
> *member_offset = parent_offset + member->offset;
> *member_tid = tid;
> + if (btf_is_array(member_type)) {
> + struct btf_var_secinfo sinfo = {.offset = 0};
> +
> + err = adjust_array_secinfo(btf, member_type,
> + &sinfo, member_name);
> + if (err)
> + return err;
> + *member_tid = sinfo.type;
> + *member_offset += sinfo.offset * 8;
> + }
> return 0;
> }
> } else if (btf_is_composite(member_type)) {
> @@ -1548,6 +1595,13 @@ static int adjust_var_secinfo(struct btf *btf, const struct btf_type *t,
> snprintf(expr, sizeof(expr), "%s", var);
> strtok_r(expr, ".", &saveptr);
>
> + if (btf_is_array(base_type)) {
> + err = adjust_array_secinfo(btf, base_type, sinfo, var);
> + if (err)
> + return err;
> + base_type = btf__type_by_id(btf, sinfo->type);
> + }
> +
> while ((name = strtok_r(NULL, ".", &saveptr))) {
> err = btf_find_member(btf, base_type, 0, name, &member_tid, &member_offset);
> if (err) {
> @@ -1673,7 +1727,8 @@ static int set_global_vars(struct bpf_object *obj, struct var_preset *presets, i
>
> if (strncmp(var_name, presets[k].name, var_len) != 0 ||
> (presets[k].name[var_len] != '\0' &&
> - presets[k].name[var_len] != '.'))
> + presets[k].name[var_len] != '.' &&
> + presets[k].name[var_len] != '['))
> continue;
>
> if (presets[k].applied) {
> --
> 2.49.0
>
