Date:   Thu, 21 Jul 2022 16:13:20 -0700
From:   Jeremy Bongio <bongiojp@...il.com>
To:     "Darrick J. Wong" <djwong@...nel.org>
Cc:     Ted Tso <tytso@....edu>, linux-ext4@...r.kernel.org,
        linux-man@...r.kernel.org
Subject: Re: [PATCH v2] Add manpage for get/set fsuuid ioctl for ext4 filesystem.

On Wed, Jul 20, 2022 at 5:12 PM Darrick J. Wong <djwong@...nel.org> wrote:
>
> On Wed, Jul 20, 2022 at 04:45:12PM -0700, Jeremy Bongio wrote:
> > Signed-off-by: Jeremy Bongio <bongiojp@...il.com>
> > ---
> >
> > 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@...il.com>
> > +.\"
> > +.\" 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.
>
> > +.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 ");"
> > +.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 {
> > +       __u32 fsu_len;      /* Number of bytes in a uuid */
> > +       __u32 fsu_flags;    /* Mapping flags */
> > +       __u8  fsu_uuid[];   /* Byte array for uuid */
> > +};
> > +.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?

I see some ioctls that use EINVAL when an ioctl's flag is not recognized,
but yes, EOPNOTSUPP looks more common.

>
> --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.
> > +.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
> >

Powered by blists - more mailing lists