[PATCH bpf-next v3 0/3] Support array presets in veristat

[PATCH bpf-next v3 0/3] Support array presets in veristat

[Mykyta Yatsenko]

From: Mykyta Yatsenko <yatsenko@meta.com>

This patch series implements support for array variable presets in
veristat. Currently users can set values to global variables before
loading BPF program, but not for arrays. With this change array
elements are supported as well, for example:
```
sudo ./veristat set_global_vars.bpf.o -G "arr[0] = 1"
```
v2 -> v3
 * Added more negative tests
 * Fix mem leak
 * Other small fixes

v1 -> v2
 * Support enums as indexes
 * Separating parsing logic from preset processing
 * Add more tests

Mykyta Yatsenko (3):
  selftests/bpf: separate var preset parsing in veristat
  selftests/bpf: support array presets in veristat
  selftests/bpf: test array presets in veristat

 .../selftests/bpf/prog_tests/test_veristat.c  | 136 +++++++-
 .../selftests/bpf/progs/set_global_vars.c     |  51 +--
 tools/testing/selftests/bpf/veristat.c        | 312 ++++++++++++++----
 3 files changed, 403 insertions(+), 96 deletions(-)

-- 
2.49.0

[PATCH bpf-next v3 1/3] selftests/bpf: separate var preset parsing in veristat

[Mykyta Yatsenko]

From: Mykyta Yatsenko <yatsenko@meta.com>

Refactor var preset parsing in veristat to simplify implementation.
Prepare parsed variable beforehand so that parsing logic is separated
from functionality of calculating offsets and searching fields.
Introduce variant struct, storing either int or enum (string value),
will be reused in the next patch, extract parsing variant into a
separate function.

Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
---
 tools/testing/selftests/bpf/veristat.c | 160 ++++++++++++++++---------
 1 file changed, 103 insertions(+), 57 deletions(-)

diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index b2bb20b00952..8291de199aab 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -155,13 +155,23 @@ struct filter {
 	bool abs;
 };
 
-struct var_preset {
-	char *name;
-	enum { INTEGRAL, ENUMERATOR } type;
+struct variant {
+	enum { NONE, INTEGRAL, ENUMERATOR } type;
 	union {
 		long long ivalue;
 		char *svalue;
 	};
+};
+
+struct var_preset_atom {
+	char *name;
+};
+
+struct var_preset {
+	struct var_preset_atom *atoms;
+	int atom_count;
+	char *full_name;
+	struct variant value;
 	bool applied;
 };
 
@@ -1278,6 +1288,35 @@ static int max_verifier_log_size(void)
 	return log_size;
 }
 
+static int parse_variant(const char *val, struct variant *variant)
+{
+	long long value;
+	char *val_end;
+
+	if (val[0] == '-' || isdigit(val[0])) {
+		/* must be a number */
+		errno = 0;
+		value = strtoll(val, &val_end, 0);
+		if (errno == ERANGE) {
+			errno = 0;
+			value = strtoull(val, &val_end, 0);
+		}
+		if (errno || *val_end != '\0') {
+			fprintf(stderr, "Failed to parse value '%s'\n", val);
+			return -EINVAL;
+		}
+		variant->ivalue = value;
+		variant->type = INTEGRAL;
+	} else {
+		/* if not a number, consider it enum value */
+		variant->svalue = strdup(val);
+		if (!variant->svalue)
+			return -ENOMEM;
+		variant->type = ENUMERATOR;
+	}
+	return 0;
+}
+
 static int process_prog(const char *filename, struct bpf_object *obj, struct bpf_program *prog)
 {
 	const char *base_filename = basename(strdupa(filename));
@@ -1363,13 +1402,36 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
 	return 0;
 };
 
+static int parse_var_atoms(const char *full_var, struct var_preset *preset)
+{
+	char expr[256], *name, *saveptr;
+
+	snprintf(expr, sizeof(expr), "%s", full_var);
+	preset->atom_count = 0;
+	while ((name = strtok_r(preset->atom_count ? NULL : expr, ".", &saveptr))) {
+		struct var_preset_atom *tmp;
+		int i = preset->atom_count;
+
+		tmp = reallocarray(preset->atoms, i + 1, sizeof(*preset->atoms));
+		if (!tmp)
+			return -ENOMEM;
+
+		preset->atoms = tmp;
+		preset->atom_count++;
+
+		preset->atoms[i].name = strdup(name);
+		if (!preset->atoms[i].name)
+			return -ENOMEM;
+	}
+	return 0;
+}
+
 static int append_var_preset(struct var_preset **presets, int *cnt, const char *expr)
 {
 	void *tmp;
 	struct var_preset *cur;
-	char var[256], val[256], *val_end;
-	long long value;
-	int n;
+	char var[256], val[256];
+	int n, err;
 
 	tmp = realloc(*presets, (*cnt + 1) * sizeof(**presets));
 	if (!tmp)
@@ -1384,32 +1446,18 @@ static int append_var_preset(struct var_preset **presets, int *cnt, const char *
 		return -EINVAL;
 	}
 
-	if (val[0] == '-' || isdigit(val[0])) {
-		/* must be a number */
-		errno = 0;
-		value = strtoll(val, &val_end, 0);
-		if (errno == ERANGE) {
-			errno = 0;
-			value = strtoull(val, &val_end, 0);
-		}
-		if (errno || *val_end != '\0') {
-			fprintf(stderr, "Failed to parse value '%s'\n", val);
-			return -EINVAL;
-		}
-		cur->ivalue = value;
-		cur->type = INTEGRAL;
-	} else {
-		/* if not a number, consider it enum value */
-		cur->svalue = strdup(val);
-		if (!cur->svalue)
-			return -ENOMEM;
-		cur->type = ENUMERATOR;
-	}
+	err = parse_variant(val, &cur->value);
+	if (err)
+		return err;
 
-	cur->name = strdup(var);
-	if (!cur->name)
+	cur->full_name = strdup(var);
+	if (!cur->full_name)
 		return -ENOMEM;
 
+	err = parse_var_atoms(var, cur);
+	if (err)
+		return err;
+
 	return 0;
 }
 
