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

[PATCH bpf-next v4 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"
```

v3 -> v4
 * Rework implementation to support multi dimensional arrays
 * Rework data structures for storing parsed presets

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  | 127 ++++++-
 .../selftests/bpf/progs/set_global_vars.c     |  56 ++-
 tools/testing/selftests/bpf/veristat.c        | 330 ++++++++++++++----
 3 files changed, 414 insertions(+), 99 deletions(-)

-- 
2.49.0

[PATCH bpf-next v4 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 rvalue struct, storing either int or enum (string value),
will be reused in the next patch, extract parsing rvalue into a
separate function.

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

diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index 56771650ee8c..483442c08ecf 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -156,13 +156,23 @@ struct filter {
 	bool abs;
 };
 
-struct var_preset {
-	char *name;
+struct rvalue {
 	enum { INTEGRAL, ENUMERATOR } type;
 	union {
 		long long ivalue;
 		char *svalue;
 	};
+};
+
+struct field_access {
+	char *name;
+};
+
+struct var_preset {
+	struct field_access *atoms;
+	int atom_count;
+	char *full_name;
+	struct rvalue value;
 	bool applied;
 };
 
@@ -1497,6 +1507,35 @@ static int reset_stat_cgroup(void)
 	return 0;
 }
 
+static int parse_rvalue(const char *val, struct rvalue *rvalue)
+{
+	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;
+		}
+		rvalue->ivalue = value;
+		rvalue->type = INTEGRAL;
+	} else {
+		/* if not a number, consider it enum value */
+		rvalue->svalue = strdup(val);
+		if (!rvalue->svalue)
+			return -ENOMEM;
+		rvalue->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));
@@ -1592,13 +1631,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 field_access *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)
@@ -1613,32 +1675,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_rvalue(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;
 }
 
@@ -1765,22 +1813,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].name,
+				      &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);
@@ -1798,7 +1844,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));
@@ -1812,17 +1858,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;
 		}
 	}
@@ -1889,20 +1936,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) {
@@ -1912,7 +1955,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;
 
@@ -1927,7 +1970,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;
 	}
@@ -3061,7 +3104,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;
@@ -3120,9 +3163,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 v4 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 | 226 ++++++++++++++++++++-----
 1 file changed, 180 insertions(+), 46 deletions(-)

diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
index 483442c08ecf..9942adbda411 100644
--- a/tools/testing/selftests/bpf/veristat.c
+++ b/tools/testing/selftests/bpf/veristat.c
@@ -165,7 +165,11 @@ struct rvalue {
 };
 
 struct field_access {
-	char *name;
+	enum { FIELD_NAME, ARRAY_INDEX } type;
+	union {
+		char *name;
+		struct rvalue index;
+	};
 };
 
 struct var_preset {
@@ -1629,28 +1633,60 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
 		free(buf);
 
 	return 0;
-};
+}
+
+static int append_preset_atom(struct var_preset *preset, char *value, bool is_index)
+{
+	struct field_access *tmp;
+	int i = preset->atom_count;
+	int err;
+
+	tmp = reallocarray(preset->atoms, i + 1, sizeof(*preset->atoms));
+	if (!tmp)
+		return -ENOMEM;
+
+	preset->atoms = tmp;
+	preset->atom_count++;
+
+	if (is_index) {
+		preset->atoms[i].type = ARRAY_INDEX;
+		err = parse_rvalue(value, &preset->atoms[i].index);
+		if (err)
+			return err;
+	} else {
+		preset->atoms[i].type = FIELD_NAME;
+		preset->atoms[i].name = strdup(value);
+		if (!preset->atoms[i].name)
+			return -ENOMEM;
+	}
+	return 0;
+}
 
 static int parse_var_atoms(const char *full_var, struct var_preset *preset)
 {
-	char expr[256], *name, *saveptr;
+	char expr[256], var[256], *name, *saveptr;
+	int n, len, off;
 
 	snprintf(expr, sizeof(expr), "%s", full_var);
 	preset->atom_count = 0;
 	while ((name = strtok_r(preset->atom_count ? NULL : expr, ".", &saveptr))) {
-		struct field_access *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++;
+		len = strlen(name);
+		/* parse variable name */
+		if (sscanf(name, "%[a-zA-Z0-9_] %n", var, &off) != 1) {
+			fprintf(stderr, "Can't parse %s\n", name);
+			return -EINVAL;
+		}
+		append_preset_atom(preset, var, false);
 
-		preset->atoms[i].name = strdup(name);
-		if (!preset->atoms[i].name)
-			return -ENOMEM;
+		/* parse optional array indexes */
+		while (off < len) {
+			if (sscanf(name + off, "[%[a-zA-Z0-9_]] %n", var, &n) != 1) {
+				fprintf(stderr, "Can't parse %s as index\n", name + off);
+				return -EINVAL;
+			}
+			append_preset_atom(preset, var, true);
+			off += n;
+		}
 	}
 	return 0;
 }
@@ -1670,7 +1706,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;
 	}
@@ -1763,17 +1799,103 @@ 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 resolve_rvalue(struct btf *btf, const struct rvalue *rvalue, long long *result)
+{
+	int err = 0;
+
+	switch (rvalue->type) {
+	case INTEGRAL:
+		*result = rvalue->ivalue;
+		break;
+	case ENUMERATOR:
+		err = find_enum_value(btf, rvalue->svalue, result);
+		if (err)
+			fprintf(stderr, "Can't resolve enum value %s\n", rvalue->svalue);
+		break;
+	}
+	return err;
+}
+
+/* Returns number of consumed atoms from preset, negative error if failed */
+static int adjust_var_secinfo_array(struct btf *btf, int tid, struct var_preset *preset,
+				    int atom_idx, struct btf_var_secinfo *sinfo)
+{
+	struct btf_array *barr;
+	int i = atom_idx, err;
+	const struct btf_type *t;
+	long long off = 0, idx;
+
+	if (atom_idx < 1) /* Array index can't be the first atom */
+		return -EINVAL;
+
+	tid = btf__resolve_type(btf, tid);
+	t = btf__type_by_id(btf, tid);
+	if (!btf_is_array(t)) {
+		fprintf(stderr, "Array index is not expected for %s\n",
+			preset->atoms[atom_idx - 1].name);
+		return -EINVAL;
+	}
+	do {
+		if (preset->atoms[i].type != ARRAY_INDEX) {
+			fprintf(stderr, "Array index is missing for %s\n",
+				preset->atoms[atom_idx - 1].name);
+			return -EINVAL;
+		}
+		err = resolve_rvalue(btf, &preset->atoms[i].index, &idx);
+		if (err)
+			return err;
+		barr = btf_array(t);
+		if (idx < 0 || idx >= barr->nelems) {
+			fprintf(stderr, "Array index %lld is out of bounds [0, %u]: %s\n",
+				idx, barr->nelems, preset->full_name);
+			return -EINVAL;
+		}
+		off *= barr->nelems;
+		off += idx;
+		tid = btf__resolve_type(btf, barr->type);
+		t = btf__type_by_id(btf, tid);
+		i++;
+	} while (btf_is_array(t));
+
+	sinfo->size = t->size;
+	sinfo->type = tid;
+	sinfo->offset += off * t->size;
+	return i - atom_idx;
+}
+
 const int btf_find_member(const struct btf *btf,
 			  const struct btf_type *parent_type,
 			  __u32 parent_offset,
 			  const char *member_name,
-			  int *member_tid,
-			  __u32 *member_offset)
+			  struct btf_var_secinfo *sinfo)
 {
 	int i;
 
-	if (!btf_is_composite(parent_type))
+	if (!btf_is_composite(parent_type)) {
+		fprintf(stderr, "Can't resolve field %s for non-composite type\n", member_name);
 		return -EINVAL;
+	}
 
 	for (i = 0; i < btf_vlen(parent_type); ++i) {
 		const struct btf_member *member;
@@ -1795,15 +1917,16 @@ const int btf_find_member(const struct btf *btf,
 						name);
 					return -EINVAL;
 				}
-				*member_offset = parent_offset + member->offset;
-				*member_tid = tid;
+				sinfo->offset += (parent_offset + member->offset) / 8;
+				sinfo->type = tid;
+				sinfo->size = member_type->size;
 				return 0;
 			}
 		} else if (btf_is_composite(member_type)) {
 			int err;
 
 			err = btf_find_member(btf, member_type, parent_offset + member->offset,
-					      member_name, member_tid, member_offset);
+					      member_name, sinfo);
 			if (!err)
 				return 0;
 		}
@@ -1815,26 +1938,29 @@ 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, struct var_preset *preset)
 {
-	const struct btf_type *base_type, *member_type;
-	int err, member_tid, i;
-	__u32 member_offset = 0;
-
-	base_type = btf__type_by_id(btf, btf__resolve_type(btf, t->type));
-
-	for (i = 1; i < preset->atom_count; ++i) {
-		err = btf_find_member(btf, base_type, 0, preset->atoms[i].name,
-				      &member_tid, &member_offset);
-		if (err) {
-			fprintf(stderr, "Could not find member %s for variable %s\n",
-				preset->atoms[i].name, preset->atoms[i - 1].name);
-			return err;
+	const struct btf_type *base_type;
+	int err, i = 1, n;
+	int tid;
+
+	tid = btf__resolve_type(btf, t->type);
+	base_type = btf__type_by_id(btf, tid);
+
+	while (i < preset->atom_count) {
+		if (preset->atoms[i].type == ARRAY_INDEX) {
+			n = adjust_var_secinfo_array(btf, tid, preset, i, sinfo);
+			if (n < 0)
+				return n;
+			i += n;
+		} else {
+			err = btf_find_member(btf, base_type, 0, preset->atoms[i].name, sinfo);
+			if (err)
+				return err;
+			i++;
 		}
-		member_type = btf__type_by_id(btf, member_tid);
-		sinfo->offset += member_offset / 8;
-		sinfo->size = member_type->size;
-		sinfo->type = member_tid;
-		base_type = member_type;
+		base_type = btf__type_by_id(btf, sinfo->type);
+		tid = sinfo->type;
 	}
+
 	return 0;
 }
 
@@ -1853,8 +1979,8 @@ static int set_global_var(struct bpf_object *obj, struct btf *btf,
 		return -EINVAL;
 	}
 	if (!is_preset_supported(base_type)) {
-		fprintf(stderr, "Setting value for type %s is not supported\n",
-			btf__name_by_offset(btf, base_type->name_off));
+		fprintf(stderr, "Can't set %s. Only ints and enums are supported\n",
+			preset->full_name);
 		return -EINVAL;
 	}
 
@@ -1971,6 +2097,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;
 	}
@@ -3164,11 +3291,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;
+			}
+		}
 		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 v4 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  | 127 +++++++++++++++++-
 .../selftests/bpf/progs/set_global_vars.c     |  56 +++++---
 2 files changed, 159 insertions(+), 24 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..f4de22302083 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_veristat.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_veristat.c
@@ -60,13 +60,19 @@ 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][2].u.var_u8[2]=170\" "\
 	    " -G \"union1.struct3.var_u8_l = 0xaa\" "\
 	    " -G \"union1.struct3.var_u8_h = 0xaa\" "\
-	    "-vl2 > %s", fix->veristat, fix->tmpfile);
+	    " -G \"arr[3]= 171\" "	\
+	    " -G \"arr[EA2] =172\" "	\
+	    " -G \"enum_arr[EC2]=EA3\" " \
+	    " -G \"three_d[31][7][EA2]=173\"" \
+	    " -G \"struct1[2].struct2[1][2].u.mat[5][3]=174\" " \
+	    " -G \"struct11[7][5].struct2[0][1].u.mat[3][0] = 175\" " \
+	    " -vl2 > %s", fix->veristat, fix->tmpfile);
 
 	read(fix->fd, fix->output, fix->sz);
 	__CHECK_STR("_w=0xf000000000000001 ", "var_s64 = 0xf000000000000001");
@@ -81,8 +87,14 @@ 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[2].struct2[1][2].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");
+	__CHECK_STR("_w=173 ", "matrix[31][7][11]=173");
+	__CHECK_STR("_w=174 ", "struct1[2].struct2[1][2].u.mat[5][3]=174");
+	__CHECK_STR("_w=175 ", "struct11[7][5].struct2[0][1].u.mat[3][0]=175");
 
 out:
 	teardown_fixture(fix);
@@ -129,6 +141,95 @@ static void test_set_global_vars_out_of_range(void)
 	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("Can't set ptr_arr[0]. Only ints and enums are supported", "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("Array index 99 is 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 enum value EG2", "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("Can't set arr. Only ints and enums are supported", "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("Can't resolve field u for non-composite type", "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 +240,22 @@ 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_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..ebaef28b2cb3 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 three_d[47][19][17];
+const volatile i32 *ptr_arr[32];
 
 struct Struct {
 	int:16;
@@ -35,34 +43,38 @@ 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;
+				s32 mat[7][5];
 			} u;
 		};
-	} struct2;
+	} struct2[2][4];
 };
 
 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];
+const volatile struct Struct struct11[11][7];
 
-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 +93,14 @@ 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][2].u.var_u8[2];
 	a = union1.var_u16;
+	a = arr[3];
+	a = arr[EA2];
+	a = enum_arr[EC2];
+	a = three_d[31][7][EA2];
+	a = struct1[2].struct2[1][2].u.mat[5][3];
+	a = struct11[7][5].struct2[0][1].u.mat[3][0];
 
 	return a;
 }
-- 
2.49.0

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

[Eduard Zingerman]

On Wed, 2025-06-18 at 21:39 +0100, Mykyta Yatsenko wrote:
> 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 rvalue struct, storing either int or enum (string value),
> will be reused in the next patch, extract parsing rvalue into a
> separate function.
> 
> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> ---

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

[...]

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

[Eduard Zingerman]

On Wed, 2025-06-18 at 21:39 +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>
> ---
>  tools/testing/selftests/bpf/veristat.c | 226 ++++++++++++++++++++-----
>  1 file changed, 180 insertions(+), 46 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
> index 483442c08ecf..9942adbda411 100644
> --- a/tools/testing/selftests/bpf/veristat.c
> +++ b/tools/testing/selftests/bpf/veristat.c

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

[...]

> @@ -1670,7 +1706,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)) {

Out of curiosity, won't match if the pattern would remain "%s = %s %n"?

>  		fprintf(stderr, "Failed to parse expression '%s'\n", expr);
>  		return -EINVAL;
>  	}
> @@ -1763,17 +1799,103 @@ 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;
> +}
> +

[...]

> @@ -1815,26 +1938,29 @@ 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, struct var_preset *preset)
>  {
> -	const struct btf_type *base_type, *member_type;
> -	int err, member_tid, i;
> -	__u32 member_offset = 0;
> -
> -	base_type = btf__type_by_id(btf, btf__resolve_type(btf, t->type));
> -
> -	for (i = 1; i < preset->atom_count; ++i) {
> -		err = btf_find_member(btf, base_type, 0, preset->atoms[i].name,
> -				      &member_tid, &member_offset);
> -		if (err) {
> -			fprintf(stderr, "Could not find member %s for variable %s\n",
> -				preset->atoms[i].name, preset->atoms[i - 1].name);
> -			return err;
> +	const struct btf_type *base_type;
> +	int err, i = 1, n;
> +	int tid;
> +
> +	tid = btf__resolve_type(btf, t->type);
> +	base_type = btf__type_by_id(btf, tid);
> +
> +	while (i < preset->atom_count) {
> +		if (preset->atoms[i].type == ARRAY_INDEX) {
> +			n = adjust_var_secinfo_array(btf, tid, preset, i, sinfo);
> +			if (n < 0)
> +				return n;
> +			i += n;

Having a nested loop to consume all indices looks annoying.
On the other hand, there is not much one can do w/o some kind of
btf__type_physical_size.

> +		} else {
> +			err = btf_find_member(btf, base_type, 0, preset->atoms[i].name, sinfo);
> +			if (err)
> +				return err;
> +			i++;
>  		}
> -		member_type = btf__type_by_id(btf, member_tid);
> -		sinfo->offset += member_offset / 8;
> -		sinfo->size = member_type->size;
> -		sinfo->type = member_tid;
> -		base_type = member_type;
> +		base_type = btf__type_by_id(btf, sinfo->type);
> +		tid = sinfo->type;
>  	}
> +
>  	return 0;
>  }
>  

[...]

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

[Mykyta Yatsenko]

On 6/19/25 08:34, Eduard Zingerman wrote:
> On Wed, 2025-06-18 at 21:39 +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>
>> ---
>>   tools/testing/selftests/bpf/veristat.c | 226 ++++++++++++++++++++-----
>>   1 file changed, 180 insertions(+), 46 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c
>> index 483442c08ecf..9942adbda411 100644
>> --- a/tools/testing/selftests/bpf/veristat.c
>> +++ b/tools/testing/selftests/bpf/veristat.c
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
>
> [...]
>
>> @@ -1670,7 +1706,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)) {
> Out of curiosity, won't match if the pattern would remain "%s = %s %n"?
"foo=1" won't be parsed correctly, as the entire string will be consumed 
by the first %s.
>
>>   		fprintf(stderr, "Failed to parse expression '%s'\n", expr);
>>   		return -EINVAL;
>>   	}
>> @@ -1763,17 +1799,103 @@ 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;
>> +}
>> +
> [...]
>
>> @@ -1815,26 +1938,29 @@ 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, struct var_preset *preset)
>>   {
>> -	const struct btf_type *base_type, *member_type;
>> -	int err, member_tid, i;
>> -	__u32 member_offset = 0;
>> -
>> -	base_type = btf__type_by_id(btf, btf__resolve_type(btf, t->type));
>> -
>> -	for (i = 1; i < preset->atom_count; ++i) {
>> -		err = btf_find_member(btf, base_type, 0, preset->atoms[i].name,
>> -				      &member_tid, &member_offset);
>> -		if (err) {
>> -			fprintf(stderr, "Could not find member %s for variable %s\n",
>> -				preset->atoms[i].name, preset->atoms[i - 1].name);
>> -			return err;
>> +	const struct btf_type *base_type;
>> +	int err, i = 1, n;
>> +	int tid;
>> +
>> +	tid = btf__resolve_type(btf, t->type);
>> +	base_type = btf__type_by_id(btf, tid);
>> +
>> +	while (i < preset->atom_count) {
>> +		if (preset->atoms[i].type == ARRAY_INDEX) {
>> +			n = adjust_var_secinfo_array(btf, tid, preset, i, sinfo);
>> +			if (n < 0)
>> +				return n;
>> +			i += n;
> Having a nested loop to consume all indices looks annoying.
> On the other hand, there is not much one can do w/o some kind of
> btf__type_physical_size.
>
>> +		} else {
>> +			err = btf_find_member(btf, base_type, 0, preset->atoms[i].name, sinfo);
>> +			if (err)
>> +				return err;
>> +			i++;
>>   		}
>> -		member_type = btf__type_by_id(btf, member_tid);
>> -		sinfo->offset += member_offset / 8;
>> -		sinfo->size = member_type->size;
>> -		sinfo->type = member_tid;
>> -		base_type = member_type;
>> +		base_type = btf__type_by_id(btf, sinfo->type);
>> +		tid = sinfo->type;
>>   	}
>> +
>>   	return 0;
>>   }
>>   
> [...]
>

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

[Eduard Zingerman]

On Wed, 2025-06-18 at 21:39 +0100, Mykyta Yatsenko wrote:
> 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>
> ---

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

>  .../selftests/bpf/prog_tests/test_veristat.c  | 127 +++++++++++++++++-
>  .../selftests/bpf/progs/set_global_vars.c     |  56 +++++---
>  2 files changed, 159 insertions(+), 24 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..f4de22302083 100644
> --- a/tools/testing/selftests/bpf/prog_tests/test_veristat.c
> +++ b/tools/testing/selftests/bpf/prog_tests/test_veristat.c
> @@ -60,13 +60,19 @@ 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\" "\

Nit: white space is allowed for '=' but is not allowed for array
     indexing, e.g. 'a[ 2]=1' or 'a [2]'.

>  	    " -G \"var_b = 1\" "\
> -	    " -G \"struct1.struct2.u.var_u8 = 170\" "\
> +	    " -G \"struct1[2].struct2[1][2].u.var_u8[2]=170\" "\
>  	    " -G \"union1.struct3.var_u8_l = 0xaa\" "\
>  	    " -G \"union1.struct3.var_u8_h = 0xaa\" "\
> -	    "-vl2 > %s", fix->veristat, fix->tmpfile);
> +	    " -G \"arr[3]= 171\" "	\
> +	    " -G \"arr[EA2] =172\" "	\
> +	    " -G \"enum_arr[EC2]=EA3\" " \
> +	    " -G \"three_d[31][7][EA2]=173\"" \
> +	    " -G \"struct1[2].struct2[1][2].u.mat[5][3]=174\" " \
> +	    " -G \"struct11[7][5].struct2[0][1].u.mat[3][0] = 175\" " \
> +	    " -vl2 > %s", fix->veristat, fix->tmpfile);
>  
>  	read(fix->fd, fix->output, fix->sz);
>  	__CHECK_STR("_w=0xf000000000000001 ", "var_s64 = 0xf000000000000001");

[...]

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

[Andrii Nakryiko]

On Wed, Jun 18, 2025 at 1:39 PM Mykyta Yatsenko
<mykyta.yatsenko5@gmail.com> 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>
> ---
>  tools/testing/selftests/bpf/veristat.c | 226 ++++++++++++++++++++-----
>  1 file changed, 180 insertions(+), 46 deletions(-)
>

[...]

> +static int resolve_rvalue(struct btf *btf, const struct rvalue *rvalue, long long *result)
> +{
> +       int err = 0;
> +
> +       switch (rvalue->type) {
> +       case INTEGRAL:
> +               *result = rvalue->ivalue;

return 0;

> +               break;
> +       case ENUMERATOR:
> +               err = find_enum_value(btf, rvalue->svalue, result);
> +               if (err)
> +                       fprintf(stderr, "Can't resolve enum value %s\n", rvalue->svalue);

if (err) {
    fprintf(...);
    return err;
}

return 0;

?

> +               break;

default: fprintf("unknown blah"); return -EOPNOTSUPP;


I think I had a similar argument with Eduard before, so I'll explain
my logic here again. Whenever you have some branching in your code and
you know that branch's processing is effectively done and the only
thing left is to return success/failure signal, *do return early* and
explicitly ASAP (unless there is non-trivial clean up for error path,
in which case not duplicating and spreading clean up logic outweighs
the simplicity of early return code). Otherwise it takes *unnecessary*
extra mental effort to trace through the rest of the code to make sure
there is no extra common post-processing logic after that
branch/switch/for loop.

So if we know the INTEGRAL case is a success, then have `return 0;`
right there, don't make anyone read through the rest of the function
just to make sure we don't do anything extra.

> +       }
> +       return err;
> +}
> +
> +/* Returns number of consumed atoms from preset, negative error if failed */
> +static int adjust_var_secinfo_array(struct btf *btf, int tid, struct var_preset *preset,
> +                                   int atom_idx, struct btf_var_secinfo *sinfo)
> +{
> +       struct btf_array *barr;
> +       int i = atom_idx, err;
> +       const struct btf_type *t;
> +       long long off = 0, idx;
> +
> +       if (atom_idx < 1) /* Array index can't be the first atom */

can atom_idx be -1 or negative? If not, then do `if (atom_idx == 0)`.
It's another small mental overhead that we can easily avoid, and so we
should.

> +               return -EINVAL;
> +
> +       tid = btf__resolve_type(btf, tid);
> +       t = btf__type_by_id(btf, tid);
> +       if (!btf_is_array(t)) {
> +               fprintf(stderr, "Array index is not expected for %s\n",
> +                       preset->atoms[atom_idx - 1].name);
> +               return -EINVAL;
> +       }

[...]

> @@ -1815,26 +1938,29 @@ 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, struct var_preset *preset)
>  {
> -       const struct btf_type *base_type, *member_type;
> -       int err, member_tid, i;
> -       __u32 member_offset = 0;
> -
> -       base_type = btf__type_by_id(btf, btf__resolve_type(btf, t->type));
> -
> -       for (i = 1; i < preset->atom_count; ++i) {
> -               err = btf_find_member(btf, base_type, 0, preset->atoms[i].name,
> -                                     &member_tid, &member_offset);
> -               if (err) {
> -                       fprintf(stderr, "Could not find member %s for variable %s\n",
> -                               preset->atoms[i].name, preset->atoms[i - 1].name);
> -                       return err;
> +       const struct btf_type *base_type;
> +       int err, i = 1, n;
> +       int tid;
> +
> +       tid = btf__resolve_type(btf, t->type);
> +       base_type = btf__type_by_id(btf, tid);
> +
> +       while (i < preset->atom_count) {
> +               if (preset->atoms[i].type == ARRAY_INDEX) {
> +                       n = adjust_var_secinfo_array(btf, tid, preset, i, sinfo);
> +                       if (n < 0)
> +                               return n;
> +                       i += n;
> +               } else {
> +                       err = btf_find_member(btf, base_type, 0, preset->atoms[i].name, sinfo);
> +                       if (err)
> +                               return err;
> +                       i++;
>                 }
> -               member_type = btf__type_by_id(btf, member_tid);
> -               sinfo->offset += member_offset / 8;
> -               sinfo->size = member_type->size;
> -               sinfo->type = member_tid;
> -               base_type = member_type;
> +               base_type = btf__type_by_id(btf, sinfo->type);
> +               tid = sinfo->type;
>         }
> +
>         return 0;
>  }

Is there a good reason to have adjust_var_secinfo() separate from
adjust_var_secinfo_array(). I won't know if I didn't miss anything
non-obvious, but in my mind this whole adjust_var_sec_info() should
look roughly like this:

cur_type = /* resolve from original var */
cur_off = 0;

for (i = 0; i < preset->atom_count; i++) {
    if (preset->atoms[i].type == ARRAY_INDEX) {
        /* a) error checking: cur_type should be array */
        /* b) resolve index (if it's enum)
        /* c) error checking: index should be within bounds of
cur_type (which is ARRAY)
        /* d) adjust cur_off += cur_type's elem_size * index_value
        /* e) cur_type = btf__resolve_type(cur_type->type) */
    } else {
        /* a) error checking: cur_type should be struct/union */
        /* b) find field by name with btf_find_member */
        /* c) cur_off += member_offset */
        /* d) cur_type = btf__resolve_type(field->type) */
    }
}

It seems inelegant that we have an outer loop over FIELD references
(one at a time), but for ARRAY_INDEX we do N items skipping. Why? We
have a set of "instructions", just execute them one at a time, and
keep track of the current type and current offset we are at.

Is there anything I am missing that would prevent this simple and more
uniform approach?

[...]

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

[Mykyta Yatsenko]

On 6/23/25 23:10, Andrii Nakryiko wrote:
> On Wed, Jun 18, 2025 at 1:39 PM Mykyta Yatsenko
> <mykyta.yatsenko5@gmail.com> 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>
>> ---
>>   tools/testing/selftests/bpf/veristat.c | 226 ++++++++++++++++++++-----
>>   1 file changed, 180 insertions(+), 46 deletions(-)
>>
> [...]
>
>> +static int resolve_rvalue(struct btf *btf, const struct rvalue *rvalue, long long *result)
>> +{
>> +       int err = 0;
>> +
>> +       switch (rvalue->type) {
>> +       case INTEGRAL:
>> +               *result = rvalue->ivalue;
> return 0;
>
>> +               break;
>> +       case ENUMERATOR:
>> +               err = find_enum_value(btf, rvalue->svalue, result);
>> +               if (err)
>> +                       fprintf(stderr, "Can't resolve enum value %s\n", rvalue->svalue);
> if (err) {
>      fprintf(...);
>      return err;
> }
>
> return 0;
>
> ?
>
>> +               break;
> default: fprintf("unknown blah"); return -EOPNOTSUPP;
>
>
> I think I had a similar argument with Eduard before, so I'll explain
> my logic here again. Whenever you have some branching in your code and
> you know that branch's processing is effectively done and the only
> thing left is to return success/failure signal, *do return early* and
> explicitly ASAP (unless there is non-trivial clean up for error path,
> in which case not duplicating and spreading clean up logic outweighs
> the simplicity of early return code). Otherwise it takes *unnecessary*
> extra mental effort to trace through the rest of the code to make sure
> there is no extra common post-processing logic after that
> branch/switch/for loop.
>
> So if we know the INTEGRAL case is a success, then have `return 0;`
> right there, don't make anyone read through the rest of the function
> just to make sure we don't do anything extra.
I understand early returns, just in this case ditched it to make the 
code more compact.
I'll change this.
>> +       }
>> +       return err;
>> +}
>> +
>> +/* Returns number of consumed atoms from preset, negative error if failed */
>> +static int adjust_var_secinfo_array(struct btf *btf, int tid, struct var_preset *preset,
>> +                                   int atom_idx, struct btf_var_secinfo *sinfo)
>> +{
>> +       struct btf_array *barr;
>> +       int i = atom_idx, err;
>> +       const struct btf_type *t;
>> +       long long off = 0, idx;
>> +
>> +       if (atom_idx < 1) /* Array index can't be the first atom */
> can atom_idx be -1 or negative? If not, then do `if (atom_idx == 0)`.
> It's another small mental overhead that we can easily avoid, and so we
> should.
sure
>
>> +               return -EINVAL;
>> +
>> +       tid = btf__resolve_type(btf, tid);
>> +       t = btf__type_by_id(btf, tid);
>> +       if (!btf_is_array(t)) {
>> +               fprintf(stderr, "Array index is not expected for %s\n",
>> +                       preset->atoms[atom_idx - 1].name);
>> +               return -EINVAL;
>> +       }
> [...]
>
>> @@ -1815,26 +1938,29 @@ 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, struct var_preset *preset)
>>   {
>> -       const struct btf_type *base_type, *member_type;
>> -       int err, member_tid, i;
>> -       __u32 member_offset = 0;
>> -
>> -       base_type = btf__type_by_id(btf, btf__resolve_type(btf, t->type));
>> -
>> -       for (i = 1; i < preset->atom_count; ++i) {
>> -               err = btf_find_member(btf, base_type, 0, preset->atoms[i].name,
>> -                                     &member_tid, &member_offset);
>> -               if (err) {
>> -                       fprintf(stderr, "Could not find member %s for variable %s\n",
>> -                               preset->atoms[i].name, preset->atoms[i - 1].name);
>> -                       return err;
>> +       const struct btf_type *base_type;
>> +       int err, i = 1, n;
>> +       int tid;
>> +
>> +       tid = btf__resolve_type(btf, t->type);
>> +       base_type = btf__type_by_id(btf, tid);
>> +
>> +       while (i < preset->atom_count) {
>> +               if (preset->atoms[i].type == ARRAY_INDEX) {
>> +                       n = adjust_var_secinfo_array(btf, tid, preset, i, sinfo);
>> +                       if (n < 0)
>> +                               return n;
>> +                       i += n;
>> +               } else {
>> +                       err = btf_find_member(btf, base_type, 0, preset->atoms[i].name, sinfo);
>> +                       if (err)
>> +                               return err;
>> +                       i++;
>>                  }
>> -               member_type = btf__type_by_id(btf, member_tid);
>> -               sinfo->offset += member_offset / 8;
>> -               sinfo->size = member_type->size;
>> -               sinfo->type = member_tid;
>> -               base_type = member_type;
>> +               base_type = btf__type_by_id(btf, sinfo->type);
>> +               tid = sinfo->type;
>>          }
>> +
>>          return 0;
>>   }
> Is there a good reason to have adjust_var_secinfo() separate from
> adjust_var_secinfo_array(). I won't know if I didn't miss anything
> non-obvious, but in my mind this whole adjust_var_sec_info() should
> look roughly like this:
>
> cur_type = /* resolve from original var */
> cur_off = 0;
>
> for (i = 0; i < preset->atom_count; i++) {
>      if (preset->atoms[i].type == ARRAY_INDEX) {
>          /* a) error checking: cur_type should be array */
>          /* b) resolve index (if it's enum)
>          /* c) error checking: index should be within bounds of
> cur_type (which is ARRAY)
>          /* d) adjust cur_off += cur_type's elem_size * index_value
>          /* e) cur_type = btf__resolve_type(cur_type->type) */
>      } else {
>          /* a) error checking: cur_type should be struct/union */
>          /* b) find field by name with btf_find_member */
>          /* c) cur_off += member_offset */
>          /* d) cur_type = btf__resolve_type(field->type) */
>      }
> }
>
> It seems inelegant that we have an outer loop over FIELD references
> (one at a time), but for ARRAY_INDEX we do N items skipping. Why? We
> have a set of "instructions", just execute them one at a time, and
> keep track of the current type and current offset we are at.
>
> Is there anything I am missing that would prevent this simple and more
> uniform approach?
yes, I think there is a small detail - `cur_type's elem_size`is not easy 
to get for multi-dim array.
If we have 3 dim array `arr` of NxMxK, to find an index of 
`arr[x][y][z]` we calculate `(x*M + y)*K + z`
This formula is tricky to integrate in the above loop an offset of the 
current element depends on the next
`btf_arr`s. Though, I can see a couple of ways to do it:
1) Look forward for the next array dimensions to multiply with current 
index: `x*M*K +  y*K + z`.
2) Have a separate `array_off` variable, that will be multiplied with 
current dimension and then zeroed when atom is non-array index:
```
   array_off = 0;
   array_off *= N; array_off += x;
   array_off *= M; array_off += y;
   array_off *= K; array_off += z;
...
    off += array_off; array_off = 0;
```
I picked an option 2, but moved it into the separate function to make 
things a little bit simpler.
>
> [...]

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

