Hi!
On Thu, Jun 06, 2024 at 07:06:05PM GMT, Joe Damato wrote:
> On Thu, Jun 06, 2024 at 11:39:58PM +0200, Alejandro Colomar wrote:
> > Hi Joe,
> >
> > On Tue, Jun 04, 2024 at 06:17:40PM GMT, Joe Damato wrote:
> > > Signed-off-by: Joe Damato <jdamato@xxxxxxxxxx>
> >
> > Thanks for the patch! Please see a few comments below.
>
> I've included some questions below just to make sure I give you a v2
> that is much closer to correct :)
Nice :)
> > > +.TH ioctl_epoll 2 (date) "Linux man-pages (unreleased)"
> > > +.SH NAME
> > > +ioctl_epoll \- ioctl() operations for epoll file descriptors
> >
> > Please add link pages <man2const/EPIOCSPARAMS.2const> and
> > <man2const/EPIOCGPARAMS.2const>.
>
> And after adding those, I'd add those to SEE ALSO and I'd omit the
> description of them from the ioctl_epoll.2 page (because they'd have
> their own pages) ?
Nope. The manual page would still be <ioctl_epoll.2>. The other two
would be link pages, that is, if you `man EPIOCSPARAMS`, you'd get the
manual page ioctl_epoll(2).
See for example the manual page slist(3):
$ head man/man3/slist.3
.\" Copyright (c) 1993
.\" The Regents of the University of California. All rights reserved.
.\" and Copyright (c) 2020 by Alejandro Colomar <alx@xxxxxxxxxx>
.\"
.\" SPDX-License-Identifier: BSD-3-Clause
.\"
.\"
.TH SLIST 3 (date) "Linux man-pages (unreleased)"
.SH NAME
SLIST_EMPTY,
It has several aliases, one for each macro that it documents. Here are
a few examples:
$ head man/man3/SLIST_EMPTY.3
.so man3/slist.3
$ head man/man3/SLIST_FOREACH.3
.so man3/slist.3
That .so roff(7) request is similar to a C #include, so that they're
actually the same page. You can check in your terminal, with
`man slist` and `man SLIST_FOREACH`, which will give you the same page.
So, basically, I want you to write these:
$ cat man/man2const/EPIOCSPARAMS.2const
.so man2/ioctl_epoll.2
$ cat man/man2const/EPIOCGPARAMS.2const
.so man2/ioctl_epoll.2
Nothing else is required.
> > > +In the above,
> > > +.I fd
> > > +is a file descriptor referring to an epoll file descriptor obtained with a
> > > +call to
> > > +.BR epoll_create (2).
> > > +.I op
> > > +is one of the operations listed below, and
> > > +.I argp
> > > +is a pointer to the data structure described below.
> >
> > If argp is a pointer to a structure, then the prototype should document
> > that:
> >
> > .BI "int ioctl(int " fd ", int " op ", struct epoll_params *" argp );
> >
> > Also, I would document the two operations separately:
> >
> > .BI "int ioctl(int " fd ", EPIOCSPARAMS, const struct epoll_params *" argp );
> > .BI "int ioctl(int " fd ", EPIOCGPARAMS, struct epoll_params *" argp );
> >
> > This allows documenting that the 'S' one doesn't modify the argp
> > (AFAICS).
>
> Do you mean that I should omit the generic
>
> .BI "int ioctl(int " fd ", int " op ", struct epoll_params *" argp );
>
> and instead have the above with the two ops?
Exactly.
As in for example, PR_SET_MM_START_DATA.2const. You can have a look at
that page in the repository, because it hasn't yet been released.
I'll paste here a copy of it, since it's short:
$ MANWIDTH=72 man PR_SET_MM_START_DATA | cat
PR_SET_MM_START_DATA(2const) PR_SET_MM_START_DATA(2const)
NAME
PR_SET_MM_START_DATA, PR_SET_MM_END_DATA - modify kernel memory
map descriptor fields
LIBRARY
Standard C library (libc, -lc)
SYNOPSIS
#include <linux/prctl.h> /* Definition of PR_* constants */
#include <sys/prctl.h>
int prctl(PR_SET_MM, PR_SET_MM_START_DATA, unsigned long addr, 0L, 0L);
int prctl(PR_SET_MM, PR_SET_MM_END_DATA, unsigned long addr, 0L, 0L);
DESCRIPTION
PR_SET_MM_START_DATA
Set the address above which initialized and uninitialized
(bss) data are placed. The corresponding memory area must
be readable and writable, but not executable or shareable.
PR_SET_MM_END_DATA
Set the address below which initialized and uninitialized
(bss) data are placed. The corresponding memory area must
be readable and writable, but not executable or shareable.
RETURN VALUE
On success, 0 is returned. On error, -1 is returned, and errno is
set to indicate the error.
ERRORS
EINVAL
addr is greater than TASK_SIZE (the limit on the size of
the user address space for this architecture).
EINVAL
The permissions of the corresponding memory area are not as
required.
STANDARDS
Linux.
HISTORY
Linux 3.3.
SEE ALSO
prctl(2)
Linux man‐pages 6.8‐151‐g58... 2024‐06‐01 PR_SET_MM_START_DATA(2const)
BTW, now I realize that page is missing a reference to PR_SET_MM(2const)
in the SEE ALSO section. I'll fix that in a moment.
> > > +.\"
> > > +.P
> > > +All supported
> > > +.I op
> > > +values (described below) use an
> > > +.I argp
> > > +argument which is a pointer to a
> > > +.I epoll_params
> > > +structure, defined as:
> > > +.P
> > > +.in +4n
> > > +.EX
> > > +struct epoll_params {
> > > + uint32_t busy_poll_usecs; /* Number of usecs to busy poll */
> > > + uint16_t busy_poll_budget; /* Maximum number of packets to retrieve per poll */
> > > + uint8_t prefer_busy_poll; /* Boolean to enable or disable prefer busy poll */
> > > +
> > > + /* pad the struct to a multiple of 64bits */
> > > + uint8_t __pad; /* Must be zero */
> > > +};
> >
> > You could add this type definition to the SYNOPSIS, below the function
> > prototypes.
>
> OK, I moved it.
>
> I'm not sure if it is formatted properly, though. I'll see if I can
> find other examples of this style to check against.
See timespec(3type) or sockaddr(3type) for examples of pages documenting
structures in the SYNOPSIS.
> > > +The specified
> > > +.I op
> > > +is invalid.
> > > +.TP
> > > +.B EINVAL
> > > +The
> > > +.I fd
> > > +specified is not an epoll file descriptor, or the
> > > +.I op
> > > +specified is invalid, or the
> > > +.I __pad
> > > +was not initialized to zero, or
> > > +.I busy_poll_usecs
> > > +exceeds
> > > +.B INT_MAX ,
> > > +or
> > > +.I prefer_busy_poll
> > > +is not 0 or 1.
> >
> > Please separate the different conditions for EINVAL into separate
> > entries. The ones that are related can go in the same one, but the
> > unrelated ones are better split.
>
> Thanks!
>
> I did something like this:
>
> .TP
> .B EINVAL
> The
> .I fd
> specified is not an epoll file descriptor.
Please use the same exact wording as in ioctl(2), for consistency.
> .TP
> .B EINVAL
> The
> .I op
> specified is invalid.
I would remove this one, since we're documenting that the calls be done
with specific operations. There's no 'op' variable. The variable 'op'
error is already documented in ioctl(2).
> .TP
> .B EINVAL
> The
I would remove the above 'The'.
> .I argp.__pad
> was not initialized to zero.
Maybe remove 'initialized to', to abbreviate. If it's not zero, it's
of course because it wasn't initialized to zero. :)
> is that what you meant?
Yup.
> > > +(which is 64 as of Linux 6.9).
> > > +.TP
> > > +.B EFAULT
> > > +.I argp
> > > +does not point to a valid memory address.
> > > +.SH EXAMPLES
> > > +.EX
> >
> > Could you write an entire program, with a main()?
> >
> > If not, it's fine; this is better than nothing. But we prefer having
> > entire programs that users can paste and try.
>
> Hmm.. I can give it a try! It's a bit tricky because to actually use
> busy polling, you need to have a program that accepts incoming
> connections and calls epoll_wait (after setting the appropriate
> values with the ioctl).
>
> I could write something simple that does that but it would be a bit
> long, I think.
Hmmm, please do write it, but if it's so big, please do it in a second
patch, so that we're always in time to revert that one if it's too much.
Keep the current example in the patch that adds the page.
Thank you,
and have a lovely day!
Alex
--
<https://www.alejandro-colomar.es/>
Attachment:
signature.asc
Description: PGP signature
