[PATCH] Fit line in 80 columns

[PATCH] Fit line in 80 columns

[Alejandro Colomar]

That line is used to generate the bpf-helpers(7) manual page.  It
is a no-fill line, since it represents a command, which means that
the formatter can't break the line, and instead just runs across
the right margin (in most set-ups this means that the pager will
break the line).

Using <fmt> makes it end exactly at the 80-col right margin, both
in the header file, and also in the manual page, and also seems to
be a sensible name.

Cc: bpf <bpf@vger.kernel.org>
Cc: linux-man <linux-man@vger.kernel.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Song Liu <songliubraving@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@kernel.org>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Jesper Dangaard Brouer <brouer@redhat.com>
Cc: Quentin Monnet <quentin@isovalent.com>
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
---

Hi,

We are adding another linter to the manual pages, which lints
about output going past the right margin of the terminal.

The following results triggered in the bpf-helpers(7) page:

$ make lint-man-groff
LINT (groff)	tmp/lint/man7/bpf-helpers.7.lint-man.groff.touch
an.tmac:man7/bpf-helpers.7:3: style: .TH missing third argument; suggest document modification date in ISO 8601 format (YYYY-MM-DD)
an.tmac:man7/bpf-helpers.7:3: style: .TH missing fourth argument; suggest package/project name and version (e.g., "groff 1.23.0")
an.tmac:man7/bpf-helpers.7:3: style: .TH missing fifth argument and second argument '7' not a recognized manual section; specify volume title
114:                        telnet-470   [001] .N.. 419421.045894: 0x00000001: <formatted msg>
2642:                                int res = bpf_probe_read_user_str(buf, sizeof(buf),
found style problems; aborting
make: *** [lib/lint-man.mk:50: tmp/lint/man7/bpf-helpers.7.lint-man.groff.touch] Error 1


This patch addresses the issue in line 114 (that line count is in
the output rendered page, not in the man(7) source file).

I'm not sure which of those files are used to produce the manual
page, but for consistency I thought it was best to fix both in the
same way.

Cheers,

Alex


 include/uapi/linux/bpf.h       | 5 ++---
 tools/include/uapi/linux/bpf.h | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index ef78e0e1a754..c03ae39c03b2 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1619,7 +1619,7 @@ union bpf_attr {
  *
  * 		::
  *
- * 			telnet-470   [001] .N.. 419421.045894: 0x00000001: <formatted msg>
+ * 			telnet-470   [001] .N.. 419421.045894: 0x00000001: <fmt>
  *
  * 		In the above:
  *
@@ -1636,8 +1636,7 @@ union bpf_attr {
  * 			* ``419421.045894`` is a timestamp.
  * 			* ``0x00000001`` is a fake value used by BPF for the
  * 			  instruction pointer register.
- * 			* ``<formatted msg>`` is the message formatted with
- * 			  *fmt*.
+ * 			* ``<fmt>`` is the message formatted with *fmt*.
  *
  * 		The conversion specifiers supported by *fmt* are similar, but
  * 		more limited than for printk(). They are **%d**, **%i**,
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index ef78e0e1a754..c03ae39c03b2 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1619,7 +1619,7 @@ union bpf_attr {
  *
  * 		::
  *
- * 			telnet-470   [001] .N.. 419421.045894: 0x00000001: <formatted msg>
+ * 			telnet-470   [001] .N.. 419421.045894: 0x00000001: <fmt>
  *
  * 		In the above:
  *
@@ -1636,8 +1636,7 @@ union bpf_attr {
  * 			* ``419421.045894`` is a timestamp.
  * 			* ``0x00000001`` is a fake value used by BPF for the
  * 			  instruction pointer register.
- * 			* ``<formatted msg>`` is the message formatted with
- * 			  *fmt*.
+ * 			* ``<fmt>`` is the message formatted with *fmt*.
  *
  * 		The conversion specifiers supported by *fmt* are similar, but
  * 		more limited than for printk(). They are **%d**, **%i**,
-- 
2.37.2

Re: [PATCH] Fit line in 80 columns

[Alexei Starovoitov]

On Thu, Aug 25, 2022 at 11:02 AM Alejandro Colomar
<alx.manpages@gmail.com> wrote:
>
> That line is used to generate the bpf-helpers(7) manual page.  It
> is a no-fill line, since it represents a command, which means that
> the formatter can't break the line, and instead just runs across
> the right margin (in most set-ups this means that the pager will
> break the line).
>
> Using <fmt> makes it end exactly at the 80-col right margin, both
> in the header file, and also in the manual page, and also seems to
> be a sensible name.

Nack.

We don't follow 80 char limit and are not going to because of man pages.

Re: [PATCH] Fit line in 80 columns

[Andrii Nakryiko]

On Thu, Aug 25, 2022 at 11:07 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Aug 25, 2022 at 11:02 AM Alejandro Colomar
> <alx.manpages@gmail.com> wrote:
> >
> > That line is used to generate the bpf-helpers(7) manual page.  It
> > is a no-fill line, since it represents a command, which means that
> > the formatter can't break the line, and instead just runs across
> > the right margin (in most set-ups this means that the pager will
> > break the line).
> >
> > Using <fmt> makes it end exactly at the 80-col right margin, both
> > in the header file, and also in the manual page, and also seems to
> > be a sensible name.
>
> Nack.
>
> We don't follow 80 char limit and are not going to because of man pages.

And it's questionable in general to enforce line length for verbatim
(code) block. It's verbatim for a good reason, it can't be wrapped.

Re: [PATCH] Fit line in 80 columns

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 1452 bytes --]

Hi Andrii and Alexei,

On 8/25/22 23:36, Andrii Nakryiko wrote:
> On Thu, Aug 25, 2022 at 11:07 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> On Thu, Aug 25, 2022 at 11:02 AM Alejandro Colomar
>> <alx.manpages@gmail.com> wrote:
>>>
>>> That line is used to generate the bpf-helpers(7) manual page.  It
>>> is a no-fill line, since it represents a command, which means that
>>> the formatter can't break the line, and instead just runs across
>>> the right margin (in most set-ups this means that the pager will
>>> break the line).
>>>
>>> Using <fmt> makes it end exactly at the 80-col right margin, both
>>> in the header file, and also in the manual page, and also seems to
>>> be a sensible name.
>>
>> Nack.
>>
>> We don't follow 80 char limit and are not going to because of man pages.
> 
> And it's questionable in general to enforce line length for verbatim
> (code) block. It's verbatim for a good reason, it can't be wrapped.

That's why instead of wrapping, I reduced the length of some 
"identifier".  It's not enforced, but it's nicer if it fits.  There are 
several other cases, where it wasn't easy to make it shorter, and I left 
it exceeding the margin.

It's not so crucial to fix it, and if you prefer it like it is 
currently, it's reasonable.  This is a suggestion, to make it easier to 
read.

Cheers,

Alex

-- 
Alejandro Colomar
<http://www.alejandro-colomar.es/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fit line in 80 columns

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 1161 bytes --]

On 8/25/22 23:51, Alejandro Colomar wrote:
> On 8/25/22 23:36, Andrii Nakryiko wrote:
>> On Thu, Aug 25, 2022 at 11:07 AM Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
>>> We don't follow 80 char limit and are not going to because of man pages.
>>
>> And it's questionable in general to enforce line length for verbatim
>> (code) block. It's verbatim for a good reason, it can't be wrapped.
> 
> That's why instead of wrapping, I reduced the length of some 
> "identifier".  It's not enforced, but it's nicer if it fits.  There are 
> several other cases, where it wasn't easy to make it shorter, and I left 
> it exceeding the margin.
> 
> It's not so crucial to fix it, and if you prefer it like it is 
> currently, it's reasonable.  This is a suggestion, to make it easier to 
> read.

As an example, you can see this commit recently applied to the man-pages:

<https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=e5da16f10f2d3d55db0bb1ef1156afcb197d8fdc>

I only made lines shorter in cases where no info was lost (including 
format).

-- 
Alejandro Colomar
<http://www.alejandro-colomar.es/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fit line in 80 columns

[G. Branden Robinson]

[-- Attachment #1: Type: text/plain, Size: 871 bytes --]

[sorry for the big CC]

At 2022-08-25T11:06:55-0700, Alexei Starovoitov wrote:
> Nack.
> 
> We don't follow 80 char limit and are not going to because of man
> pages.

If someone got a contract with O'Reilly or No Starch Press to write a
book on BPF and how revolutionarily awesome it is, it's conceivable they
would be faced with exposing some BPF-related function declarations in
the text.  In cases like the following, what would you have them do?

int bpf_map_update_elem(struct bpf_map *map, const void *key, const void *value, u64 flags)

Be aware that the author may not have infinite flexibility; publishers
generally impose a "house style" which restricts selection of typeface
(so they can't necessarily print at a smaller type size or with the
kerning reduced beyond a certain point to squeeze the text onto the
line).

Regards,
Branden

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fit line in 80 columns

[Alexei Starovoitov]

On Thu, Aug 25, 2022 at 3:54 PM G. Branden Robinson
<g.branden.robinson@gmail.com> wrote:
>
> [sorry for the big CC]
>
> At 2022-08-25T11:06:55-0700, Alexei Starovoitov wrote:
> > Nack.
> >
> > We don't follow 80 char limit and are not going to because of man
> > pages.
>
> If someone got a contract with O'Reilly or No Starch Press to write a
> book on BPF and how revolutionarily awesome it is, it's conceivable they
> would be faced with exposing some BPF-related function declarations in
> the text.  In cases like the following, what would you have them do?

If they're using a script to write a book their contract
should probably be revoked.

Re: [PATCH] Fit line in 80 columns

[Seth David Schoen]

G. Branden Robinson writes:

> If someone got a contract with O'Reilly or No Starch Press to write a
> book on BPF and how revolutionarily awesome it is, it's conceivable they
> would be faced with exposing some BPF-related function declarations in
> the text.  In cases like the following, what would you have them do?
> 
> int bpf_map_update_elem(struct bpf_map *map, const void *key, const void *value, u64 flags)

I imagine that the author or editor would break it over two lines like

int bpf_map_update_elem(struct bpf_map *map, const void *key,
                        const void *value, u64 flags)

or maybe the slightly uglier

int bpf_map_update_elem(struct bpf_map *map, const void *key,
    const void *value, u64 flags)

I just looked in _Advanced Programming in the Unix Environment, Third
Edition_ by Stevens and Rago (published by Addison Wesley rather than
either of the fine publishers you mentioned), and it didn't take long
to find a C function prototype that was split across two lines in the
former style, on p. 880

sem_t     *sem_open(const char *name, int oflag, ... /* mode_t mode,
                    unsigned int value */ );

... or on the next page, a better example because of the absence of the
comment:

ssize_t    sendto(int sockfd, const void *buf, size_t nbytes, int flags,
                  const struct sockaddr *destaddr, socklen_t destlen);

It's convenient that C's rules for whitespace semantics make all of
these line-wrapped prototypes have exactly the same meaning as their
un-line-wrapped equivalents, so programmers reading the books shouldn't
have cause to be confused by this.  It could be more challenging in a
language like Python, although there, too, there are syntactically
valid ways to break up some kinds of long lines.

Re: [PATCH] Fit line in 80 columns

[Greg KH]

On Thu, Aug 25, 2022 at 05:54:25PM -0500, G. Branden Robinson wrote:
> [sorry for the big CC]
> 
> At 2022-08-25T11:06:55-0700, Alexei Starovoitov wrote:
> > Nack.
> > 
> > We don't follow 80 char limit and are not going to because of man
> > pages.
> 
> If someone got a contract with O'Reilly or No Starch Press to write a
> book on BPF and how revolutionarily awesome it is, it's conceivable they
> would be faced with exposing some BPF-related function declarations in
> the text.  In cases like the following, what would you have them do?
> 
> int bpf_map_update_elem(struct bpf_map *map, const void *key, const void *value, u64 flags)
> 
> Be aware that the author may not have infinite flexibility; publishers
> generally impose a "house style" which restricts selection of typeface
> (so they can't necessarily print at a smaller type size or with the
> kerning reduced beyond a certain point to squeeze the text onto the
> line).

As someone who has written a book for one of those publishers you
mention above, this is totally not an issue at all.  Authors and
typesetters know how to properly wrap and handle stuff like this
automatically, it's what they do and has nothing to do with how kernel
header files are layed out.

But even then, if it was an issue, we don't write kernel code for "some
potential commercial entity that can't handle long lines", you know
better than this :)

greg k-h

[PATCH v2] Fit lines in 80 columns

[Alejandro Colomar]

Those lines is used to generate the bpf-helpers(7) manual page.
They are no-fill lines, since they represent code, which means
that the formatter can't break the line, and instead just runs
across the right margin (in most set-ups this means that the pager
will break the line).

Using <fmt> makes it end exactly at the 80-col right margin, both
in the header file, and also in the manual page, and also seems to
be a sensible name to me.

In the other case, the fix has been to separate the variable
definition and its use, as the kernel coding style recommends.

Nacked-by: Alexei Starovoitov <ast@kernel.org>
Cc: bpf <bpf@vger.kernel.org>
Cc: linux-man <linux-man@vger.kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Song Liu <songliubraving@fb.com>
Cc: Yonghong Song <yhs@fb.com>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@kernel.org>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Jesper Dangaard Brouer <brouer@redhat.com>
Cc: Quentin Monnet <quentin@isovalent.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: "G. Branden Robinson" <g.branden.robinson@gmail.com>
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
---
 include/uapi/linux/bpf.h       | 11 ++++++-----
 tools/include/uapi/linux/bpf.h | 11 ++++++-----
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index ef78e0e1a754..1443fa2a1915 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1619,7 +1619,7 @@ union bpf_attr {
  *
  * 		::
  *
- * 			telnet-470   [001] .N.. 419421.045894: 0x00000001: <formatted msg>
+ * 			telnet-470   [001] .N.. 419421.045894: 0x00000001: <fmt>
  *
  * 		In the above:
  *
@@ -1636,8 +1636,7 @@ union bpf_attr {
  * 			* ``419421.045894`` is a timestamp.
  * 			* ``0x00000001`` is a fake value used by BPF for the
  * 			  instruction pointer register.
- * 			* ``<formatted msg>`` is the message formatted with
- * 			  *fmt*.
+ * 			* ``<fmt>`` is the message formatted with *fmt*.
  *
  * 		The conversion specifiers supported by *fmt* are similar, but
  * 		more limited than for printk(). They are **%d**, **%i**,
@@ -3860,8 +3859,10 @@ union bpf_attr {
  * 			void bpf_sys_open(struct pt_regs *ctx)
  * 			{
  * 			        char buf[PATHLEN]; // PATHLEN is defined to 256
- * 			        int res = bpf_probe_read_user_str(buf, sizeof(buf),
- * 				                                  ctx->di);
+ * 			        int res;
+ *
+ * 			        res = bpf_probe_read_user_str(buf, sizeof(buf),
+ * 				                              ctx->di);
  *
  * 				// Consume buf, for example push it to
  * 				// userspace via bpf_perf_event_output(); we
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index ef78e0e1a754..1443fa2a1915 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1619,7 +1619,7 @@ union bpf_attr {
  *
  * 		::
  *
- * 			telnet-470   [001] .N.. 419421.045894: 0x00000001: <formatted msg>
+ * 			telnet-470   [001] .N.. 419421.045894: 0x00000001: <fmt>
  *
  * 		In the above:
  *
@@ -1636,8 +1636,7 @@ union bpf_attr {
  * 			* ``419421.045894`` is a timestamp.
  * 			* ``0x00000001`` is a fake value used by BPF for the
  * 			  instruction pointer register.
- * 			* ``<formatted msg>`` is the message formatted with
- * 			  *fmt*.
+ * 			* ``<fmt>`` is the message formatted with *fmt*.
  *
  * 		The conversion specifiers supported by *fmt* are similar, but
  * 		more limited than for printk(). They are **%d**, **%i**,
@@ -3860,8 +3859,10 @@ union bpf_attr {
  * 			void bpf_sys_open(struct pt_regs *ctx)
  * 			{
  * 			        char buf[PATHLEN]; // PATHLEN is defined to 256
- * 			        int res = bpf_probe_read_user_str(buf, sizeof(buf),
- * 				                                  ctx->di);
+ * 			        int res;
+ *
+ * 			        res = bpf_probe_read_user_str(buf, sizeof(buf),
+ * 				                              ctx->di);
  *
  * 				// Consume buf, for example push it to
  * 				// userspace via bpf_perf_event_output(); we
-- 
2.37.2

Re: [PATCH v2] Fit lines in 80 columns

[Daniel Borkmann]

On 8/29/22 9:58 PM, Alejandro Colomar wrote:
> Those lines is used to generate the bpf-helpers(7) manual page.
> They are no-fill lines, since they represent code, which means
> that the formatter can't break the line, and instead just runs
> across the right margin (in most set-ups this means that the pager
> will break the line).
> 
> Using <fmt> makes it end exactly at the 80-col right margin, both
> in the header file, and also in the manual page, and also seems to
> be a sensible name to me.
> 
> In the other case, the fix has been to separate the variable
> definition and its use, as the kernel coding style recommends.
> 
> Nacked-by: Alexei Starovoitov <ast@kernel.org>
> Cc: bpf <bpf@vger.kernel.org>
> Cc: linux-man <linux-man@vger.kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Andrii Nakryiko <andrii@kernel.org>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Cc: Song Liu <songliubraving@fb.com>
> Cc: Yonghong Song <yhs@fb.com>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: KP Singh <kpsingh@kernel.org>
> Cc: Stanislav Fomichev <sdf@google.com>
> Cc: Hao Luo <haoluo@google.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Jesper Dangaard Brouer <brouer@redhat.com>
> Cc: Quentin Monnet <quentin@isovalent.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Cc: "G. Branden Robinson" <g.branden.robinson@gmail.com>
> Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
> ---
>   include/uapi/linux/bpf.h       | 11 ++++++-----
>   tools/include/uapi/linux/bpf.h | 11 ++++++-----
>   2 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index ef78e0e1a754..1443fa2a1915 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1619,7 +1619,7 @@ union bpf_attr {
>    *
>    * 		::
>    *
> - * 			telnet-470   [001] .N.. 419421.045894: 0x00000001: <formatted msg>
> + * 			telnet-470   [001] .N.. 419421.045894: 0x00000001: <fmt>
>    *
>    * 		In the above:
>    *
> @@ -1636,8 +1636,7 @@ union bpf_attr {
>    * 			* ``419421.045894`` is a timestamp.
>    * 			* ``0x00000001`` is a fake value used by BPF for the
>    * 			  instruction pointer register.
> - * 			* ``<formatted msg>`` is the message formatted with
> - * 			  *fmt*.
> + * 			* ``<fmt>`` is the message formatted with *fmt*.
>    *
>    * 		The conversion specifiers supported by *fmt* are similar, but
>    * 		more limited than for printk(). They are **%d**, **%i**,
> @@ -3860,8 +3859,10 @@ union bpf_attr {
>    * 			void bpf_sys_open(struct pt_regs *ctx)
>    * 			{
>    * 			        char buf[PATHLEN]; // PATHLEN is defined to 256
> - * 			        int res = bpf_probe_read_user_str(buf, sizeof(buf),
> - * 				                                  ctx->di);
> + * 			        int res;
> + *
> + * 			        res = bpf_probe_read_user_str(buf, sizeof(buf),
> + * 				                              ctx->di);
>    *

Aside that this has been Nacked before, this looks really ugly. I'm not applying
this, sorry.

Thanks,
Daniel