[Andrii Nakryiko]

On Mon, Jun 23, 2025 at 5:00 PM Mykyta Yatsenko
<mykyta.yatsenko5@gmail.com> wrote:
>
> On 6/23/25 23:10, Andrii Nakryiko wrote:
> > On Wed, Jun 18, 2025 at 1:39 PM Mykyta Yatsenko
> > <mykyta.yatsenko5@gmail.com> 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>
> >> ---
> >>   tools/testing/selftests/bpf/veristat.c | 226 ++++++++++++++++++++-----
> >>   1 file changed, 180 insertions(+), 46 deletions(-)
> >>
> > [...]
> >
> >> +static int resolve_rvalue(struct btf *btf, const struct rvalue *rvalue, long long *result)
> >> +{
> >> +       int err = 0;
> >> +
> >> +       switch (rvalue->type) {
> >> +       case INTEGRAL:
> >> +               *result = rvalue->ivalue;
> > return 0;
> >
> >> +               break;
> >> +       case ENUMERATOR:
> >> +               err = find_enum_value(btf, rvalue->svalue, result);
> >> +               if (err)
> >> +                       fprintf(stderr, "Can't resolve enum value %s\n", rvalue->svalue);
> > if (err) {
> >      fprintf(...);
> >      return err;
> > }
> >
> > return 0;
> >
> > ?
> >
> >> +               break;
> > default: fprintf("unknown blah"); return -EOPNOTSUPP;
> >
> >
> > I think I had a similar argument with Eduard before, so I'll explain
> > my logic here again. Whenever you have some branching in your code and
> > you know that branch's processing is effectively done and the only
> > thing left is to return success/failure signal, *do return early* and
> > explicitly ASAP (unless there is non-trivial clean up for error path,
> > in which case not duplicating and spreading clean up logic outweighs
> > the simplicity of early return code). Otherwise it takes *unnecessary*
> > extra mental effort to trace through the rest of the code to make sure
> > there is no extra common post-processing logic after that
> > branch/switch/for loop.
> >
> > So if we know the INTEGRAL case is a success, then have `return 0;`
> > right there, don't make anyone read through the rest of the function
> > just to make sure we don't do anything extra.
> I understand early returns, just in this case ditched it to make the
> code more compact.
> I'll change this.
> >> +       }
> >> +       return err;
> >> +}
> >> +
> >> +/* Returns number of consumed atoms from preset, negative error if failed */
> >> +static int adjust_var_secinfo_array(struct btf *btf, int tid, struct var_preset *preset,
> >> +                                   int atom_idx, struct btf_var_secinfo *sinfo)
> >> +{
> >> +       struct btf_array *barr;
> >> +       int i = atom_idx, err;
> >> +       const struct btf_type *t;
> >> +       long long off = 0, idx;
> >> +
> >> +       if (atom_idx < 1) /* Array index can't be the first atom */
> > can atom_idx be -1 or negative? If not, then do `if (atom_idx == 0)`.
> > It's another small mental overhead that we can easily avoid, and so we
> > should.
> sure
> >
> >> +               return -EINVAL;
> >> +
> >> +       tid = btf__resolve_type(btf, tid);
> >> +       t = btf__type_by_id(btf, tid);
> >> +       if (!btf_is_array(t)) {
> >> +               fprintf(stderr, "Array index is not expected for %s\n",
> >> +                       preset->atoms[atom_idx - 1].name);
> >> +               return -EINVAL;
> >> +       }
> > [...]
> >
> >> @@ -1815,26 +1938,29 @@ 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, struct var_preset *preset)
> >>   {
> >> -       const struct btf_type *base_type, *member_type;
> >> -       int err, member_tid, i;
> >> -       __u32 member_offset = 0;
> >> -
> >> -       base_type = btf__type_by_id(btf, btf__resolve_type(btf, t->type));
> >> -
> >> -       for (i = 1; i < preset->atom_count; ++i) {
> >> -               err = btf_find_member(btf, base_type, 0, preset->atoms[i].name,
> >> -                                     &member_tid, &member_offset);
> >> -               if (err) {
> >> -                       fprintf(stderr, "Could not find member %s for variable %s\n",
> >> -                               preset->atoms[i].name, preset->atoms[i - 1].name);
> >> -                       return err;
> >> +       const struct btf_type *base_type;
> >> +       int err, i = 1, n;
> >> +       int tid;
> >> +
> >> +       tid = btf__resolve_type(btf, t->type);
> >> +       base_type = btf__type_by_id(btf, tid);
> >> +
> >> +       while (i < preset->atom_count) {
> >> +               if (preset->atoms[i].type == ARRAY_INDEX) {
> >> +                       n = adjust_var_secinfo_array(btf, tid, preset, i, sinfo);
> >> +                       if (n < 0)
> >> +                               return n;
> >> +                       i += n;
> >> +               } else {
> >> +                       err = btf_find_member(btf, base_type, 0, preset->atoms[i].name, sinfo);
> >> +                       if (err)
> >> +                               return err;
> >> +                       i++;
> >>                  }
> >> -               member_type = btf__type_by_id(btf, member_tid);
> >> -               sinfo->offset += member_offset / 8;
> >> -               sinfo->size = member_type->size;
> >> -               sinfo->type = member_tid;
> >> -               base_type = member_type;
> >> +               base_type = btf__type_by_id(btf, sinfo->type);
> >> +               tid = sinfo->type;
> >>          }
> >> +
> >>          return 0;
> >>   }
> > Is there a good reason to have adjust_var_secinfo() separate from
> > adjust_var_secinfo_array(). I won't know if I didn't miss anything
> > non-obvious, but in my mind this whole adjust_var_sec_info() should
> > look roughly like this:
> >
> > cur_type = /* resolve from original var */
> > cur_off = 0;
> >
> > for (i = 0; i < preset->atom_count; i++) {
> >      if (preset->atoms[i].type == ARRAY_INDEX) {
> >          /* a) error checking: cur_type should be array */
> >          /* b) resolve index (if it's enum)
> >          /* c) error checking: index should be within bounds of
> > cur_type (which is ARRAY)
> >          /* d) adjust cur_off += cur_type's elem_size * index_value
> >          /* e) cur_type = btf__resolve_type(cur_type->type) */
> >      } else {
> >          /* a) error checking: cur_type should be struct/union */
> >          /* b) find field by name with btf_find_member */
> >          /* c) cur_off += member_offset */
> >          /* d) cur_type = btf__resolve_type(field->type) */
> >      }
> > }
> >
> > It seems inelegant that we have an outer loop over FIELD references
> > (one at a time), but for ARRAY_INDEX we do N items skipping. Why? We
> > have a set of "instructions", just execute them one at a time, and
> > keep track of the current type and current offset we are at.
> >
> > Is there anything I am missing that would prevent this simple and more
> > uniform approach?
> yes, I think there is a small detail - `cur_type's elem_size`is not easy
> to get for multi-dim array.
> If we have 3 dim array `arr` of NxMxK, to find an index of
> `arr[x][y][z]` we calculate `(x*M + y)*K + z`
> This formula is tricky to integrate in the above loop an offset of the
> current element depends on the next
> `btf_arr`s. Though, I can see a couple of ways to do it:
> 1) Look forward for the next array dimensions to multiply with current
> index: `x*M*K +  y*K + z`.

does it really matter if it's multi-dimenstional array or
single-dimensional? In both cases you calculate the size of on array
element (which for multi-dimensional will be another array with
outermost dimension stripped, and in BTF that's a separate type
referenced by array->type BTF ID), and multiply it by the index.

So let's say you have

struct my_struct {int x, y;};

struct my_struct arr[10];

And trying to calculate arr[5] offset (without yet going inside
my_struct, one step only). You'll add 5 * sizeof(arr[0]) -> 5 * 8 = 40
bytes.

Now, let's say you have

int arr[10][20];

And you are processing the outermost array indexing step in arr[5][7].
Here you'll adjust offset by 5 * sizeof(arr[0]) -> 5 * sizeof(int[20])
-> 5 * 20 * 4 = 80 bytes.

In both cases you can just use btf__resolve_size() on
btf_array(cur_type)->type to get the size of the array's element.

Would that work?

> 2) Have a separate `array_off` variable, that will be multiplied with
> current dimension and then zeroed when atom is non-array index:
> ```
>    array_off = 0;
>    array_off *= N; array_off += x;
>    array_off *= M; array_off += y;
>    array_off *= K; array_off += z;
> ...
>     off += array_off; array_off = 0;
> ```
> I picked an option 2, but moved it into the separate function to make
> things a little bit simpler.
> >
> > [...]
>

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

