[PATCH bpf-next] bpftool: Fix handling enum64 in btf dump sorting

[PATCH bpf-next] bpftool: Fix handling enum64 in btf dump sorting

[Mykyta]

From: Mykyta Yatsenko <yatsenko@meta.com>

Wrong function is used to access the first enum64 element.
Substituting btf_enum(t) with btf_enum64(t) for BTF_KIND_ENUM64.

Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
---
 tools/bpf/bpftool/btf.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 6789c7a4d5ca..b0f12c511bb3 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -557,16 +557,23 @@ static const char *btf_type_sort_name(const struct btf *btf, __u32 index, bool f
 	const struct btf_type *t = btf__type_by_id(btf, index);
 
 	switch (btf_kind(t)) {
-	case BTF_KIND_ENUM:
-	case BTF_KIND_ENUM64: {
+	case BTF_KIND_ENUM: {
 		int name_off = t->name_off;
 
 		/* Use name of the first element for anonymous enums if allowed */
-		if (!from_ref && !t->name_off && btf_vlen(t))
+		if (!from_ref && !name_off && btf_vlen(t))
 			name_off = btf_enum(t)->name_off;
 
 		return btf__name_by_offset(btf, name_off);
 	}
+	case BTF_KIND_ENUM64: {
+		int name_off = t->name_off;
+
+		if (!from_ref && !name_off && btf_vlen(t))
+			name_off = btf_enum64(t)->name_off;
+
+		return btf__name_by_offset(btf, name_off);
+	}
 	case BTF_KIND_ARRAY:
 		return btf_type_sort_name(btf, btf_array(t)->type, true);
 	case BTF_KIND_TYPE_TAG:
-- 
2.46.0

Re: [PATCH bpf-next] bpftool: Fix handling enum64 in btf dump sorting

[Daniel Borkmann]

On 9/1/24 11:30 PM, "Mykyta Yatsenko mykyta.yatsenko5"@gmail.com wrote:
> From: Mykyta Yatsenko <yatsenko@meta.com>
> 
> Wrong function is used to access the first enum64 element.
> Substituting btf_enum(t) with btf_enum64(t) for BTF_KIND_ENUM64.
> 
> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> ---
>   tools/bpf/bpftool/btf.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 6789c7a4d5ca..b0f12c511bb3 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -557,16 +557,23 @@ static const char *btf_type_sort_name(const struct btf *btf, __u32 index, bool f
>   	const struct btf_type *t = btf__type_by_id(btf, index);
>   
>   	switch (btf_kind(t)) {
> -	case BTF_KIND_ENUM:
> -	case BTF_KIND_ENUM64: {
> +	case BTF_KIND_ENUM: {
>   		int name_off = t->name_off;
>   
>   		/* Use name of the first element for anonymous enums if allowed */
> -		if (!from_ref && !t->name_off && btf_vlen(t))
> +		if (!from_ref && !name_off && btf_vlen(t))
>   			name_off = btf_enum(t)->name_off;
>   
>   		return btf__name_by_offset(btf, name_off);
>   	}

Small nit, could we consolidate the logic into the below? Still somewhat nicer
than duplicating all of the rest.

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 6789c7a4d5ca..aae6f5262c6a 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -562,8 +562,10 @@ static const char *btf_type_sort_name(const struct btf *btf, __u32 index, bool f
                 int name_off = t->name_off;

                 /* Use name of the first element for anonymous enums if allowed */
-               if (!from_ref && !t->name_off && btf_vlen(t))
-                       name_off = btf_enum(t)->name_off;
+               if (!from_ref && !name_off && btf_vlen(t))
+                       name_off = btf_kind(t) == BTF_KIND_ENUM64 ?
+                                  btf_enum64(t)->name_off :
+                                  btf_enum(t)->name_off;

                 return btf__name_by_offset(btf, name_off);
         }

Thanks,
Daniel

Re: [PATCH bpf-next] bpftool: Fix handling enum64 in btf dump sorting

[Mykyta Yatsenko]

On 02/09/2024 17:22, Daniel Borkmann wrote:
> On 9/1/24 11:30 PM, "Mykyta Yatsenko mykyta.yatsenko5"@gmail.com wrote:
>> From: Mykyta Yatsenko <yatsenko@meta.com>
>>
>> Wrong function is used to access the first enum64 element.
>> Substituting btf_enum(t) with btf_enum64(t) for BTF_KIND_ENUM64.
>>
>> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
>> ---
>>   tools/bpf/bpftool/btf.c | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
>> index 6789c7a4d5ca..b0f12c511bb3 100644
>> --- a/tools/bpf/bpftool/btf.c
>> +++ b/tools/bpf/bpftool/btf.c
>> @@ -557,16 +557,23 @@ static const char *btf_type_sort_name(const 
>> struct btf *btf, __u32 index, bool f
>>       const struct btf_type *t = btf__type_by_id(btf, index);
>>         switch (btf_kind(t)) {
>> -    case BTF_KIND_ENUM:
>> -    case BTF_KIND_ENUM64: {
>> +    case BTF_KIND_ENUM: {
>>           int name_off = t->name_off;
>>             /* Use name of the first element for anonymous enums if 
>> allowed */
>> -        if (!from_ref && !t->name_off && btf_vlen(t))
>> +        if (!from_ref && !name_off && btf_vlen(t))
>>               name_off = btf_enum(t)->name_off;
>>             return btf__name_by_offset(btf, name_off);
>>       }
>
> Small nit, could we consolidate the logic into the below? Still 
> somewhat nicer
> than duplicating all of the rest.
Thanks for the suggestion, this makes sense,  I will apply it.
>
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 6789c7a4d5ca..aae6f5262c6a 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -562,8 +562,10 @@ static const char *btf_type_sort_name(const 
> struct btf *btf, __u32 index, bool f
>                 int name_off = t->name_off;
>
>                 /* Use name of the first element for anonymous enums 
> if allowed */
> -               if (!from_ref && !t->name_off && btf_vlen(t))
> -                       name_off = btf_enum(t)->name_off;
> +               if (!from_ref && !name_off && btf_vlen(t))
> +                       name_off = btf_kind(t) == BTF_KIND_ENUM64 ?
> +                                  btf_enum64(t)->name_off :
> +                                  btf_enum(t)->name_off;
>
>                 return btf__name_by_offset(btf, name_off);
>         }
>
> Thanks,
> Daniel
>

[PATCH bpf-next] bpftool: Fix handling enum64 in btf dump sorting

[Mykyta Yatsenko]

From: Mykyta Yatsenko <yatsenko@meta.com>

Wrong function is used to access the first enum64 element.
Substituting btf_enum(t) with btf_enum64(t) for BTF_KIND_ENUM64.

Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
---
 tools/bpf/bpftool/btf.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
index 6789c7a4d5ca..3b57ba095ab6 100644
--- a/tools/bpf/bpftool/btf.c
+++ b/tools/bpf/bpftool/btf.c
@@ -561,9 +561,10 @@ static const char *btf_type_sort_name(const struct btf *btf, __u32 index, bool f
 	case BTF_KIND_ENUM64: {
 		int name_off = t->name_off;
 
-		/* Use name of the first element for anonymous enums if allowed */
-		if (!from_ref && !t->name_off && btf_vlen(t))
-			name_off = btf_enum(t)->name_off;
+		if (!from_ref && !name_off && btf_vlen(t))
+			name_off = btf_kind(t) == BTF_KIND_ENUM64 ?
+				btf_enum64(t)->name_off :
+				btf_enum(t)->name_off;
 
 		return btf__name_by_offset(btf, name_off);
 	}
-- 
2.46.0

Re: [PATCH bpf-next] bpftool: Fix handling enum64 in btf dump sorting

[Quentin Monnet]

On 2024-09-02 18:17 BST, Mykyta Yatsenko wrote:
> From: Mykyta Yatsenko <yatsenko@meta.com>
> 
> Wrong function is used to access the first enum64 element.
> Substituting btf_enum(t) with btf_enum64(t) for BTF_KIND_ENUM64.
> 
> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> ---
>  tools/bpf/bpftool/btf.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 6789c7a4d5ca..3b57ba095ab6 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -561,9 +561,10 @@ static const char *btf_type_sort_name(const struct btf *btf, __u32 index, bool f
>  	case BTF_KIND_ENUM64: {
>  		int name_off = t->name_off;
>  
> -		/* Use name of the first element for anonymous enums if allowed */
> -		if (!from_ref && !t->name_off && btf_vlen(t))
> -			name_off = btf_enum(t)->name_off;
> +		if (!from_ref && !name_off && btf_vlen(t))
> +			name_off = btf_kind(t) == BTF_KIND_ENUM64 ?
> +				btf_enum64(t)->name_off :
> +				btf_enum(t)->name_off;
>  
>  		return btf__name_by_offset(btf, name_off);
>  	}


(Please don't forget to tag your patch as a v2.)
Looks good, thanks!

Acked-by: Quentin Monnet <qmo@kernel.org>

Re: [PATCH bpf-next] bpftool: Fix handling enum64 in btf dump sorting

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Mon,  2 Sep 2024 18:17:21 +0100 you wrote:
> From: Mykyta Yatsenko <yatsenko@meta.com>
> 
> Wrong function is used to access the first enum64 element.
> Substituting btf_enum(t) with btf_enum64(t) for BTF_KIND_ENUM64.
> 
> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpftool: Fix handling enum64 in btf dump sorting
    https://git.kernel.org/bpf/bpf-next/c/b0222d1d9e6f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf-next] bpftool: Fix handling enum64 in btf dump sorting

[Andrii Nakryiko]

On Mon, Sep 2, 2024 at 9:22 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 9/1/24 11:30 PM, "Mykyta Yatsenko mykyta.yatsenko5"@gmail.com wrote:
> > From: Mykyta Yatsenko <yatsenko@meta.com>
> >
> > Wrong function is used to access the first enum64 element.
> > Substituting btf_enum(t) with btf_enum64(t) for BTF_KIND_ENUM64.
> >
> > Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
> > ---
> >   tools/bpf/bpftool/btf.c | 13 ++++++++++---
> >   1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> > index 6789c7a4d5ca..b0f12c511bb3 100644
> > --- a/tools/bpf/bpftool/btf.c
> > +++ b/tools/bpf/bpftool/btf.c
> > @@ -557,16 +557,23 @@ static const char *btf_type_sort_name(const struct btf *btf, __u32 index, bool f
> >       const struct btf_type *t = btf__type_by_id(btf, index);
> >
> >       switch (btf_kind(t)) {
> > -     case BTF_KIND_ENUM:
> > -     case BTF_KIND_ENUM64: {
> > +     case BTF_KIND_ENUM: {
> >               int name_off = t->name_off;
> >
> >               /* Use name of the first element for anonymous enums if allowed */
> > -             if (!from_ref && !t->name_off && btf_vlen(t))
> > +             if (!from_ref && !name_off && btf_vlen(t))
> >                       name_off = btf_enum(t)->name_off;
> >
> >               return btf__name_by_offset(btf, name_off);
> >       }
>
> Small nit, could we consolidate the logic into the below? Still somewhat nicer
> than duplicating all of the rest.
>
> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
> index 6789c7a4d5ca..aae6f5262c6a 100644
> --- a/tools/bpf/bpftool/btf.c
> +++ b/tools/bpf/bpftool/btf.c
> @@ -562,8 +562,10 @@ static const char *btf_type_sort_name(const struct btf *btf, __u32 index, bool f
>                  int name_off = t->name_off;
>
>                  /* Use name of the first element for anonymous enums if allowed */
> -               if (!from_ref && !t->name_off && btf_vlen(t))
> -                       name_off = btf_enum(t)->name_off;
> +               if (!from_ref && !name_off && btf_vlen(t))
> +                       name_off = btf_kind(t) == BTF_KIND_ENUM64 ?

Just fyi for the future (I don't think we need to fix this or anything
like that), but using BTF_KIND_xxx constants in btf_kind(t)
comparisons should be rare. Libbpf provides a full set of shorter
btf_is_xxx(t) helpers for this. So this would be just
`btf_is_enum64(t)`. What you did is not wrong, it's just more
open-coded and verbose.

> +                                  btf_enum64(t)->name_off :
> +                                  btf_enum(t)->name_off;
>
>                  return btf__name_by_offset(btf, name_off);
>          }
>
> Thanks,
> Daniel

Re: [PATCH bpf-next] bpftool: Fix handling enum64 in btf dump sorting

[Daniel Borkmann]

On 9/3/24 6:51 PM, Andrii Nakryiko wrote:
> On Mon, Sep 2, 2024 at 9:22 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> On 9/1/24 11:30 PM, "Mykyta Yatsenko mykyta.yatsenko5"@gmail.com wrote:
>>> From: Mykyta Yatsenko <yatsenko@meta.com>
>>>
>>> Wrong function is used to access the first enum64 element.
>>> Substituting btf_enum(t) with btf_enum64(t) for BTF_KIND_ENUM64.
>>>
>>> Signed-off-by: Mykyta Yatsenko <yatsenko@meta.com>
>>> ---
>>>    tools/bpf/bpftool/btf.c | 13 ++++++++++---
>>>    1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
>>> index 6789c7a4d5ca..b0f12c511bb3 100644
>>> --- a/tools/bpf/bpftool/btf.c
>>> +++ b/tools/bpf/bpftool/btf.c
>>> @@ -557,16 +557,23 @@ static const char *btf_type_sort_name(const struct btf *btf, __u32 index, bool f
>>>        const struct btf_type *t = btf__type_by_id(btf, index);
>>>
>>>        switch (btf_kind(t)) {
>>> -     case BTF_KIND_ENUM:
>>> -     case BTF_KIND_ENUM64: {
>>> +     case BTF_KIND_ENUM: {
>>>                int name_off = t->name_off;
>>>
>>>                /* Use name of the first element for anonymous enums if allowed */
>>> -             if (!from_ref && !t->name_off && btf_vlen(t))
>>> +             if (!from_ref && !name_off && btf_vlen(t))
>>>                        name_off = btf_enum(t)->name_off;
>>>
>>>                return btf__name_by_offset(btf, name_off);
>>>        }
>>
>> Small nit, could we consolidate the logic into the below? Still somewhat nicer
>> than duplicating all of the rest.
>>
>> diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c
>> index 6789c7a4d5ca..aae6f5262c6a 100644
>> --- a/tools/bpf/bpftool/btf.c
>> +++ b/tools/bpf/bpftool/btf.c
>> @@ -562,8 +562,10 @@ static const char *btf_type_sort_name(const struct btf *btf, __u32 index, bool f
>>                   int name_off = t->name_off;
>>
>>                   /* Use name of the first element for anonymous enums if allowed */
>> -               if (!from_ref && !t->name_off && btf_vlen(t))
>> -                       name_off = btf_enum(t)->name_off;
>> +               if (!from_ref && !name_off && btf_vlen(t))
>> +                       name_off = btf_kind(t) == BTF_KIND_ENUM64 ?
> 
> Just fyi for the future (I don't think we need to fix this or anything
> like that), but using BTF_KIND_xxx constants in btf_kind(t)
> comparisons should be rare. Libbpf provides a full set of shorter
> btf_is_xxx(t) helpers for this. So this would be just
> `btf_is_enum64(t)`. What you did is not wrong, it's just more
> open-coded and verbose.

Noted, that would have been better agree.

Thanks,
Daniel