On 11/02/2025 01:24, Eduard Zingerman wrote:
On Mon, 2025-02-10 at 13:51 +0000, Mykyta Yatsenko wrote:
[...]
@@ -363,6 +378,24 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
return -ENOMEM;
env.filename_cnt++;
break;
+ case 'G': {
+ static int presets_cap;
+ char *expr = strdup(arg);
+
+ if (expr[0] == '@') {
+ if (parse_var_presets_from_file(expr + 1, &env.presets,
+ &presets_cap, &env.npresets)) {
Nit: I'd modify 'env' directly in parse_var_presets{,_from_file} and
add presets_cap field to 'env': to avoid static variable and to
avoid '(*presets)[*size].name = ...' below.
+ fprintf(stderr, "Could not parse global variables preset: %s\n",
+ arg);
+ argp_usage(state);
+ }
+ } else if (parse_var_presets(expr, &env.presets, &presets_cap, &env.npresets)) {
+ fprintf(stderr, "Could not parse global variables preset: %s\n", arg);
+ argp_usage(state);
+ }
+ free(expr);
+ break;
+ }
default:
return ARGP_ERR_UNKNOWN;
}
@@ -1292,6 +1325,273 @@ static int process_prog(const char *filename, struct bpf_object *obj, struct bpf
return 0;
};
+static int parse_var_presets(char *expr, struct var_preset **presets, int *capacity, int *size)
+{
+ char *eq_ptr = strchr(expr, '=');
+ char *name_ptr = expr;
+ char *name_end = eq_ptr - 1;
+ char *val_ptr = eq_ptr + 1;
+ long long value;
+
+ if (!eq_ptr) {
+ fprintf(stderr, "No assignment in expression\n");
+ return -EINVAL;
+ }
+
+ while (isspace(*name_ptr))
+ ++name_ptr;
+ while (isspace(*name_end))
+ --name_end;
I think this loop has to be capped by string start check,
otherwise for -G ' = 10' it might read some uninitialized memory.
+ while (isspace(*val_ptr))
+ ++val_ptr;
+
+ if (name_ptr > name_end) {
+ fprintf(stderr, "Empty variable name in expression %s\n", expr);
+ return -EINVAL;
+ }
+
+ if (*size >= *capacity) {
+ *capacity = max(*capacity * 2, 1);
+ *presets = realloc(*presets, *capacity * sizeof(**presets));
Nit: if realloc() fails it returns NULL,
so the pointer to older *presets would be lost and never freed
(but we exit the program in case of an error, so not really an issue).
Also, check for NULL and return -ENOMEM?
+ }
+
+ if (isalpha(*val_ptr)) {
+ char *value_end = val_ptr + strlen(val_ptr) - 1;
+
+ while (isspace(*value_end))
+ --value_end;
+ *(value_end + 1) = '\0';
+
+ (*presets)[*size].svalue = strdup(val_ptr);
Silly question, why strdup here and for .name?
Keeping pointers to argv should be fine as far as I know.
Right, It seems like I don't really need to make this copy. Although,
I'm going to follow up what
Andrii suggested and use sscanf, in that case copy will be needed.
+ (*presets)[*size].type = NAME;
+ } else if (*val_ptr == '-' || isdigit(*val_ptr)) {
+ errno = 0;
+ value = strtoll(val_ptr, NULL, 0);
+ if (errno == ERANGE) {
+ errno = 0;
+ value = strtoull(val_ptr, NULL, 0);
+ }
+ (*presets)[*size].ivalue = value;
+ (*presets)[*size].type = INTEGRAL;
+ if (errno) {
+ fprintf(stderr, "Could not parse integer value %s\n", val_ptr);
+ return -EINVAL;
+ }
+ } else {
+ fprintf(stderr, "Could not parse value %s\n", val_ptr);
+ return -EINVAL;
+ }
+ *(name_end + 1) = '\0';
+ (*presets)[*size].name = strdup(name_ptr);
+ (*size)++;
+ return 0;
+}
+
+static int parse_var_presets_from_file(const char *filename, struct var_preset **presets,
+ int *capacity, int *size)
Thank you for adding this!
+{
+ FILE *f;
+ char line[256];
+ int err = 0;
+
+ f = fopen(filename, "rt");
+ if (!f) {
+ fprintf(stderr, "Could not open file %s\n", filename);
+ return -EINVAL;
+ }
+
+ while (fgets(line, sizeof(line), f)) {
+ int err = parse_var_presets(line, presets, capacity, size);
+
+ if (err)
+ goto cleanup;
+ }
+
+cleanup:
Nit: I'd check for ferror(f) and write something to stderr here.
+ fclose(f);
+ return err;
+}
+
+static bool is_signed_type(const struct btf_type *t)
+{
+ if (btf_is_int(t))
+ return btf_int_encoding(t) & BTF_INT_SIGNED;
+ if (btf_is_enum(t))
+ return btf_kflag(t);
+ return true;
+}
[...]
+static int set_global_vars(struct bpf_object *obj, struct var_preset *presets, int npresets)
+{
+ struct btf_var_secinfo *sinfo;
+ const char *sec_name;
+ const struct btf_type *type;
+ struct bpf_map *map;
+ struct btf *btf;
+ bool *set_var;
+ int i, j, k, n, cnt, err = 0;
+
+ if (npresets == 0)
+ return 0;
+
+ btf = bpf_object__btf(obj);
+ if (!btf)
+ return -EINVAL;
+
+ set_var = calloc(npresets, sizeof(bool));
+ for (i = 0; i < npresets; ++i)
+ set_var[i] = false;
As Andrii writes in a sibling thread, I'd keep this flag in the
'struct var_preset'.
+
+ cnt = btf__type_cnt(btf);
+ for (i = 0; i != cnt; ++i) {
+ type = btf__type_by_id(btf, i);
+
+ if (!btf_is_datasec(type))
+ continue;
+
+ sinfo = btf_var_secinfos(type);
+ sec_name = btf__name_by_offset(btf, type->name_off);
+ map = bpf_object__find_map_by_name(obj, sec_name);
+ if (!map)
+ continue;
+
+ n = btf_vlen(type);
+ for (j = 0; j < n; ++j, ++sinfo) {
+ const struct btf_type *var_type = btf__type_by_id(btf, sinfo->type);
+ const char *var_name = btf__name_by_offset(btf, var_type->name_off);
+
+ if (!btf_is_var(var_type))
+ continue;
+
+ for (k = 0; k < npresets; ++k) {
+ if (strcmp(var_name, presets[k].name) != 0)
+ continue;
+
+ if (set_var[k]) {
+ fprintf(stderr, "Variable %s is set more than once",
+ var_name);
+ }
+
+ err = set_global_var(obj, btf, var_type, map, sinfo, presets + k);
+ if (err)
+ goto out;
+
+ set_var[k] = true;
+ break;
+ }
+ }
+ }
+ for (i = 0; i < npresets; ++i) {
+ if (!set_var[i]) {
+ fprintf(stderr, "Global variable preset %s has not been applied\n",
+ presets[i].name);
+ }
+ }
+out:
+ free(set_var);
+ return err;
+}
+
[...]
Thanks, I'll apply your suggestions in v3.
