Message-ID: <e1573002-7ea3-2636-b2d2-331767a5622f@gmail.com>
Date:   Fri, 22 Jul 2022 12:03:23 +0200
From:   "Alejandro Colomar (man-pages)" <alx.manpages@...il.com>
To:     "Darrick J. Wong" <djwong@...nel.org>
Cc:     Jeremy Bongio <bongiojp@...il.com>, 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 Darrick,

On 7/21/22 20:12, Darrick J. Wong wrote:
> 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@...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.
> 
> 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.
> 

Okay.  Then may I ask for an EXAMPLES section with a program that 
unequivocally shows users how to use it?

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

I prefer types in separate pages, as the information is organized, and 
also because it allows to `man 2type fsuuid` if you want to see 
information about the type without knowing what the type is for (e.g., 
you've seen the type in some random code, maybe far from its 
corresponding ioctl(2) use, and want to understand it).

But `man fsuuid` can also be accomplished if we add a link page (see 
e.g. int8_t.3type for how to do it), if you prefer that.

Thanks,

Alex

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

-- 
Alejandro Colomar
Linux man-pages comaintainer; http://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

Powered by blists - more mailing lists