@@ -1489,7 +1537,7 @@ static bool is_preset_supported(const struct btf_type *t)
 const int btf_find_member(const struct btf *btf,
 			  const struct btf_type *parent_type,
 			  __u32 parent_offset,
-			  const char *member_name,
+			  struct var_preset_atom *var_atom,
 			  int *member_tid,
 			  __u32 *member_offset)
 {
@@ -1512,7 +1560,7 @@ const int btf_find_member(const struct btf *btf,
 		if (member->name_off) {
 			const char *name = btf__name_by_offset(btf, member->name_off);
 
-			if (strcmp(member_name, name) == 0) {
+			if (strcmp(var_atom->name, name) == 0) {
 				if (btf_member_bitfield_size(parent_type, i) != 0) {
 					fprintf(stderr, "Bitfield presets are not supported %s\n",
 						name);
@@ -1526,7 +1574,7 @@ const int btf_find_member(const struct btf *btf,
 			int err;
 
 			err = btf_find_member(btf, member_type, parent_offset + member->offset,
-					      member_name, member_tid, member_offset);
+					      var_atom, member_tid, member_offset);
 			if (!err)
 				return 0;
 		}
@@ -1536,22 +1584,20 @@ const int btf_find_member(const struct btf *btf,
 }
 
 static int adjust_var_secinfo(struct btf *btf, const struct btf_type *t,
-			      struct btf_var_secinfo *sinfo, const char *var)
+			      struct btf_var_secinfo *sinfo, struct var_preset *preset)
 {
-	char expr[256], *saveptr;
 	const struct btf_type *base_type, *member_type;
-	int err, member_tid;
-	char *name;
+	int err, member_tid, i;
 	__u32 member_offset = 0;
 
 	base_type = btf__type_by_id(btf, btf__resolve_type(btf, t->type));
-	snprintf(expr, sizeof(expr), "%s", var);
-	strtok_r(expr, ".", &saveptr);
 
-	while ((name = strtok_r(NULL, ".", &saveptr))) {
-		err = btf_find_member(btf, base_type, 0, name, &member_tid, &member_offset);
+	for (i = 1; i < preset->atom_count; ++i) {
+		err = btf_find_member(btf, base_type, 0, &preset->atoms[i],
+				      &member_tid, &member_offset);
 		if (err) {
-			fprintf(stderr, "Could not find member %s for variable %s\n", name, var);
+			fprintf(stderr, "Could not find member %s for variable %s\n",
+				preset->atoms[i].name, preset->atoms[i - 1].name);
 			return err;
 		}
 		member_type = btf__type_by_id(btf, member_tid);
@@ -1569,7 +1615,7 @@ static int set_global_var(struct bpf_object *obj, struct btf *btf,
 {
 	const struct btf_type *base_type;
 	void *ptr;
-	long long value = preset->ivalue;
+	long long value = preset->value.ivalue;
 	size_t size;
 
 	base_type = btf__type_by_id(btf, btf__resolve_type(btf, sinfo->type));
@@ -1583,17 +1629,18 @@ static int set_global_var(struct bpf_object *obj, struct btf *btf,
 		return -EINVAL;
 	}
 
-	if (preset->type == ENUMERATOR) {
+	if (preset->value.type == ENUMERATOR) {
 		if (btf_is_any_enum(base_type)) {
-			if (enum_value_from_name(btf, base_type, preset->svalue, &value)) {
+			if (enum_value_from_name(btf, base_type, preset->value.svalue, &value)) {
 				fprintf(stderr,
 					"Failed to find integer value for enum element %s\n",
-					preset->svalue);
+					preset->value.svalue);
 				return -EINVAL;
 			}
 		} else {
 			fprintf(stderr, "Value %s is not supported for type %s\n",
-				preset->svalue, btf__name_by_offset(btf, base_type->name_off));
+				preset->value.svalue,
+				btf__name_by_offset(btf, base_type->name_off));
 			return -EINVAL;
 		}
 	}
@@ -1660,20 +1707,16 @@ static int set_global_vars(struct bpf_object *obj, struct var_preset *presets, i
 		for (j = 0; j < n; ++j, ++sinfo) {
 			const struct btf_type *var_type = btf__type_by_id(btf, sinfo->type);
 			const char *var_name;
-			int var_len;
 
 			if (!btf_is_var(var_type))
 				continue;
 
 			var_name = btf__name_by_offset(btf, var_type->name_off);
-			var_len = strlen(var_name);
 
 			for (k = 0; k < npresets; ++k) {
 				struct btf_var_secinfo tmp_sinfo;
 
-				if (strncmp(var_name, presets[k].name, var_len) != 0 ||
-				    (presets[k].name[var_len] != '\0' &&
-				     presets[k].name[var_len] != '.'))
+				if (strcmp(var_name, presets[k].atoms[0].name) != 0)
 					continue;
 
 				if (presets[k].applied) {
@@ -1683,7 +1726,7 @@ static int set_global_vars(struct bpf_object *obj, struct var_preset *presets, i
 				}
 				tmp_sinfo = *sinfo;
 				err = adjust_var_secinfo(btf, var_type,
-							 &tmp_sinfo, presets[k].name);
+							 &tmp_sinfo, presets + k);
 				if (err)
 					return err;
 
@@ -1698,7 +1741,7 @@ static int set_global_vars(struct bpf_object *obj, struct var_preset *presets, i
 	for (i = 0; i < npresets; ++i) {
 		if (!presets[i].applied) {
 			fprintf(stderr, "Global variable preset %s has not been applied\n",
-				presets[i].name);
+				presets[i].full_name);
 		}
 		presets[i].applied = false;
 	}
@@ -2826,7 +2869,7 @@ static int handle_replay_mode(void)
 
 int main(int argc, char **argv)
 {
-	int err = 0, i;
+	int err = 0, i, j;
 
 	if (argp_parse(&argp, argc, argv, 0, NULL, NULL))
 		return 1;
@@ -2885,9 +2928,12 @@ int main(int argc, char **argv)
 	}
 	free(env.deny_filters);
 	for (i = 0; i < env.npresets; ++i) {
-		free(env.presets[i].name);
-		if (env.presets[i].type == ENUMERATOR)
-			free(env.presets[i].svalue);
+		free(env.presets[i].full_name);
+		for (j = 0; j < env.presets[i].atom_count; ++j)
+			free(env.presets[i].atoms[j].name);
+		free(env.presets[i].atoms);
+		if (env.presets[i].value.type == ENUMERATOR)
+			free(env.presets[i].value.svalue);
 	}
 	free(env.presets);
 	return -err;
-- 
2.49.0

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

[Mykyta Yatsenko]

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>
---
 tools/testing/selftests/bpf/veristat.c | 164 +++++++++++++++++++++----
 1 file changed, 142 insertions(+), 22 deletions(-)

diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index 8291de199aab..bc9ebf5a2985 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -165,6 +165,7 @@ struct variant {
 
 struct var_preset_atom {
 	char *name;
+	struct variant index;
 };
 
 struct var_preset {
@@ -1404,7 +1405,8 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
 
 static int parse_var_atoms(const char *full_var, struct var_preset *preset)
 {
-	char expr[256], *name, *saveptr;
+	char expr[256], var[256], idx[256], *name, *saveptr;
+	int n, err;
 
 	snprintf(expr, sizeof(expr), "%s", full_var);
 	preset->atom_count = 0;
@@ -1419,9 +1421,25 @@ static int parse_var_atoms(const char *full_var, struct var_preset *preset)
 		preset->atoms = tmp;
 		preset->atom_count++;
 
-		preset->atoms[i].name = strdup(name);
-		if (!preset->atoms[i].name)
-			return -ENOMEM;
+		if (sscanf(name, "%[a-zA-Z0-9_][%[a-zA-Z0-9]] %n", var, idx, &n) == 2 &&
+		    strlen(name) == n) {
+			/* current atom is an array, parse index */
+			preset->atoms[i].name = strdup(var);
+			if (!preset->atoms[i].name)
+				return -ENOMEM;
+			err = parse_variant(idx, &preset->atoms[i].index);
+			if (err)
+				return err;
+		} else if (sscanf(name, "%[a-zA-Z0-9_] %n", var, &n) == 1 &&
+			   strlen(name) == n) {
+			preset->atoms[i].name = strdup(name);
+			if (!preset->atoms[i].name)
+				return -ENOMEM;
+			preset->atoms[i].index.type = NONE;
+		} else {
+			fprintf(stderr, "Could not parse '%s'", name);
+			return -EINVAL;
+		}
 	}
 	return 0;
 }
@@ -1441,7 +1459,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;
 	}
@@ -1534,6 +1552,75 @@ 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 find_enum_value(const struct btf *btf, const char *name, long long *value)
+{
+	const struct btf_type *t;
+	int cnt, i;
+	long long lvalue;
+
+	cnt = btf__type_cnt(btf);
+	for (i = 1; i != cnt; ++i) {
+		t = btf__type_by_id(btf, i);
+
+		if (!btf_is_any_enum(t))
+			continue;
+
+		if (enum_value_from_name(btf, t, name, &lvalue) == 0) {
+			*value = lvalue;
+			return 0;
+		}
+	}
+	return -ESRCH;
+}
+
+static int adjust_array_secinfo(const struct btf *btf, const struct btf_type *t,
+				const struct var_preset_atom *var_atom,
+				struct btf_var_secinfo *sinfo)
+{
+	struct btf_array *barr;
+	const struct btf_type *type;
+	long long index;
+	int tid;
+
+	if (!btf_is_array(t))
+		return -EINVAL;
+
+	barr = btf_array(t);
+	tid = btf__resolve_type(btf, barr->type);
+	type = btf__type_by_id(btf, tid);
+	if (!btf_is_int(type) && !btf_is_any_enum(type) && !btf_is_composite(type)) {
+		fprintf(stderr,
+			"Unsupported array element type for variable %s. Only int, enum, struct, union are supported\n",
+			var_atom->name);
+		return -EINVAL;
+	}
+	switch (var_atom->index.type) {
+	case INTEGRAL:
+		index = var_atom->index.ivalue;
+		break;
+	case ENUMERATOR:
+		if (find_enum_value(btf, var_atom->index.svalue, &index) != 0) {
+			fprintf(stderr, "Can't resolve %s enum as an array index",
+				var_atom->index.svalue);
+			return -EINVAL;
+		}
+		break;
+	case NONE:
+		fprintf(stderr, "Array index is expected for %s\n", var_atom->name);
+		return -EINVAL;
+	}
+
+	if (index < 0 || index >= barr->nelems) {
+		fprintf(stderr, "Preset index %lld is invalid or out of bounds [0, %u]\n",
+			index, barr->nelems);
+		return -EINVAL;
+	}
+	sinfo->size = type->size;
+	sinfo->type = tid;
+	sinfo->offset += index * type->size;
+	return 0;
+}
+
 const int btf_find_member(const struct btf *btf,
 			  const struct btf_type *parent_type,
 			  __u32 parent_offset,
@@ -1541,7 +1628,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;
@@ -1550,37 +1637,57 @@ const int btf_find_member(const struct btf *btf,
 		const struct btf_member *member;
 		const struct btf_type *member_type;
 		int tid;
+		const char *name;
+		u32 offset;
 
 		member = btf_members(parent_type) + i;
 		tid =  btf__resolve_type(btf, member->type);
 		if (tid < 0)
 			return -EINVAL;
 
+		name = btf__name_by_offset(btf, member->name_off);
+		if (name[0] != '\0' && strcmp(var_atom->name, name) != 0)
+			continue;
+
+		if (btf_member_bitfield_size(parent_type, i) != 0) {
+			fprintf(stderr, "Bitfield presets are not supported %s\n",
+				name);
+			return -EINVAL;
+		}
+
 		member_type = btf__type_by_id(btf, tid);
-		if (member->name_off) {
-			const char *name = btf__name_by_offset(btf, member->name_off);
+		offset = parent_offset + member->offset;
 
-			if (strcmp(var_atom->name, name) == 0) {
-				if (btf_member_bitfield_size(parent_type, i) != 0) {
-					fprintf(stderr, "Bitfield presets are not supported %s\n",
-						name);
-					return -EINVAL;
-				}
-				*member_offset = parent_offset + member->offset;
-				*member_tid = tid;
-				return 0;
-			}
-		} else if (btf_is_composite(member_type)) {
+		if (name[0] == '\0' && btf_is_composite(member_type)) { /* anon struct/union */
 			int err;
 
-			err = btf_find_member(btf, member_type, parent_offset + member->offset,
+			err = btf_find_member(btf, member_type, offset,
 					      var_atom, member_tid, member_offset);
 			if (!err)
 				return 0;
+		} else if (name[0] && btf_is_array(member_type)) {
+			struct btf_var_secinfo sinfo = {.offset = 0};
+
+			err = adjust_array_secinfo(btf, member_type, var_atom, &sinfo);
+			if (err)
+				return err;
+
+			*member_tid = sinfo.type;
+			*member_offset = offset + sinfo.offset * 8;
+			return 0;
+		} else if (name[0]) {
+			if (var_atom->index.type != NONE) {
+				fprintf(stderr, "Array index is not expected for %s\n", name);
+				return -EINVAL;
+			}
+
+			*member_offset = offset;
+			*member_tid = tid;
+			return 0;
 		}
 	}
 
-	return -EINVAL;
+	return -ESRCH;
 }
 
 static int adjust_var_secinfo(struct btf *btf, const struct btf_type *t,
@@ -1591,6 +1698,15 @@ static int adjust_var_secinfo(struct btf *btf, const struct btf_type *t,
 	__u32 member_offset = 0;
 
 	base_type = btf__type_by_id(btf, btf__resolve_type(btf, t->type));
+	if (btf_is_array(base_type)) {
+		err = adjust_array_secinfo(btf, base_type, &preset->atoms[0], sinfo);
+		if (err)
+			return err;
+		base_type = btf__type_by_id(btf, sinfo->type);
+	} else if (preset->atoms[0].index.type != NONE) {
+		fprintf(stderr, "Array index is not expected for %s\n", preset->atoms[0].name);
+		return -EINVAL;
+	}
 
 	for (i = 1; i < preset->atom_count; ++i) {
 		err = btf_find_member(btf, base_type, 0, &preset->atoms[i],
@@ -1742,6 +1858,7 @@ static int set_global_vars(struct bpf_object *obj, struct var_preset *presets, i
 		if (!presets[i].applied) {
 			fprintf(stderr, "Global variable preset %s has not been applied\n",
 				presets[i].full_name);
+			err = -EINVAL;
 		}
 		presets[i].applied = false;
 	}
@@ -2929,8 +3046,11 @@ 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)
+		for (j = 0; j < env.presets[i].atom_count; ++j) {
 			free(env.presets[i].atoms[j].name);
+			if (env.presets[i].atoms[j].index.type == ENUMERATOR)
+				free(env.presets[i].atoms[j].index.svalue);
+		}
 		free(env.presets[i].atoms);
 		if (env.presets[i].value.type == ENUMERATOR)
 			free(env.presets[i].value.svalue);
-- 
2.49.0

[PATCH bpf-next v3 3/3] selftests/bpf: test array presets in veristat

[Mykyta Yatsenko]

From: Mykyta Yatsenko <yatsenko@meta.com>

Modify existing veristat tests to verify that array presets are applied
as expected.
Introduce few negative tests as well to check that common error modes
are handled.

Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
---
 .../selftests/bpf/prog_tests/test_veristat.c  | 136 +++++++++++++++++-
 .../selftests/bpf/progs/set_global_vars.c     |  51 ++++---
 2 files changed, 164 insertions(+), 23 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..bd3c04d9c54d 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_veristat.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_veristat.c
@@ -60,12 +60,15 @@ 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\" "\
 	    " -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[3]= 171\" "	\
+	    " -G \"arr[EA2] =172\" "	\
+	    " -G \"enum_arr[EC2]=EA3\" "	\
 	    "-vl2 > %s", fix->veristat, fix->tmpfile);
 
 	read(fix->fd, fix->output, fix->sz);
@@ -81,8 +84,11 @@ static void test_set_global_vars_succeeds(void)
 	__CHECK_STR("_w=12 ", "var_eb = EB2");
 	__CHECK_STR("_w=13 ", "var_ec = EC2");
 	__CHECK_STR("_w=1 ", "var_b = 1");
-	__CHECK_STR("_w=170 ", "struct1.struct2.u.var_u8 = 170");
+	__CHECK_STR("_w=170 ", "struct1.struct2[1].u.var_u8[2] = 170");
 	__CHECK_STR("_w=0xaaaa ", "union1.var_u16 = 0xaaaa");
+	__CHECK_STR("_w=171 ", "arr[3]= 171");
+	__CHECK_STR("_w=172 ", "arr[EA2] =172");
+	__CHECK_STR("_w=10 ", "enum_arr[EC2]=EA3");
 
 out:
 	teardown_fixture(fix);
@@ -129,6 +135,110 @@ static void test_set_global_vars_out_of_range(void)
 	teardown_fixture(fix);
 }
 
+static void test_unsupported_2dim_array_type(void)
+{
+	struct fixture *fix = init_fixture();
+
+	SYS_FAIL(out,
+		 "%s set_global_vars.bpf.o -G \"matrix[0][0] = 1\" -vl2 2> %s",
+		 fix->veristat, fix->tmpfile);
+
+	read(fix->fd, fix->output, fix->sz);
+	__CHECK_STR("Could not parse 'matrix[0][0]'", "matrix");
+
+out:
+	teardown_fixture(fix);
+}
+
+static void test_unsupported_ptr_array_type(void)
+{
+	struct fixture *fix = init_fixture();
+
+	SYS_FAIL(out,
+		 "%s set_global_vars.bpf.o -G \"ptr_arr[0] = 0\" -vl2 2> %s",
+		 fix->veristat, fix->tmpfile);
+
+	read(fix->fd, fix->output, fix->sz);
+	__CHECK_STR("Unsupported array element type for variable ptr_arr", "ptr_arr");
+
+out:
+	teardown_fixture(fix);
+}
+
+static void test_array_out_of_bounds(void)
+{
+	struct fixture *fix = init_fixture();
+
+	SYS_FAIL(out,
+		 "%s set_global_vars.bpf.o -G \"arr[99] = 0\" -vl2 2> %s",
+		 fix->veristat, fix->tmpfile);
+
+	read(fix->fd, fix->output, fix->sz);
+	__CHECK_STR("Preset index 99 is invalid or out of bounds", "arr[99]");
+
+out:
+	teardown_fixture(fix);
+}
+
+static void test_array_index_not_found(void)
+{
+	struct fixture *fix = init_fixture();
+
+	SYS_FAIL(out,
+		 "%s set_global_vars.bpf.o -G \"arr[EG2] = 0\" -vl2 2> %s",
+		 fix->veristat, fix->tmpfile);
+
+	read(fix->fd, fix->output, fix->sz);
+	__CHECK_STR("Can't resolve EG2 enum as an array indexFailed to set global variables", "arr[EG2]");
+
+out:
+	teardown_fixture(fix);
+}
+
+static void test_array_index_for_non_array(void)
+{
+	struct fixture *fix = init_fixture();
+
+	SYS_FAIL(out,
+		 "%s set_global_vars.bpf.o -G \"var_b[0] = 1\" -vl2 2> %s",
+		 fix->veristat, fix->tmpfile);
+
+	pread(fix->fd, fix->output, fix->sz, 0);
+	__CHECK_STR("Array index is not expected for var_b", "var_b[0] = 1");
+
+	SYS_FAIL(out,
+		 "%s set_global_vars.bpf.o -G \"union1.struct3[0].var_u8_l=1\" -vl2 2> %s",
+		 fix->veristat, fix->tmpfile);
+
+	pread(fix->fd, fix->output, fix->sz, 0);
+	__CHECK_STR("Array index is not expected for struct3", "union1.struct3[0].var_u8_l=1");
+
+out:
+	teardown_fixture(fix);
+}
+
+static void test_no_array_index_for_array(void)
+{
+	struct fixture *fix = init_fixture();
+
+	SYS_FAIL(out,
+		 "%s set_global_vars.bpf.o -G \"arr = 1\" -vl2 2> %s",
+		 fix->veristat, fix->tmpfile);
+
+	pread(fix->fd, fix->output, fix->sz, 0);
+	__CHECK_STR("Array index is expected for arr", "arr = 1");
+
+	SYS_FAIL(out,
+		 "%s set_global_vars.bpf.o -G \"struct1[0].struct2.u.var_u8[2]=1\" -vl2 2> %s",
+		 fix->veristat, fix->tmpfile);
+
+	pread(fix->fd, fix->output, fix->sz, 0);
+	__CHECK_STR("Array index is expected for struct2", "struct1[0].struct2.u.var_u8[2]=1");
+
+out:
+	teardown_fixture(fix);
+}
+
 void test_veristat(void)
 {
 	if (test__start_subtest("set_global_vars_succeeds"))
@@ -139,6 +249,24 @@ void test_veristat(void)
 
 	if (test__start_subtest("set_global_vars_from_file_succeeds"))
 		test_set_global_vars_from_file_succeeds();
+
+	if (test__start_subtest("test_unsupported_2dim_array_type"))
+		test_unsupported_2dim_array_type();
+
+	if (test__start_subtest("test_unsupported_ptr_array_type"))
+		test_unsupported_ptr_array_type();
+
+	if (test__start_subtest("test_array_out_of_bounds"))
+		test_array_out_of_bounds();
+
+	if (test__start_subtest("test_array_index_not_found"))
+		test_array_index_not_found();
+
+	if (test__start_subtest("test_array_index_for_non_array"))
+		test_array_index_for_non_array();
+
+	if (test__start_subtest("test_no_array_index_for_array"))
+		test_no_array_index_for_array();
 }
 
 #undef __CHECK_STR
diff --git a/tools/testing/selftests/bpf/progs/set_global_vars.c b/tools/testing/selftests/bpf/progs/set_global_vars.c
index 90f5656c3991..a022c08895b9 100644
--- a/tools/testing/selftests/bpf/progs/set_global_vars.c
+++ b/tools/testing/selftests/bpf/progs/set_global_vars.c
@@ -7,22 +7,30 @@
 
 char _license[] SEC("license") = "GPL";
 
-enum Enum { EA1 = 0, EA2 = 11 };
+typedef __s32 s32;
+typedef s32 i32;
+typedef __u8 u8;
+
+enum Enum { EA1 = 0, EA2 = 11, EA3 = 10 };
 enum Enumu64 {EB1 = 0llu, EB2 = 12llu };
 enum Enums64 { EC1 = 0ll, EC2 = 13ll };
 
 const volatile __s64 var_s64 = -1;
 const volatile __u64 var_u64 = 0;
-const volatile __s32 var_s32 = -1;
+const volatile i32 var_s32 = -1;
 const volatile __u32 var_u32 = 0;
 const volatile __s16 var_s16 = -1;
 const volatile __u16 var_u16 = 0;
 const volatile __s8 var_s8 = -1;
-const volatile __u8 var_u8 = 0;
+const volatile u8 var_u8 = 0;
 const volatile enum Enum var_ea = EA1;
 const volatile enum Enumu64 var_eb = EB1;
 const volatile enum Enums64 var_ec = EC1;
 const volatile bool var_b = false;
+const volatile i32 arr[32];
+const volatile enum Enum enum_arr[32];
+const volatile i32 matrix[32][32];
+const volatile i32 *ptr_arr[32];
 
 struct Struct {
 	int:16;
@@ -35,34 +43,36 @@ struct Struct {
 		volatile struct {
 			const int:1;
 			union {
-				const volatile __u8 var_u8;
+				const volatile u8 var_u8[3];
 				const volatile __s16 filler3;
 				const int:1;
 			} u;
 		};
-	} struct2;
+	} struct2[2];
 };
 
 const volatile __u32 stru = 0; /* same prefix as below */
-const volatile struct Struct struct1 = {.struct2 = {.u = {.var_u8 = 1}}};
+const volatile struct Struct struct1[3] = {{.struct2 = {{}, {.u = {.var_u8 = {1}}}}}};
 
-union Union {
-	__u16 var_u16;
-	struct Struct3 {
-		struct {
-			__u8 var_u8_l;
-		};
+struct Struct3 {
+	struct {
+		u8 var_u8_l;
+	};
+	struct {
 		struct {
-			struct {
-				__u8 var_u8_h;
-			};
+			u8 var_u8_h;
 		};
-	} struct3;
+	};
 };
 
-const volatile union Union union1 = {.var_u16 = -1};
+typedef struct Struct3 Struct3_t;
 
-char arr[4] = {0};
+union Union {
+	__u16 var_u16;
+	Struct3_t struct3;
+};
+
+const volatile union Union union1 = {.var_u16 = -1};
 
 SEC("socket")
 int test_set_globals(void *ctx)
@@ -81,8 +91,11 @@ 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];
+	a = arr[EA2];
+	a = enum_arr[EC2];
 
 	return a;
 }
-- 
2.49.0

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

[Eduard Zingerman]

[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]

On Tue, 2025-06-10 at 20:08 +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>
> ---

A few nits regarding error reporting:

  ./veristat -q -G ptr_arr[10]=0 set_global_vars.bpf.o 
  Unsupported array element type for variable ptr_arr. Only int, enum, struct, union are supported
                                                                                                 ^^^
											 missing dot

  ./veristat -G arr[[10]]=0 set_global_vars.bpf.o 
  Could not parse 'arr[[10]]'Failed to parse global variable presets: arr[[10]]=0
  ^^^^^^^^^                ^^^                                                  ^^^
  Can't ?		   dot or comma                                 missing dot

  ./veristat -q -G "struct1[0] = 0" set_global_vars.bpf.o 
  Setting value for type Struct is not supported
                                           ^^^^^^^^^
					   report full_name here?

I applied a diff as in the attachment, to see what offsets are being assigned.
Looks like this shows a bug:

  ./veristat  -q -G "struct1[0].filler = 42" set_global_vars.bpf.o > /dev/null
  setting struct1[0].filler: offset 54, kind int, value 42

Shouldn't offset be 2 in this case?

(maybe print such info in debug (-d) mode?)

Unrelated to this patch, but still a bug:

  # Catches range error:
  ./veristat -q -G "struct1[0].filler2 = 100500" set_global_vars.bpf.o 
  Variable unsigned short value 100500 is out of range [0; 65535]
  # Does not range error:
  ./veristat -q -G "struct1[0].filler2 = -1" set_global_vars.bpf.o
  ... success ...

[...]

[-- Attachment #2: show-offset.patch --]
[-- Type: text/x-patch, Size: 1039 bytes --]

diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index bc9ebf5a2985..7f3e8ba75acc 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -1725,6 +1725,8 @@ static int adjust_var_secinfo(struct btf *btf, const struct btf_type *t,
        return 0;
 }
 
+const char *btf_kind_str(const struct btf_type *t);
+
 static int set_global_var(struct bpf_object *obj, struct btf *btf,
                          struct bpf_map *map, struct btf_var_secinfo *sinfo,
                          struct var_preset *preset)
@@ -1761,6 +1763,9 @@ static int set_global_var(struct bpf_object *obj, struct btf *btf,
                }
        }
 
+       fprintf(stderr, "setting %s: offset %d, kind %s, value %lld\n",
+               preset->full_name, sinfo->offset, btf_kind_str(base_type), value);
+
        /* Check if value fits into the target variable size */
        if  (sinfo->size < sizeof(value)) {
                bool is_signed = is_signed_type(base_type);

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

[Eduard Zingerman]

On Tue, 2025-06-10 at 17:45 -0700, Eduard Zingerman wrote:

[...]

> I applied a diff as in the attachment, to see what offsets are being assigned.
> Looks like this shows a bug:
> 
>   ./veristat  -q -G "struct1[0].filler = 42" set_global_vars.bpf.o > /dev/null
>   setting struct1[0].filler: offset 54, kind int, value 42
> 
> Shouldn't offset be 2 in this case?

Nevermind, it's an offset inside section, sorry for the noise.

[...]

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

[Mykyta Yatsenko]

On 6/11/25 01:45, Eduard Zingerman wrote:
> On Tue, 2025-06-10 at 20:08 +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>
>> ---
> A few nits regarding error reporting:
>
>    ./veristat -q -G ptr_arr[10]=0 set_global_vars.bpf.o
>    Unsupported array element type for variable ptr_arr. Only int, enum, struct, union are supported
>                                                                                                   ^^^
> 											 missing dot
>
>    ./veristat -G arr[[10]]=0 set_global_vars.bpf.o
>    Could not parse 'arr[[10]]'Failed to parse global variable presets: arr[[10]]=0
>    ^^^^^^^^^                ^^^                                                  ^^^
>    Can't ?		   dot or comma                                 missing dot
>
>    ./veristat -q -G "struct1[0] = 0" set_global_vars.bpf.o
>    Setting value for type Struct is not supported
>                                             ^^^^^^^^^
> 					   report full_name here?
>
> I applied a diff as in the attachment, to see what offsets are being assigned.
> Looks like this shows a bug:
>
>    ./veristat  -q -G "struct1[0].filler = 42" set_global_vars.bpf.o > /dev/null
>    setting struct1[0].filler: offset 54, kind int, value 42
>
> Shouldn't offset be 2 in this case?
>
> (maybe print such info in debug (-d) mode?)
>
> Unrelated to this patch, but still a bug:
>
>    # Catches range error:
>    ./veristat -q -G "struct1[0].filler2 = 100500" set_global_vars.bpf.o
>    Variable unsigned short value 100500 is out of range [0; 65535]
>    # Does not range error:
>    ./veristat -q -G "struct1[0].filler2 = -1" set_global_vars.bpf.o
Thanks for taking a look and checking few testcases.
I'll fix this one in the next patch, along with the error messages.
>    ... success ...
>
> [...]

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

[Eduard Zingerman]

What do you think about a more recursive representation for presets?
E.g. as follows:

  struct rvalue {
    long long i; /* use find_enum_value() at parse time to avoid union */
  };
  
  struct lvalue {
    enum { VAR, FIELD, ARRAY } type;
    union {
      struct {
        char *name;
      } var;
      struct {
        struct lvalue *base;
        char *name;
      } field;
      struct {
        struct lvalue *base;
        struct rvalue index;
      } array;
    };
  };
  
  struct preset {
    struct lvalue *lv;
    struct rvalue rv;
  };

It can handle matrices ("a[2][3]") and offset/type computation would
be a simple recursive function.

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

[Mykyta Yatsenko]

On 6/11/25 13:21, Eduard Zingerman wrote:
> What do you think about a more recursive representation for presets?
> E.g. as follows:
>
>    struct rvalue {
>      long long i; /* use find_enum_value() at parse time to avoid union */
>    };
>    
>    struct lvalue {
>      enum { VAR, FIELD, ARRAY } type;
>      union {
>        struct {
>          char *name;
>        } var;
>        struct {
>          struct lvalue *base;
>          char *name;
>        } field;
>        struct {
>          struct lvalue *base;
>          struct rvalue index;
>        } array;
>      };
>    };
>    
>    struct preset {
>      struct lvalue *lv;
>      struct rvalue rv;
>    };
>
> It can handle matrices ("a[2][3]") and offset/type computation would
> be a simple recursive function.
>
Yes, this looks cleaner, if we want to support multi-dimensional arrays,
recursive representation works well. A minor problem is that we don't 
have BTF at parsing time,
so resolving enums early won't be possible.

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

[Eduard Zingerman]

On Wed, 2025-06-11 at 14:32 +0100, Mykyta Yatsenko wrote:
> On 6/11/25 13:21, Eduard Zingerman wrote:
> > What do you think about a more recursive representation for presets?
> > E.g. as follows:
> > 
> >    struct rvalue {
> >      long long i; /* use find_enum_value() at parse time to avoid union */
> >    };
> >    
> >    struct lvalue {
> >      enum { VAR, FIELD, ARRAY } type;
> >      union {
> >        struct {
> >          char *name;
> >        } var;
> >        struct {
> >          struct lvalue *base;
> >          char *name;
> >        } field;
> >        struct {
> >          struct lvalue *base;
> >          struct rvalue index;
> >        } array;
> >      };
> >    };
> >    
> >    struct preset {
> >      struct lvalue *lv;
> >      struct rvalue rv;
> >    };
> > 
> > It can handle matrices ("a[2][3]") and offset/type computation would
> > be a simple recursive function.
> > 
> Yes, this looks cleaner, if we want to support multi-dimensional arrays,
> recursive representation works well.

I coded a scketch of an implementation [1] and it looks like for the
same amount of code or slightly less the support for multi-dimensional
arrays can be gained. And code looks a bit simpler, but that's subjective.

> A minor problem is that we don't have BTF at parsing time, so
> resolving enums early won't be possible.

My bad, the name of the enum constant would be necessary.

---

Anyways, I read through the code in this patch-set and am now
convinced that it works.

[1] https://gist.github.com/eddyz87/a96e6b2e1697f19f6e74c5621ecc48c2

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

[Andrii Nakryiko]

On Wed, Jun 11, 2025 at 5:21 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> What do you think about a more recursive representation for presets?
> E.g. as follows:
>
>   struct rvalue {
>     long long i; /* use find_enum_value() at parse time to avoid union */
>   };
>
>   struct lvalue {
>     enum { VAR, FIELD, ARRAY } type;
>     union {
>       struct {
>         char *name;
>       } var;
>       struct {
>         struct lvalue *base;
>         char *name;
>       } field;
>       struct {
>         struct lvalue *base;
>         struct rvalue index;
>       } array;
>     };
>   };
>
>   struct preset {
>     struct lvalue *lv;
>     struct rvalue rv;
>   };
>
> It can handle matrices ("a[2][3]") and offset/type computation would
> be a simple recursive function.
>

Why do we need recursion, though? All we should need is an array specs
of one of the following types:

  a) field access by name
    a.1) we might want to distinguish array field vs non-array field
to better catch unintended user errors
  b) indexing by immediate integer
  c) indexing by symbolic enum name (or we can combine b and c,
whatever works better).

And that's all. And it will support multi-dimensional arrays.

We then process this array one at a time. Each *step* itself might be
recursive: finding a field by name in C struct is necessarily
recursive due to anonymous embedded struct/union fields. But other
than that, it's a simple sequential operation.

So unless I'm missing something, let's not add data recursion if it's
not needed.

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

[Eduard Zingerman]

On Fri, 2025-06-13 at 09:34 -0700, Andrii Nakryiko wrote:
> On Wed, Jun 11, 2025 at 5:21 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > What do you think about a more recursive representation for presets?
> > E.g. as follows:
> > 
> >   struct rvalue {
> >     long long i; /* use find_enum_value() at parse time to avoid union */
> >   };
> > 
> >   struct lvalue {
> >     enum { VAR, FIELD, ARRAY } type;
> >     union {
> >       struct {
> >         char *name;
> >       } var;
> >       struct {
> >         struct lvalue *base;
> >         char *name;
> >       } field;
> >       struct {
> >         struct lvalue *base;
> >         struct rvalue index;
> >       } array;
> >     };
> >   };
> > 
> >   struct preset {
> >     struct lvalue *lv;
> >     struct rvalue rv;
> >   };
> > 
> > It can handle matrices ("a[2][3]") and offset/type computation would
> > be a simple recursive function.
> > 
> 
> Why do we need recursion, though? All we should need is an array specs
> of one of the following types:
> 
>   a) field access by name
>     a.1) we might want to distinguish array field vs non-array field
> to better catch unintended user errors
>   b) indexing by immediate integer
>   c) indexing by symbolic enum name (or we can combine b and c,
> whatever works better).
> 
> And that's all. And it will support multi-dimensional arrays.
> 
> We then process this array one at a time. Each *step* itself might be
> recursive: finding a field by name in C struct is necessarily
> recursive due to anonymous embedded struct/union fields. But other
> than that, it's a simple sequential operation.
> 
> So unless I'm missing something, let's not add data recursion if it's
> not needed.

Recursive representation I simpler in a sense that you need only one
data type. W/o recursion you need to distinguish between container
data type that links to each constituent.
Plus in a computation function you need to distinguish first step from
all others.  Recursive organization simplifies both data types and
computation function.

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

[Andrii Nakryiko]

On Fri, Jun 13, 2025 at 9:48 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Fri, 2025-06-13 at 09:34 -0700, Andrii Nakryiko wrote:
> > On Wed, Jun 11, 2025 at 5:21 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > >
> > > What do you think about a more recursive representation for presets?
> > > E.g. as follows:
> > >
> > >   struct rvalue {
> > >     long long i; /* use find_enum_value() at parse time to avoid union */
> > >   };
> > >
> > >   struct lvalue {
> > >     enum { VAR, FIELD, ARRAY } type;
> > >     union {
> > >       struct {
> > >         char *name;
> > >       } var;
> > >       struct {
> > >         struct lvalue *base;
> > >         char *name;
> > >       } field;
> > >       struct {
> > >         struct lvalue *base;
> > >         struct rvalue index;
> > >       } array;
> > >     };
> > >   };
> > >
> > >   struct preset {
> > >     struct lvalue *lv;
> > >     struct rvalue rv;
> > >   };
> > >
> > > It can handle matrices ("a[2][3]") and offset/type computation would
> > > be a simple recursive function.
> > >
> >
> > Why do we need recursion, though? All we should need is an array specs
> > of one of the following types:
> >
> >   a) field access by name
> >     a.1) we might want to distinguish array field vs non-array field
> > to better catch unintended user errors
> >   b) indexing by immediate integer
> >   c) indexing by symbolic enum name (or we can combine b and c,
> > whatever works better).
> >
> > And that's all. And it will support multi-dimensional arrays.
> >
> > We then process this array one at a time. Each *step* itself might be
> > recursive: finding a field by name in C struct is necessarily
> > recursive due to anonymous embedded struct/union fields. But other
> > than that, it's a simple sequential operation.
> >
> > So unless I'm missing something, let's not add data recursion if it's
> > not needed.
>
> Recursive representation I simpler in a sense that you need only one
> data type. W/o recursion you need to distinguish between container
> data type that links to each constituent.

So you have this tagged union of three different types some of which
are self-referential (struct lvalue *). Is that what is simpler than
an array of a similarly tagged union, but with no `struct lvalue *`
recursive data pointers? How so?

> Plus in a computation function you need to distinguish first step from
> all others.  Recursive organization simplifies both data types and
> computation function.

Not sure I follow. You have two situations, and it doesn't matter
whether it's a first or not first element. It's either lookup by field
name or indexing using integer or enum. In the latter case we should
have an active "current type" of array kind we are working with (so
yes, we need error checking, but just like with a recursive approach).

Maybe I'm slow, but I don't see how recursivity is making anything
simpler, sorry.

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

[Eduard Zingerman]

On Fri, 2025-06-13 at 09:59 -0700, Andrii Nakryiko wrote:
> On Fri, Jun 13, 2025 at 9:48 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Fri, 2025-06-13 at 09:34 -0700, Andrii Nakryiko wrote:
> > > On Wed, Jun 11, 2025 at 5:21 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > > > 
> > > > What do you think about a more recursive representation for presets?
> > > > E.g. as follows:
> > > > 
> > > >   struct rvalue {
> > > >     long long i; /* use find_enum_value() at parse time to avoid union */
> > > >   };
> > > > 
> > > >   struct lvalue {
> > > >     enum { VAR, FIELD, ARRAY } type;
> > > >     union {
> > > >       struct {
> > > >         char *name;
> > > >       } var;
> > > >       struct {
> > > >         struct lvalue *base;
> > > >         char *name;
> > > >       } field;
> > > >       struct {
> > > >         struct lvalue *base;
> > > >         struct rvalue index;
> > > >       } array;
> > > >     };
> > > >   };
> > > > 
> > > >   struct preset {
> > > >     struct lvalue *lv;
> > > >     struct rvalue rv;
> > > >   };
> > > > 
> > > > It can handle matrices ("a[2][3]") and offset/type computation would
> > > > be a simple recursive function.
> > > > 
> > > 
> > > Why do we need recursion, though? All we should need is an array specs
> > > of one of the following types:
> > > 
> > >   a) field access by name
> > >     a.1) we might want to distinguish array field vs non-array field
> > > to better catch unintended user errors
> > >   b) indexing by immediate integer
> > >   c) indexing by symbolic enum name (or we can combine b and c,
> > > whatever works better).
> > > 
> > > And that's all. And it will support multi-dimensional arrays.
> > > 
> > > We then process this array one at a time. Each *step* itself might be
> > > recursive: finding a field by name in C struct is necessarily
> > > recursive due to anonymous embedded struct/union fields. But other
> > > than that, it's a simple sequential operation.
> > > 
> > > So unless I'm missing something, let's not add data recursion if it's
> > > not needed.
> > 
> > Recursive representation I simpler in a sense that you need only one
> > data type. W/o recursion you need to distinguish between container
> > data type that links to each constituent.
> 
> So you have this tagged union of three different types some of which
> are self-referential (struct lvalue *). Is that what is simpler than
> an array of a similarly tagged union, but with no `struct lvalue *`
> recursive data pointers? How so?

