Re: [PATCH bpf-next v2 1/2] selftests/bpf: implement setting global variables in veristat

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

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.