Re: [PATCH v2 1/1] ioctl_epoll.2: New page describing epoll ioctl(2)

[Joe Damato <jdamato@fastly.com>]

On Tue, Jun 11, 2024 at 07:14:14PM +0200, Alejandro Colomar wrote:
> Hi Joe,
> 
> On Tue, Jun 11, 2024 at 09:34:29AM GMT, Joe Damato wrote:
> > No problem at all; I really do appreciate your work here keeping the
> > man pages consistent and usable. Thanks for giving your time to help
> > me get this man page setup properly.
> 
> :-}
> 
> > > Ahh, sure, and for the constants too.  We prefer glibc headers when
> > > available.  I had the inertia that most ioctl(2)s do not have glibc
> > > headers.
> > 
> > Ah right forgot about the constants, so what I did instead was this, which
> > includes the header only once:
> > 
> > .SH SYNOPSIS
> > .EX
> > .BR "#include <sys/epoll.h>" "  /* Definition of " EPIOC* " constants */"
> > .B "#include <sys/ioctl.h>"
> > .P
> > .BI "int ioctl(int " fd ", EPIOCSPARAMS, const struct epoll_params *" argp );
> > .BI "int ioctl(int " fd ", EPIOCGPARAMS, struct epoll_params *" argp );
> > .P
> > .B struct epoll_params {
> > .BR "    uint32_t busy_poll_usecs;" "  /* Number of usecs to busy poll */"
> > .BR "    uint16_t busy_poll_budget;" " /* Maximum number of packets to retrieve per poll */"
> > .BR "    uint8_t prefer_busy_poll;" "  /* Boolean to enable or disable prefer busy poll  */"
> > \&
> > .BR " " "   /* pad the struct to a multiple of 64bits */"
> > .BR "    uint8_t __pad;"            "  /* Must be zero */"
> > .B };
> > 
> > Does that look OK?
> 
> Nah, that makes it unclear which header provides the type.  I'd add the
> include again right before the struct definition.  Some other pages have
> similar style (although I can't remember at the moment any example).

OK, I've included it twice -- once before the ioctls and once before
the struct, with a comment:

.BR "#include <sys/epoll.h>" " /* Definition of " struct " "epoll_params " */"
.P
.B struct epoll_params {

Hope that is OK! If not, let me know ;)

> > > > Changed this to:
> > > > 
> > > > retrieve on each poll attempt. This value cannot exceed
> > > > .B NAPI_POLL_WEIGHT
> > > > (which is 64 as of Linux 6.9), unless the process is run with
> > > > .B CAP_NET_ADMIN.
> > > > 
> > > > How is that?
> > > 
> > > Much better.  (But still needs to use semantic newlines.)
> > 
> > Hmm, I need to go back and re-read the semantic newline email because I made
> > this section look like this:
> 
> You may want to also read this commit:
> 
> <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/man7/man-pages.7?id=6ff6f43d68164f99a8c3fb66f4525d145571310c>
> 
> It includes a quote from Brian W. Kernighan, which is a little bit more
> detailed than man-pages(7) about it.

I just read that and will continue read it a few more times. I will
try to better understand how to format the man page text as you've
explained.

Please accept my apologies if I've still gotten it wrong in the v3,
I'm not quite sure I've totally wrapped my head around when/where
are good places to wrap long lines that exceed 80 characters.

> > 
> > .P
> > .I argp.busy_poll_budget
> > the maximum number of packets that the network stack will retrieve on each poll attempt.
> > This value cannot exceed
> > .B NAPI_POLL_WEIGHT
> > (which is 64 as of Linux 6.9), unless the process is run with
> > .B CAP_NET_ADMIN.
> > 
> > But I get the feeling this is still incorrect.
> 
> Yep; it's incorrect.  We have a strict limit on column 80, so you'd need
> to find a good break point in the middle.  I'd say
> 
> 	s/retrieve on/retrieve\non/
> 
> (although there are other good break points, such as for example maybe
> before 'retrieve').
> 
> and also break after the comma.
> 
> However, it was more correct than the previous, which continued the line
> after a period, which is a no-no.  :)

Thanks, I've made the change you've suggested above and am
re-reading each line looking for anything over 80 chars that I can
break on punctuation or any other clause.

I have already broken lines at periods and simple cases like that,
but I am sure to be missing a few.

> > I tried to follow the discussion you and Branden had in the following emails. I
> > apologize, but I don't think I quite follow what I should be using as a result
> > of that conversation?
> > 
> > \en 
> > \\n
> 
> TL;DR:
> 
> \\n is not appropriate, since it can be misinterpreted in some cases.
> For example, in some cases, you'd need to double-escape: \\\\n.  Don't
> use it.
> 
> \e is meh.  It means "write an escape character, whatever that is".
> This is incorrect, since we want a backslash, not "whatever an escape
> character is".  However, since the project has been using that for a
> long time, you can use it in your patch.
> 
> \[rs] is the most appropriate, which means write a "reverse solidus"
> (a.k.a., the backslash).  I'll prepare a churny patch for replacing \e
> by \[rs] globally in the project.  If you feel like using it in your
> patch, you're invited.  :)

OK, I'll switch the \e for \[rs], which also seems to render
properly for me.

Thanks,
Joe