The following:

  struct rvalue {
    long long i;
    const char *enum;
  };

  struct field_access {
    enum { FIELD, ARRAY } type;
    union {
      struct {
        char *name;
      } field;
      struct {
        struct rvalue index;
      } array;
    };
  };

  struct preset {
    struct field_access *atoms;
    int atoms_cnt;
    const char *var;
    struct rvalue rv;
  };

Is logically more complex data structure, because it has two levels
for expression computation: to compute single offset and to drive a
loop for offset accumulation.

> > Plus in a computation function you need to distinguish first step from
> > all others.  Recursive organization simplifies both data types and
> > computation function.
> 
> Not sure I follow. You have two situations, and it doesn't matter
> whether it's a first or not first element. It's either lookup by field
> name or indexing using integer or enum. In the latter case we should
> have an active "current type" of array kind we are working with (so
> yes, we need error checking, but just like with a recursive approach).
> 
> Maybe I'm slow, but I don't see how recursivity is making anything
> simpler, sorry.

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

[Andrii Nakryiko]

On Fri, Jun 13, 2025 at 10:10 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Fri, 2025-06-13 at 09:59 -0700, Andrii Nakryiko wrote:
> > On Fri, Jun 13, 2025 at 9:48 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > >
> > > On Fri, 2025-06-13 at 09:34 -0700, Andrii Nakryiko wrote:
> > > > On Wed, Jun 11, 2025 at 5:21 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > > > >
> > > > > What do you think about a more recursive representation for presets?
> > > > > E.g. as follows:
> > > > >
> > > > >   struct rvalue {
> > > > >     long long i; /* use find_enum_value() at parse time to avoid union */
> > > > >   };
> > > > >
> > > > >   struct lvalue {
> > > > >     enum { VAR, FIELD, ARRAY } type;
> > > > >     union {
> > > > >       struct {
> > > > >         char *name;
> > > > >       } var;
> > > > >       struct {
> > > > >         struct lvalue *base;
> > > > >         char *name;
> > > > >       } field;
> > > > >       struct {
> > > > >         struct lvalue *base;
> > > > >         struct rvalue index;
> > > > >       } array;
> > > > >     };
> > > > >   };
> > > > >
> > > > >   struct preset {
> > > > >     struct lvalue *lv;
> > > > >     struct rvalue rv;
> > > > >   };
> > > > >
> > > > > It can handle matrices ("a[2][3]") and offset/type computation would
> > > > > be a simple recursive function.
> > > > >
> > > >
> > > > Why do we need recursion, though? All we should need is an array specs
> > > > of one of the following types:
> > > >
> > > >   a) field access by name
> > > >     a.1) we might want to distinguish array field vs non-array field
> > > > to better catch unintended user errors
> > > >   b) indexing by immediate integer
> > > >   c) indexing by symbolic enum name (or we can combine b and c,
> > > > whatever works better).
> > > >
> > > > And that's all. And it will support multi-dimensional arrays.
> > > >
> > > > We then process this array one at a time. Each *step* itself might be
> > > > recursive: finding a field by name in C struct is necessarily
> > > > recursive due to anonymous embedded struct/union fields. But other
> > > > than that, it's a simple sequential operation.
> > > >
> > > > So unless I'm missing something, let's not add data recursion if it's
> > > > not needed.
> > >
> > > Recursive representation I simpler in a sense that you need only one
> > > data type. W/o recursion you need to distinguish between container
> > > data type that links to each constituent.
> >
> > So you have this tagged union of three different types some of which
> > are self-referential (struct lvalue *). Is that what is simpler than
> > an array of a similarly tagged union, but with no `struct lvalue *`
> > recursive data pointers? How so?
>
> The following:
>
>   struct rvalue {
>     long long i;
>     const char *enum;
>   };
>
>   struct field_access {
>     enum { FIELD, ARRAY } type;
>     union {
>       struct {
>         char *name;
>       } field;
>       struct {
>         struct rvalue index;
>       } array;
>     };
>   };
>
>   struct preset {
>     struct field_access *atoms;
>     int atoms_cnt;
>     const char *var;
>     struct rvalue rv;
>   };
>
> Is logically more complex data structure, because it has two levels
> for expression computation: to compute single offset and to drive a
> loop for offset accumulation.

Perhaps it's subjective? I find an array of field_access structs
simpler. Maybe because we have a similar approach with resolving CO-RE
relocation spec strings (I think it's in bpf_core_spec_match()).

I do prefer simpler non-recursive data structures.

>
> > > Plus in a computation function you need to distinguish first step from
> > > all others.  Recursive organization simplifies both data types and
> > > computation function.
> >
> > Not sure I follow. You have two situations, and it doesn't matter
> > whether it's a first or not first element. It's either lookup by field
> > name or indexing using integer or enum. In the latter case we should
> > have an active "current type" of array kind we are working with (so
> > yes, we need error checking, but just like with a recursive approach).
> >
> > Maybe I'm slow, but I don't see how recursivity is making anything
> > simpler, sorry.