Re: [PATCH bpf-next v3 3/7] bpf: create repeated fields for arrays.

[Kui-Feng Lee <sinquersw@gmail.com>]

On 5/2/24 10:20, Eduard Zingerman wrote:
> On Wed, 2024-05-01 at 13:47 -0700, Kui-Feng Lee wrote:
> 
> I think this looks good for repeating fields of nested arrays
> (w/o visiting nested structures), two nits below.
> 
>> @@ -3575,6 +3628,19 @@ static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
>>   	for_each_vsi(i, t, vsi) {
>>   		const struct btf_type *var = btf_type_by_id(btf, vsi->type);
>>   		const struct btf_type *var_type = btf_type_by_id(btf, var->type);
>> +		const struct btf_array *array;
>> +		u32 j, nelems = 1;
>> +
>> +		/* Walk into array types to find the element type and the
>> +		 * number of elements in the (flattened) array.
>> +		 */
>> +		for (j = 0; j < MAX_RESOLVE_DEPTH && btf_type_is_array(var_type); j++) {
>> +			array = btf_array(var_type);
>> +			nelems *= array->nelems;
>> +			var_type = btf_type_by_id(btf, array->type);
>> +		}
>> +		if (nelems == 0)
>> +			continue;
> 
> Nit: Should this return an error if j == MAX_RESOLVE_DEPTH after the loop?

agree


> 
>>   
>>   		field_type = btf_get_field_type(__btf_name_by_offset(btf, var_type->name_off),
>>   						field_mask, &seen_mask, &align, &sz);
>> @@ -3584,7 +3650,7 @@ static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
>>   			return field_type;
>>   
>>   		off = vsi->offset;
>> -		if (vsi->size != sz)
>> +		if (vsi->size != sz * nelems)
>>   			continue;
>>   		if (off % align)
>>   			continue;
>> @@ -3624,9 +3690,14 @@ static int btf_find_datasec_var(const struct btf *btf, const struct btf_type *t,
>>   
>>   		if (ret == BTF_FIELD_IGNORE)
>>   			continue;
>> -		if (idx >= info_cnt)
>> +		if (idx + nelems > info_cnt)
>>   			return -E2BIG;
> 
> Nit: This is bounded by BTF_FIELDS_MAX which has value of 11,
>       would that be enough?

So far, no one has complained it yet!
But, some one will reach the limit in future.
If people want a flexible length, I will solve it in a follow-up.
WDYT?

> 
>> -		++idx;
>> +		if (nelems > 1) {
>> +			ret = btf_repeat_field(info, idx, nelems - 1, sz);
>> +			if (ret < 0)
>> +				return ret;
>> +		}
>> +		idx += nelems;
>>   	}
>>   	return idx;
>>   }
> 
>