On Thu, Jul 21, 2022 at 03:04:23PM +0200, Alejandro Colomar wrote:
> Hi Jeremy and Darrick,
>
> On 7/21/22 02:12, Darrick J. Wong wrote:
> > On Wed, Jul 20, 2022 at 04:45:12PM -0700, Jeremy Bongio wrote:
> > > Signed-off-by: Jeremy Bongio <bongiojp@xxxxxxxxx>
> > > ---
> > >
> > > This is a ext4 filesystem specific ioctl. However, this ioctl will
> > > likely be implemented for multiple filesystems at which point this
> > > manpage will be updated.
> > >
> > > man2/ioctl_fsuuid.2 | 115 ++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 115 insertions(+)
> > > create mode 100644 man2/ioctl_fsuuid.2
> > >
> > > diff --git a/man2/ioctl_fsuuid.2 b/man2/ioctl_fsuuid.2
> > > new file mode 100644
> > > index 000000000..53747684f
> > > --- /dev/null
> > > +++ b/man2/ioctl_fsuuid.2
> > > @@ -0,0 +1,115 @@
> > > +.\" Copyright (c) 2022 Google, Inc., written by Jeremy Bongio <bongiojp@xxxxxxxxx>
> > > +.\"
> > > +.\" SPDX-License-Identifier: Linux-man-pages-copyleft
> > > +.TH IOCTL_FSUUID 2 2022-07-20 "Linux" "Linux Programmer's Manual"
> > > +.SH NAME
> > > +ioctl_fsuuid \- get or set an ext4 filesystem uuid
> > > +.SH LIBRARY
> > > +Standard C library
> > > +.RI ( libc ", " \-lc )
> >
> > I'm not sure if libc will actually wrap this one, they often won't do
> > that for ioctls.
>
> Actually, we also specify libc for syscalls without a wrapper (e.g., see
> membarrier(2)). That rationale is that you need libc even if you use
> syscall(SYS_membarrier, ...), since syscall(2) is provided by libc.
>
> However, there's a difference in the synopsis:
> If syscall(2) needs to be used to call the syscall, we document it as such.
> Again, see membarrier(2) for an example of how we document that.
I understand that manpages for system calls that don't have a libc
wrapper document the use of syscall(SYS_fubar...) to call them. But
this is an ioctl, not a kernel system call that has no convenient libc
wrapper. ioctl(2) has been part of the Unix programming manual since
1979 or so, and it's been in Linux since v0.99. I think we can take for
granted that programmers can figure out 'man -s2 ioctl' if we tell them
to.
> >
> > > +.SH SYNOPSIS
> > > +.nf
> > > +.B #include <sys/ioctl.h>
> > > +.PP
> > > +.BI "int ioctl(int " fd ", EXT4_IOC_GETFSUUID, struct " fsuuid ");"
> > > +.BI "int ioctl(int " fd ", EXT4_IOC_SETFSUUID, struct " fsuuid ");"
>
> Can we use ioctl(2), or do we need syscall(SYS_ioctl, ...)?
So yes, technically an ioctl_XXX manpage should document the fact that
it depends on the existence of an ioctl(fd, number, param...) call, and
that in turn depends on syscall(SYS_ioctl, fd, number, param...) if
somehow ioctl() itself is not available and that in turn depends on
using any of the usual Linux C libraries, but this seems very pedantic
to repeat that for every single ioctl manpage in existence.
IOWS, I think we can take for granted that most C programmers on Linux
are working with a conventional C library, so it's sufficient to put:
SEE ALSO
ioctl(2)
at the end of an ioctl_XXX manpage like this one.
>
> > > +.fi
> > > +.SH DESCRIPTION
> > > +If an ext4 filesystem supports uuid manipulation, these
> > > +.BR ioctl (2)
> > > +operations can be used to get or set the uuid for the ext4 filesystem
> > > +on which
> > > +.I fd
> > > +resides.
> > > +.PP
> > > +The argument to these operations should be a pointer to a
> > > +.IR "struct fsuuid" ":"
> > > +.PP
> > > +.in +4n
> > > +.EX
> > > +struct fsuuid {
>
> Would you consider documenting the type separate manual page?
> See for example man2/open_how.2type and man3/tm.3type.
Why? There's only one user of this struct, there's no need to waste
people's time making them look up the third ioctl argument in a separate
manpage. If some other ioctl/syscall/whatever wants to start using
struct fsuuid then yes, this should be hoisted to a separate file so
that both manpages can reference it.
> > > + __u32 fsu_len; /* Number of bytes in a uuid */
> > > + __u32 fsu_flags; /* Mapping flags */
> > > + __u8 fsu_uuid[]; /* Byte array for uuid */
>
> We use 4-space indents for code.
>
> > > +};
> > > +.EE
> > > +.PP
> > > +The
> > > +.I fsu_flags
> > > +field must be set to 0.
> >
> > Nit: whitespace at the end of the line.
> >
> > > +.PP
> > > +If an
> > > +.BR EXT4_IOC_GETFSUUID
> > > +operation is called with
> > > +.I fsu_len
> > > +set to 0,
> > > +.I fsu_len
> > > +will be reassigned the number of bytes in an ext4 filesystem uuid
> >
> > "...will be set to the number of bytes..." ?
> >
> > > +and the return code will be -EINVAL.
> > > +.PP
> > > +If an
> > > +.BR EXT4_IOC_GETFSUUID
> > > +operation is called with
> > > +.I fsu_len
> > > +set to the number of bytes in an ext4 filesystem uuid and
> > > +.I fsu_uuid
> > > +is allocated at least that many bytes, then
> > > +the filesystem uuid will be written to
> > > +.I fsu_uuid.
> >
> > Hm. It's not like the kernel actually checks the allocation -- if
> > fsu_len is set to the length of the filesystem's volume uuid, then
> > the that volume uuid will be written to fsu_uuid[]. How about:
> >
> > "If EXT4_IOC_GETFSUUID is called with fsu_len matching the length of the
> > ext4 filesystem uuid, then that uuid will be written to fsu_uuid[] and
> > the return value will be zero.
> > If fsu_len does not match, the return value will be -EINVAL."
> >
> > > +.PP
> > > +If an
> > > +.BR EXT4_IOC_SETFSUUID
> > > +operation is called with
> > > +.I fsu_len
> > > +set to the number of bytes in an ext4 filesystem uuid and
> > > +.I fsu_uuid
> > > +contains a uuid with
> >
> > Nit: whitespace at EOL.
> >
> > > +.I fsu_uuid
> > > +bytes, then
> > > +the filesystem uuid will be set to
> > > +.I fsu_uuid.
> >
> > "If EXT4_IOC_SETFSUUID is called with fsu_len matching the length of the
> > ext4 filesystem uuid, then the filesystem uuid will be set to the
> > contents of fsu_uuid[] and the return value will reflect the outcome of
> > the update operation.
> > If fsu_len does not match, the return value will be -EINVAL."
> >
> > > +.PP
> > > +The
> > > +.B FS_IOC_SETFSUUID
> > > +operation requires privilege
> > > +.RB ( CAP_SYS_ADMIN ).
> > > +If the filesystem is currently being resized, an
> > > +.B EXT4_IOC_SETFSUUID
> > > +operation will wait until the resize is finished and the uuid can safely be set.
> > > +This may take a long time.
> >
> > Why is resize called out here specifically? Won't setfsuuid block on
> > /any/ operation that has tied up the filesystem superblocks? I think
> > this could be more general:
> >
> > "If the filesystem is busy, an EXT4_IOC_SETFSUUID operation will wait
> > until it can apply the uuid changes.
> > This may take a long time."
> >
> > > +.PP
> > > +.SH RETURN VALUE
> > > +On success zero is returned.
> > > +On error, \-1 is returned, and
> > > +.I errno
> > > +is set to indicate the error.
> > > +.SH ERRORS
> > > +Possible errors include (but are not limited to) the following:
> > > +.TP
> > > +.B EFAULT
> > > +Either the pointer to the
> > > +.I fsuuid
> > > +structure is invalid or
> > > +.I fsu_uuid
> > > +has not been initialized properly.
> >
> > Invalid? Isn't that what EINVAL is for?
> >
> > I think EFAULT is for "could not copy to/from userspace".
> >
> > > +.TP
> > > +.B EINVAL
> > > +The specified arguments are invalid.
> > > +.I fsu_len
> > > +did not match the filesystem uuid length or
> > > +.I fsu_flags
> > > +has bits set that are not implemented.
> >
> > "...not recognized."
> >
> > If they're not implemented, shouldn't that be EOPNOTSUPP?
> >
> > --D
> >
> > > +.TP
> > > +.B ENOTTY
> > > +The filesystem does not support the ioctl.
> > > +.TP
> > > +.B EOPNOTSUPP
> > > +The filesystem does not currently support changing the uuid through this
> > > +ioctl. This may be due to incompatible feature flags.
>
> Please see the following paragraph from man-pages(7):
> Use semantic newlines
> In the source of a manual page, new sentences should be
> started on new lines, long sentences should be split into
> lines at clause breaks (commas, semicolons, colons, and
> so on), and long clauses should be split at phrase bound‐
> aries. This convention, sometimes known as "semantic
> newlines", makes it easier to see the effect of patches,
> which often operate at the level of individual sentences,
> clauses, or phrases.
Agreed.
--D
>
> Cheers,
>
> Alex
>
>
> > > +.TP
> > > +.B EPERM
> > > +The calling process does not have sufficient permissions to set the uuid.
> > > +.SH CONFORMING TO
> > > +This API is Linux-specific.
> > > +.SH SEE ALSO
> > > +.BR ioctl (2)
> > > --
> > > 2.37.0.170.g444d1eabd0-goog
> > >
