Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1610921764-7526-1-git-send-email-alan.maguire@oracle.com> <1610921764-7526-2-git-send-email-alan.maguire@oracle.com>
In-Reply-To: <1610921764-7526-2-git-send-email-alan.maguire@oracle.com>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Wed, 20 Jan 2021 20:11:17 -0800
Message-ID: <CAEf4BzYvPiWnJYfVjg5qXaefYOsR1QHHzMfB6XFUSVeOA9W8Rg@mail.gmail.com>
Subject: Re: [PATCH v2 bpf-next 1/4] libbpf: add btf_has_size() and btf_int() inlines
To:     Alan Maguire <alan.maguire@oracle.com>
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>, Martin Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        john fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>, morbo@google.com,
        Shuah Khan <shuah@kernel.org>, bpf <bpf@vger.kernel.org>,
        Networking <netdev@vger.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sun, Jan 17, 2021 at 2:22 PM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> BTF type data dumping will use them in later patches, and they
> are useful generally when handling BTF data.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  tools/lib/bpf/btf.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index 1237bcd..0c48f2e 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -294,6 +294,20 @@ static inline bool btf_is_datasec(const struct btf_type *t)
>         return btf_kind(t) == BTF_KIND_DATASEC;
>  }
>
> +static inline bool btf_has_size(const struct btf_type *t)
> +{
> +       switch (BTF_INFO_KIND(t->info)) {
> +       case BTF_KIND_INT:
> +       case BTF_KIND_STRUCT:
> +       case BTF_KIND_UNION:
> +       case BTF_KIND_ENUM:
> +       case BTF_KIND_DATASEC:
> +               return true;
> +       default:
> +               return false;
> +       }
> +}

it's not clear what "has_size" means, actually. E.g., array type
definitely has size, it's not just as readily available. And you are
actually misusing this in your algorithm, I'll point it out in the
respective patch. Please remove this, or if absolutely necessary move
into btf_dump.c as an inner static function.

> +
>  static inline __u8 btf_int_encoding(const struct btf_type *t)
>  {
>         return BTF_INT_ENCODING(*(__u32 *)(t + 1));
> @@ -309,6 +323,11 @@ static inline __u8 btf_int_bits(const struct btf_type *t)
>         return BTF_INT_BITS(*(__u32 *)(t + 1));
>  }
>
> +static inline __u32 btf_int(const struct btf_type *t)
> +{
> +       return *(__u32 *)(t + 1);
> +}
> +

there is btf_int_encoding(), btf_ind_offset() and btf_int_bits() that
properly decompose what btf_int() above returns. I'm not convinced
btf_int() has to be exposed as a public API.

>  static inline struct btf_array *btf_array(const struct btf_type *t)
>  {
>         return (struct btf_array *)(t + 1);
> --
> 1.8.3.1
>