Fix for issue#104

[rameezrehman408]

Weird indentation is corrected in all the .rst files.

Extra spaces at the start of the paragraphs are corrected. However, tabs/spaces in the examples (below '::') have been kept unchanged.

[qmonnet]

Looks mostly OK, apart from some new lines that sneaked in in one file. Thanks!

Once the commit is ready, we'll need to rebase it onto the Linux repository, and submit it to the BPF mailing list. Relevant documentation for submitting include:

• https://docs.kernel.org/process/submitting-patches.html
• https://docs.kernel.org/bpf/bpf_devel_QA.html#submitting-patches

Before submitting, we'll need to clean up the commit title and description:

• Commit title should be prefixed with bpftool:
• It should describe more precisely what we're doing (weird was good enough for the GitHub issue, but we want something clearer).
• We should also motivate the change. What we're doing is picking more sensible indentation markers, to have them easier to work with. I think the mix of tabs and spaces is here for historical reason: the first bpftool authors chose tabs because this is what is used in most places in the repo for indentation, and completed with spaces because, if I remember correctly, they wanted the description to align nicely on the first argument after **bpftool in the command description. But this has proved tricky to work with, and text editors typically trip on the syntax.

(Again, I'm happy to help with the submission process.)

Before we do that: I've been thinking more about formatting here. How much more are you willing to work on this? Looking at the description, I note that:

• You kept tabs for indents, which is OK. But I note that most other RST documentation in the kernel repo are using spaces for indents, and to be honest, spaces look a better choice to me for RST (where indent is sometimes tricky and spaces allow for more flexibility). So maybe we could take this chance to replace tabs with spaces in the whole section.
• Since we'd touch the whole section anyway, and given the indentation level will likely change, we could just as well re-wrap the body of the descriptions on 80-character wide lines. I did a quick test and it would change the generated man page, but only the wrapping of the paragraphs in its sources, not the result produced with man.
• I realised that the markup for bold text (**) is in fact not necessary in the lines with the command syntax, in the DESCRIPTION section, we're using RST description lists and rst2man already sets the full line in bold save for the keywords we manually set to italics instead. So we could maybe remove these markers, too.

Apologies for not providing this feedback earlier.

What do you think?

[qmonnet]

Thanks a lot! Apologies for the delay, I've been on vacations for a few days.

The change look great overall! I've got some minor comments, in particular:

• Can we make the indent changes in the OPTIONS sections consistent? Let's use spaces there as well, as you did for some pages. See the comments inline below.

• You have a lot of trailing spaces at the end of lines, can you please clean them up?

• Regarding the change to the cgroup attach types, can we instead make it a list (prefixing items with - ..., for example:

           *ATTACH_TYPE* can be on of:
  
           - **ingress** ingress path of the inet socket (since 4.10);
           - **egress** egress path of the inet socket (since 4.10);
           - **sock_create** opening of an inet socket (since 4.10);
           ...

  I would maybe make this change a separate commit, to make it easier to track - although you described it well in the commit log so one commit would also be acceptable.

• Can you rebase your changes on the latest version of the master branch? I've synced with the kernel repo today, and there's some conflict with Daniel's series.

• Please go through the other comments inline below.

[qmonnet]

Heads up on the next steps, once the PR is ready:

• We'll have to port it to the kernel repo (there won't be anything changing, other than the paths of the file, under tools/bpf/bpftool/Documentation/ instead of simply docs).
• We'll need to make sure that the commits are properly split (if we have several ones), and that they are properly formatted. In particular:
  • We want to prefix the title with bpftool: and explain what the commit does, for example: bpftool: simplify indentation in man pages or something like this
  • We want a proper Signed-off-by: tag, but you've got this in order already
  • We want a good description, and you have it; but we'll want to wrap it on something narrower, typically 72 character-wide lines
  • We'll run checkpatch.pl on the commits to make sure that everything else looks in order.
• Then we'll be able to submit the patches to the mailing list.

[qmonnet]

• We'll have to port it to the kernel repo (there won't be anything changing

That is, unless other patches are merged in the meantime. https://lore.kernel.org/all/20230829101838.851350-7-daan.j.demeyer@gmail.com/ looks like a good candidate for that.

[rameezrehman408]

I am grateful for your detailed review earlier. I believe all of the suggestions have been included in this 2nd commit.

I am new to the open-source domain and very excited to have commits/PR that may get included upstream into the kernel. I deeply appreciate your time and guidance on this.

I am a bit unsure about submitting the patches and will look into the references you shared earlier.

I also apologize for the huge delay in the requested changes. I have been quite busy since my last commit, in my day job along with back-to-back trainings for security certs (ISMS LA & CISM for now) required by my employer.

Once again thank you for the opportunity and waiting anxiously as well as nervously for your kind review. 😊

[qmonnet]

5. Examples in 'bpftool-gen.rst' are improved as 1tab/indentation = 2x spaces.

Can we revert this part? I've got nothing against the initial indent at 4 tabs, although if we did it, it should be consistent everywhere. But let's not use 2 spaces for one tab in the C code. The convention in the kernel repo is 8 spaces for one tab, there's no reason to have a different value here. But I'd recommend to just leave the code snippets are they are, it's probably acceptable to have tabs in there, and they're not something people edit often anyway. Same thing for bpftool-iter.rst and bpftool-net.rst where you changed the indent - let's keep the examples for another time?

Looks good otherwise, this is going into the right direction :)

