Re: [PATCH bpf-next v2 07/18] libbpf: Add enum64 support for btf_dump

[Yonghong Song <yhs@fb.com>]

On 5/16/22 5:37 PM, Andrii Nakryiko wrote:
> On Fri, May 13, 2022 at 8:13 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> Add enum64 btf dumping support. For long long and unsigned long long
>> dump, suffixes 'LL' and 'ULL' are added to avoid compilation errors
>> in some cases.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   tools/lib/bpf/btf.h      |   5 ++
>>   tools/lib/bpf/btf_dump.c | 135 ++++++++++++++++++++++++++++++---------
>>   2 files changed, 110 insertions(+), 30 deletions(-)
>>
> 
> [...]
> 
>> @@ -989,38 +992,88 @@ static void btf_dump_emit_enum_fwd(struct btf_dump *d, __u32 id,
>>          btf_dump_printf(d, "enum %s", btf_dump_type_name(d, id));
>>   }
>>
>> -static void btf_dump_emit_enum_def(struct btf_dump *d, __u32 id,
>> -                                  const struct btf_type *t,
>> -                                  int lvl)
>> +static void btf_dump_emit_enum32_val(struct btf_dump *d,
>> +                                    const struct btf_type *t,
>> +                                    int lvl, __u16 vlen)
>>   {
>>          const struct btf_enum *v = btf_enum(t);
>> -       __u16 vlen = btf_vlen(t);
> 
> why passing it from outside if we can just get it from t? you don't do
> it for kflag, for example, so I see no reason to do that for vlen here

We have a vlen passed in because we have a check for vlen outside
btf_dump_emit_enum32_val. Basically this function and some other codes
will not be executed if vlen == 0.

> 
>> +       bool is_signed = btf_kflag(t);
>> +       const char *fmt_str;
>>          const char *name;
>>          size_t dup_cnt;
>>          int i;
>>
>> +
> 
> nit: extra empty line?

sure.

> 
>> +       for (i = 0; i < vlen; i++, v++) {
>> +               name = btf_name_of(d, v->name_off);
>> +               /* enumerators share namespace with typedef idents */
>> +               dup_cnt = btf_dump_name_dups(d, d->ident_names, name);
>> +               if (dup_cnt > 1) {
>> +                       fmt_str = is_signed ? "\n%s%s___%zd = %d,"
>> +                                           : "\n%s%s___%zd = %u,";
>> +                       btf_dump_printf(d, fmt_str,
>> +                                       pfx(lvl + 1), name, dup_cnt,
>> +                                       v->val);
>> +               } else {
>> +                       fmt_str = is_signed ? "\n%s%s = %d,"
>> +                                           : "\n%s%s = %u,";
>> +                       btf_dump_printf(d, fmt_str,
>> +                                       pfx(lvl + 1), name,
>> +                                       v->val);
> 
> 100 character lines are ok now, try to make all those statements
> single-line, if possible

okay.

> 
>> +               }
>> +       }
>> +}
>> +
> 
> [...]