Re: [PATCH bpf-next v5 4/6] bpftool: generated shadow variables for struct_ops maps.

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

On 2/28/24 16:09, Andrii Nakryiko wrote:
> On Wed, Feb 28, 2024 at 2:28 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>>
>>
>>
>> On 2/28/24 13:21, Kui-Feng Lee wrote:
>>> Will fix most of issues.
>>>
>>> On 2/28/24 10:25, Andrii Nakryiko wrote:
>>>> On Mon, Feb 26, 2024 at 5:04 PM Kui-Feng Lee <thinker.li@gmail.com>
>>>> wrote:
>>>>>
>>>>> + * type. Accessing them through the generated names may unintentionally
>>>>> + * corrupt data.
>>>>> + */
>>>>> +static int gen_st_ops_shadow_type(struct btf *btf, const char *ident,
>>>>> +                                 const struct bpf_map *map)
>>>>> +{
>>>>> +       int err;
>>>>> +
>>>>> +       printf("\t\tstruct {\n");
>>>>
>>>> would it be useful to still name this type? E.g., if it is `struct
>>>> bpf_struct_ops_tcp_congestion_ops in vmlinux BTF` we can name this one
>>>> as <skeleton-name>__bpf_struct_ops_tcp_congestion_ops. We have a
>>>> similar pattern for bss/data/rodata sections, having names is useful.
>>>
>>> If a user defines several struct_ops maps with the same name and type in
>>> different files, it can cause name conflicts. Unless we also prefix the
>>> name with the name of the skeleton. I am not sure if it is a good idea
>>> to generate such long names. If a user want to refer to the type, he
>>> still can use typeof(). WDYT?
>>
>> I misread your words. So, you were saying to prefix the skeleton name,
>> not map names. It is doable.
> 
> I did say to prefix with skeleton name, but *that* actually can lead
> to a conflict if you have two struct_ops maps that use the same BTF
> type. On the other hand, map names are unique, they are forced to be
> global symbols, so there is no way for them to conflict (it would be
> link-time error).

I avoided conflicts by checking if the definition of a type is already
generated.

For example, if there are two maps with the same type, they would looks 
like.
struct XXXSekelton {
     ...
     struct {
         struct struct_ops_type {
              ....
         } *map1;
         struct struct_ops_type *map2;
     } struct_ops;
   ...
};

WDYT?

> 
> How about we append both skeleton name, map name, and map's underlying
> BTF type? So:
> 
> <skel>__<map>__bpf_struct_ops_tcp_congestion_ops
> 
> ?
> 
> Is there any problem with having a long name?

No a big problem! Just not convenient to use.

> 
>>
>>>
>>>>
>>>>> +
>>>>> +       err = walk_st_ops_shadow_vars(btf, ident, map);
>>>>> +       if (err)
>>>>> +               return err;
>>>>> +
>>>>> +       printf("\t\t} *%s;\n", ident);
>>>>> +
>>>>> +       return 0;
>>>>> +}
>>>>> +