Re: [PATCH V3 bpf-next] BPF, docs: libbpf Overview Document

[Bagas Sanjaya]

On Mon, Mar 13, 2023 at 07:59:47AM -0500, David Vernet wrote:
> On Mon, Mar 13, 2023 at 04:44:59PM +0700, Bagas Sanjaya wrote:
> > On Fri, Mar 10, 2023 at 10:09:28AM -0800, Sreevani Sreejith wrote:
> > > From: Sreevani <ssreevani@xxxxxxxx>
> > > 
> > > 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.
> 
> Sreevani, please be sure to reply to and address all reviewers'
> comments. I've also requested that we not use these internal Meta tags
> on more than one occasion, so please be mindful of it for future
> patches, and take a bit of extra time to double check that you've
> addressed all reviewers' concerns. I also suggest reading over [0],
> which specifies that new versions of patches should include descriptions
> of what's changed from prior versions. Please see Joanne's patch set in
> [1] which serves as a very nice example.
> 
> [0]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format
> [1]: https://lore.kernel.org/all/20230301154953.641654-1-joannelkoong@xxxxxxxxx/
> 
> Bagas -- just FYI, a quick git log would have shown that this is only
> Sreevani's second patch. I don't think she intentionally ignored
> anything. It's likely just an artifact of getting used to the kernel
> review process.

Oops, you mean this v3 is actually v2, right?

> > Why did you add heading overline and change the heading character marker?
> 
> I assume that Sreevani is following python documentation conventions [0], which
> suggest that #### with overline refers to the highest-level heading in a page.
> This is suggested in Sphinx documentation [1] as well.
> 
> [0]: https://devguide.python.org/documentation/markup/#sections
> [1]: https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#sections

OK.

> > 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.
> 
> What is :lineos:? I don't see it anywhere else in Documentation/ and if
> I add it, the docs build complains:
> 
> Documentation/bpf/libbpf/libbpf_overview.rst:177: WARNING: Error in "code-block" directive:
> unknown option: "lineos".
> 
> .. code-block:: C
>   :lineos:
>   :emphasize-lines: 6

You forget to indent both options (see [1]).

> 
>   //...
>   struct task_struct *task = (void *)bpf_get_current_task();
>   struct task_struct *parent_task;
>   int err;
> 
>   err = bpf_core_read(&parent_task, sizeof(void *), &task->parent);
>   if (err) {
>     /* handle error */
>   }
> 
>   /* parent_task contains the value of task->parent pointer */
> 
> I personally think adding line numbers is overkill. The highlighting is
> already a nice touch, and gets the point across without the additional
> visual cue of line numbers.

But if the snippet above is instead long, how can one looking for the
emphasized line number when reading doc (especially in .rst source) other
than manually counting from the first line of the snippet? See
Documentation/RCU/rcubarrier.rst for example of manual line numbering
(and [2] for the related patch).

> > 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.
> 
> It says that at the end of the paragraph?
> 

I was confused between BPF apps and BPF programs, since I was accustomed
that apps and programs refer to the same thing.

Thanks.

[1]: https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-code-block
[2]: https://lore.kernel.org/linux-doc/e458e625-9a4e-da3f-13cd-a5b56fc36edf@xxxxxxxxx/
 
-- 
An old man doll... just what I always wanted! - Clara

Attachment: signature.asc
Description: PGP signature