Date:   Mon, 13 Mar 2023 16:44:59 +0700
From:   Bagas Sanjaya <bagasdotme@gmail.com>
To:     Sreevani Sreejith <ssreevani@meta.com>, void@manifault.com,
        psreep@gmail.com, bpf@vger.kernel.org,
        Linux-kernel@vger.kernel.org, andrii@kernel.org, mykola@meta.com
Cc:     linux-doc@vger.kernel.org
Subject: Re: [PATCH V3 bpf-next] BPF, docs: libbpf Overview Document
Message-ID: <ZA7wm8scokV+XPav@debian.me>
References: <20230310180928.2462527-1-ssreevani@meta.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="okP/XnxEkGF/S5Bq"
Content-Disposition: inline
In-Reply-To: <20230310180928.2462527-1-ssreevani@meta.com>
Precedence: bulk


--okP/XnxEkGF/S5Bq
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Mar 10, 2023 at 10:09:28AM -0800, Sreevani Sreejith wrote:
> From: Sreevani <ssreevani@meta.com>
>=20
> Summary: Document that provides an overview of libbpf features for BPF
> application development.

It seems like you ignore some of my reviews at [1]. Anyway, I repeat
them here, augmenting my new comments.

The patch description should have been "Document overview of libbpf,
including its features for developing BPF programs.".

> +######
>  libbpf
> -=3D=3D=3D=3D=3D=3D
> +######

Why did you add heading overline and change the heading character marker?

> +The following code snippet shows how to read the parent field of a kernel
> +``task_struct`` using BPF CO-RE and libbf. The basic helper to read a fi=
eld in a
> +CO-RE relocatable manner is ``bpf_core_read(dst, sz, src)``, which will =
read
> +``sz`` bytes from the field referenced by ``src`` into the memory pointe=
d to by
> +``dst``.
> +
> +  .. code-block:: C
> +    :emphasize-lines: 6
> +
> +    //...
> +    struct task_struct *task =3D (void *)bpf_get_current_task();
> +    struct task_struct *parent_task;
> +    int err;
> +
> +    err =3D bpf_core_read(&parent_task, sizeof(void *), &task->parent);
> +    if (err) {
> +      /* handle error */
> +    }
> +
> +    /* parent_task contains the value of task->parent pointer */

You may want to also add :lineos: option or manually add line numbers
if you add :emphasize-lines: so that readers can see the line number
it refers to.

> +Also, find the libbpf API documentation `here
> +<https://libbpf.readthedocs.io/en/latest/api.html>`_

"See also `libbpf API documentation <link>`_".

> +
> +libbpf and Rust
> +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> +
> +If you are building BPF applications in Rust, it is recommended to use t=
he
> +`Libbpf-rs <https://github.com/libbpf/libbpf-rs>`_ library instead of bi=
ndgen
> +bindings directly to libbpf. Libbpf-rs wraps libbpf functionality in
> +Rust-idiomatic interfaces and provides libbpf-cargo plugin to handle BPF=
 code
> +compilation and skeleton generation. Using Libbpf-rs will make building =
user
> +space part of the BPF application easier. Note that the BPF program them=
selves
> +must still be written in plain C.

BPF apps are application that use BPF program, right? I thought that
despite there is libbpf-rs, I still have to develop BPF apps in C.

Thanks.

[1]: https://lore.kernel.org/linux-doc/ZAqzeQZLNMyaZOck@debian.me/

--=20
An old man doll... just what I always wanted! - Clara

--okP/XnxEkGF/S5Bq
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQSSYQ6Cy7oyFNCHrUH2uYlJVVFOowUCZA7wlwAKCRD2uYlJVVFO
ow8vAP4rjUd07tAFMNXeoUbvQ2+9unWvXYLxgNH7/5YFykfw7wD/asWTzftMz36d
LRR+Tn7gOkXksj1jB+PZ4+OVTHyIKA0=
=eyQJ
-----END PGP SIGNATURE-----

--okP/XnxEkGF/S5Bq--