Please find a few minor comments below.

[qmonnet]

I am grateful for your detailed review earlier. I believe all of the suggestions have been included in this 2nd commit.

Yes, thanks, great work!

I am new to the open-source domain and very excited to have commits/PR that may get included upstream into the kernel. I deeply appreciate your time and guidance on this.

You're welcome, thanks for the work and for coming back to this - there's nothing complex in this PR but this is a big one and I do realise that I've been giving you a good amount of changes during the review.

I am a bit unsure about submitting the patches and will look into the references you shared earlier.

Yes please, take a look at the docs. The summary is as I described earlier. But I'm happy to help with the process and check before you send, if it's helpful.

I also apologize for the huge delay in the requested changes. I have been quite busy since my last commit, in my day job along with back-to-back trainings for security certs (ISMS LA & CISM for now) required by my employer.

This is entirely fine. We're all busy with stuff. I perfectly understand you're not 100% focusing on this PR. Neither am I - that's why you get a 2-day long delay for review 🙂. Thanks for working on it and no worries about the time it takes, there's no rush. The most important is to do it well.

Once again thank you for the opportunity and waiting anxiously as well as nervously for your kind review. 😊

Oh dear, please don't feel anxious or nervous, that's not the objective! 🙂 I know the “Changes requested” may feel intimidating or frustrating because GitHub puts it in red, but that's just that really, some changes are necessary but the rest of the PR is great. Again I do realise I've raised a lot of things during the review, but since we're updating all bpftool docs, we should do it well, so I'm trying to be thorough!

We're nearly there, there are just a bunch of minor things to fix this time - aside from reverting the changes on the examples, maybe. I'd also recommend to rebase on the top of the master branch, although we may have more rebasing to do by the time we get the patch ready (there are at least two patch sets in flight with bpftool doc changes on the mailing list - nothing major, thankfully).

[rameezrehman408]

Thanks for your review.

I have included the corrections you asked and created a commit 169871f.

I did rebase my local to the rameezrehman408:master before committing. However, I believe you meant to rebase from the libbpf:master, which I believe required synching and was visible under the "sync fork" button (showing the difference in no. of commits between both repos) as below.

I have now merged the upstream/master and my master branch. I believe this was the thing that was required.

Requesting for your review.
Thanks a lot.

[qmonnet]

One remaining issue with the list, the rest looks OK.

[qmonnet]

I did rebase my local to the rameezrehman408:master before committing. However, I believe you meant to rebase from the libbpf:master, which I believe required synching and was visible under the "sync fork" button (showing the difference in no. of commits between both repos) as below.

Yes, I meant rebasing on top of upstream (libbpf/bpftool), sorry if this was unclear.

There's something wrong with your rebase though, it shouldn't change the libbpf version in use (this change). This is why the CI fails to run. Not really an issue because we'll send this patch to the ML anyway, but just so that you know.

[qmonnet]

Thanks! Changes look good. There's still a conflict in this repo (the updates in the synopsis of the document, see upstream commit).

As it turns out, we're also missing the latest updates from upstream that are not in the bpftool mirror repo yet, such as this commit.

Not sure what's the best workflow here, but we should probably start rebasing your changes against the bpf-next tree anyway. We can do it as a PR on this fork if you want to stay on GitHub for now; or your can rebase your changes, and send a patch over to me with git send-email (or to the mailing list, if you've read through the documentation for the process and are feeling confident about the updates).

What we need now:

• Rebase on upstream (bpf-next tree): rebase on top of latest changes + edit tools/bpf/bpftool/Documentation/*.rst instead of docs/*.rst in the current repo. This can be either on GitHub as I mentioned earlier, if you want another review, or just locally on your machine.
• Rework the Git history: split the set into logical commits (ideally: one commit for all whitespace/indent changes, one for everything else such as typo fixes), write relevant commit descriptions
• Run scripts/checkpatch.pl --strict from Linux repo on the changes, to make sure it doesn't raise any complaint
• Submit: git send-email to BPF mailing list and relevant reviewers (but feel free to send only to me first, if you want a review).

Let me know if you need more details.
I'm available on Cilium/eBPF Slack or IRC or email or whatever, if necessary

I know this is long and probably frustrating, bpftool is not extremely active but given that we're touching nearly all doc contents, it's super easy to get conflicts everytime someone else touches them. Thanks for bearing with it! :)

[qmonnet]

Hi @rameezrehman408, any news on this PR? I'm seeing upstream contributions with people still tripping over the weird indentation in the file, and I'd like to get this fix out in the coming weeks/days.

Are you still interested in this? If not, would you mind me to pick it up and send it as joint work?

[qmonnet]

I've rebased as the last three commits on this branch, let me know if you're OK with me posting these.

[rameezrehman408]

Dear @qmonnet,

I sincerely apologize for the delay, I have caused. I believe this branch is the most optimum way forward. I'd be more than glad to be a part of this contribution.

Thank you for your kind consideration and support.

[qmonnet]

https://lore.kernel.org/bpf/20240331200346.29118-1-qmo@kernel.org/T/#m60662ee17d95ab9f7fc02ac929fd816382966f52

[qmonnet]

Now merged upstream, I'll pull it to the GitHub mirror at the next sync-up. Thanks Rameez!