[Mykyta Yatsenko]

On 6/24/25 16:59, Andrii Nakryiko wrote:
> On Mon, Jun 23, 2025 at 5:00 PM Mykyta Yatsenko
> <mykyta.yatsenko5@gmail.com> wrote:
>> On 6/23/25 23:10, Andrii Nakryiko wrote:
>>> On Wed, Jun 18, 2025 at 1:39 PM Mykyta Yatsenko
>>> <mykyta.yatsenko5@gmail.com> 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>
>>>> ---
>>>>    tools/testing/selftests/bpf/veristat.c | 226 ++++++++++++++++++++-----
>>>>    1 file changed, 180 insertions(+), 46 deletions(-)
>>>>
>>> [...]
>>>
>>>> +static int resolve_rvalue(struct btf *btf, const struct rvalue *rvalue, long long *result)
>>>> +{
>>>> +       int err = 0;
>>>> +
>>>> +       switch (rvalue->type) {
>>>> +       case INTEGRAL:
>>>> +               *result = rvalue->ivalue;
>>> return 0;
>>>
>>>> +               break;
>>>> +       case ENUMERATOR:
>>>> +               err = find_enum_value(btf, rvalue->svalue, result);
>>>> +               if (err)
>>>> +                       fprintf(stderr, "Can't resolve enum value %s\n", rvalue->svalue);
>>> if (err) {
>>>       fprintf(...);
>>>       return err;
>>> }
>>>
>>> return 0;
>>>
>>> ?
>>>
>>>> +               break;
>>> default: fprintf("unknown blah"); return -EOPNOTSUPP;
>>>
>>>
>>> I think I had a similar argument with Eduard before, so I'll explain
>>> my logic here again. Whenever you have some branching in your code and
>>> you know that branch's processing is effectively done and the only
>>> thing left is to return success/failure signal, *do return early* and
>>> explicitly ASAP (unless there is non-trivial clean up for error path,
>>> in which case not duplicating and spreading clean up logic outweighs
>>> the simplicity of early return code). Otherwise it takes *unnecessary*
>>> extra mental effort to trace through the rest of the code to make sure
>>> there is no extra common post-processing logic after that
>>> branch/switch/for loop.
>>>
>>> So if we know the INTEGRAL case is a success, then have `return 0;`
>>> right there, don't make anyone read through the rest of the function
>>> just to make sure we don't do anything extra.
>> I understand early returns, just in this case ditched it to make the
>> code more compact.
>> I'll change this.
>>>> +       }
>>>> +       return err;
>>>> +}
>>>> +
>>>> +/* Returns number of consumed atoms from preset, negative error if failed */
>>>> +static int adjust_var_secinfo_array(struct btf *btf, int tid, struct var_preset *preset,
>>>> +                                   int atom_idx, struct btf_var_secinfo *sinfo)
>>>> +{
>>>> +       struct btf_array *barr;
>>>> +       int i = atom_idx, err;
>>>> +       const struct btf_type *t;
>>>> +       long long off = 0, idx;
>>>> +
>>>> +       if (atom_idx < 1) /* Array index can't be the first atom */
>>> can atom_idx be -1 or negative? If not, then do `if (atom_idx == 0)`.
>>> It's another small mental overhead that we can easily avoid, and so we
>>> should.
>> sure
>>>> +               return -EINVAL;
>>>> +
>>>> +       tid = btf__resolve_type(btf, tid);
>>>> +       t = btf__type_by_id(btf, tid);
>>>> +       if (!btf_is_array(t)) {
>>>> +               fprintf(stderr, "Array index is not expected for %s\n",
>>>> +                       preset->atoms[atom_idx - 1].name);
>>>> +               return -EINVAL;
>>>> +       }
>>> [...]
>>>
>>>> @@ -1815,26 +1938,29 @@ 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, struct var_preset *preset)
>>>>    {
>>>> -       const struct btf_type *base_type, *member_type;
>>>> -       int err, member_tid, i;
>>>> -       __u32 member_offset = 0;
>>>> -
>>>> -       base_type = btf__type_by_id(btf, btf__resolve_type(btf, t->type));
>>>> -
>>>> -       for (i = 1; i < preset->atom_count; ++i) {
>>>> -               err = btf_find_member(btf, base_type, 0, preset->atoms[i].name,
>>>> -                                     &member_tid, &member_offset);
>>>> -               if (err) {
>>>> -                       fprintf(stderr, "Could not find member %s for variable %s\n",
>>>> -                               preset->atoms[i].name, preset->atoms[i - 1].name);
>>>> -                       return err;
>>>> +       const struct btf_type *base_type;
>>>> +       int err, i = 1, n;
>>>> +       int tid;
>>>> +
>>>> +       tid = btf__resolve_type(btf, t->type);
>>>> +       base_type = btf__type_by_id(btf, tid);
>>>> +
>>>> +       while (i < preset->atom_count) {
>>>> +               if (preset->atoms[i].type == ARRAY_INDEX) {
>>>> +                       n = adjust_var_secinfo_array(btf, tid, preset, i, sinfo);
>>>> +                       if (n < 0)
>>>> +                               return n;
>>>> +                       i += n;
>>>> +               } else {
>>>> +                       err = btf_find_member(btf, base_type, 0, preset->atoms[i].name, sinfo);
>>>> +                       if (err)
>>>> +                               return err;
>>>> +                       i++;
>>>>                   }
>>>> -               member_type = btf__type_by_id(btf, member_tid);
>>>> -               sinfo->offset += member_offset / 8;
>>>> -               sinfo->size = member_type->size;
>>>> -               sinfo->type = member_tid;
>>>> -               base_type = member_type;
>>>> +               base_type = btf__type_by_id(btf, sinfo->type);
>>>> +               tid = sinfo->type;
>>>>           }
>>>> +
>>>>           return 0;
>>>>    }
>>> Is there a good reason to have adjust_var_secinfo() separate from
>>> adjust_var_secinfo_array(). I won't know if I didn't miss anything
>>> non-obvious, but in my mind this whole adjust_var_sec_info() should
>>> look roughly like this:
>>>
>>> cur_type = /* resolve from original var */
>>> cur_off = 0;
>>>
>>> for (i = 0; i < preset->atom_count; i++) {
>>>       if (preset->atoms[i].type == ARRAY_INDEX) {
>>>           /* a) error checking: cur_type should be array */
>>>           /* b) resolve index (if it's enum)
>>>           /* c) error checking: index should be within bounds of
>>> cur_type (which is ARRAY)
>>>           /* d) adjust cur_off += cur_type's elem_size * index_value
>>>           /* e) cur_type = btf__resolve_type(cur_type->type) */
>>>       } else {
>>>           /* a) error checking: cur_type should be struct/union */
>>>           /* b) find field by name with btf_find_member */
>>>           /* c) cur_off += member_offset */
>>>           /* d) cur_type = btf__resolve_type(field->type) */
>>>       }
>>> }
>>>
>>> It seems inelegant that we have an outer loop over FIELD references
>>> (one at a time), but for ARRAY_INDEX we do N items skipping. Why? We
>>> have a set of "instructions", just execute them one at a time, and
>>> keep track of the current type and current offset we are at.
>>>
>>> Is there anything I am missing that would prevent this simple and more
>>> uniform approach?
>> yes, I think there is a small detail - `cur_type's elem_size`is not easy
>> to get for multi-dim array.
>> If we have 3 dim array `arr` of NxMxK, to find an index of
>> `arr[x][y][z]` we calculate `(x*M + y)*K + z`
>> This formula is tricky to integrate in the above loop an offset of the
>> current element depends on the next
>> `btf_arr`s. Though, I can see a couple of ways to do it:
>> 1) Look forward for the next array dimensions to multiply with current
>> index: `x*M*K +  y*K + z`.
> does it really matter if it's multi-dimenstional array or
> single-dimensional? In both cases you calculate the size of on array
> element (which for multi-dimensional will be another array with
> outermost dimension stripped, and in BTF that's a separate type
> referenced by array->type BTF ID), and multiply it by the index.
>
> So let's say you have
>
> struct my_struct {int x, y;};
>
> struct my_struct arr[10];
>
> And trying to calculate arr[5] offset (without yet going inside
> my_struct, one step only). You'll add 5 * sizeof(arr[0]) -> 5 * 8 = 40
> bytes.
>
> Now, let's say you have
>
> int arr[10][20];
>
> And you are processing the outermost array indexing step in arr[5][7].
> Here you'll adjust offset by 5 * sizeof(arr[0]) -> 5 * sizeof(int[20])
> -> 5 * 20 * 4 = 80 bytes.
>
> In both cases you can just use btf__resolve_size() on
> btf_array(cur_type)->type to get the size of the array's element.
>
> Would that work?
yeah, `btf__resolve_type_size` will do, did not know it exists.
Eduard mentioned that it would be great to have something like
btf__type_physical_size, apparently we do have it.
>> 2) Have a separate `array_off` variable, that will be multiplied with
>> current dimension and then zeroed when atom is non-array index:
>> ```
>>     array_off = 0;
>>     array_off *= N; array_off += x;
>>     array_off *= M; array_off += y;
>>     array_off *= K; array_off += z;
>> ...
>>      off += array_off; array_off = 0;
>> ```
>> I picked an option 2, but moved it into the separate function to make
>> things a little bit simpler.
>>> [...]