Message-ID: <e503645b-e665-50c4-37a9-cdc8637ba1d8@gmail.com>
Date:   Thu, 21 Jul 2022 15:04:23 +0200
From:   Alejandro Colomar <alx.manpages@...il.com>
To:     "Darrick J. Wong" <djwong@...nel.org>,
        Jeremy Bongio <bongiojp@...il.com>
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.

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

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.

> 
>> +.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, ...)?

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

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

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

Powered by blists - more mailing lists