[PATCH v2] Add manpage for get/set fsuuid ioctl for ext4 filesystem.

[PATCH v2] Add manpage for get/set fsuuid ioctl for ext4 filesystem.

[Jeremy Bongio]

Signed-off-by: Jeremy Bongio <bongiojp@gmail.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@gmail.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 )
+.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. 
+.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
+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.
+.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 
+.I fsu_uuid
+bytes, then
+the filesystem uuid will be set to
+.I fsu_uuid.
+.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.
+.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.
+.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.
+.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

Re: [PATCH v2] Add manpage for get/set fsuuid ioctl for ext4 filesystem.

[Darrick J. Wong]

On Wed, Jul 20, 2022 at 04:45:12PM -0700, Jeremy Bongio wrote:
> Signed-off-by: Jeremy Bongio <bongiojp@gmail.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@gmail.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?

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

Re: [PATCH v2] Add manpage for get/set fsuuid ioctl for ext4 filesystem.

[Alejandro Colomar]

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

Re: [PATCH v2] Add manpage for get/set fsuuid ioctl for ext4 filesystem.

[Darrick J. Wong]

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

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

Re: [PATCH v2] Add manpage for get/set fsuuid ioctl for ext4 filesystem.

[Alejandro Colomar (man-pages)]

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

Re: [PATCH v2] Add manpage for get/set fsuuid ioctl for ext4 filesystem.

[Theodore Ts'o]

On Fri, Jul 22, 2022 at 12:03:23PM +0200, Alejandro Colomar (man-pages) wrote:
> > 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?

I'll note that existing ioctl man pages don't have an explicit
statement that a libc is required --- nor do we do this for open(2),
stat(2), etc.   (And that's especially necessary for stat(2), BTW!)

Many of the ioctl man pages (or other system call man pages, for that
matter) also don't have an EXAMPLES section, either.

Perhaps it would be useful to have a discussion over what the
standards are for man pages in section 2, and when we need to state
things that seem to be rather obvious (like "you must have a C
library") and when there should be things like an EXAMPLES section?

Some the suggestions you are making don't seem to be adhered to by
the existing man pages, and more text is not always better.

https://www.npr.org/sections/13.7/2014/02/03/270680304/this-could-have-been-shorter

      	     	       	       		- Ted

Re: [PATCH v2] Add manpage for get/set fsuuid ioctl for ext4 filesystem.

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 3200 bytes --]

Hi Ted,

On 7/22/22 15:55, Theodore Ts'o wrote:
> On Fri, Jul 22, 2022 at 12:03:23PM +0200, Alejandro Colomar (man-pages) wrote:
>>> 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?
> 
> I'll note that existing ioctl man pages don't have an explicit
> statement that a libc is required --- nor do we do this for open(2),
> stat(2), etc.

That's because there hasn't been a man-pages release in around a year.
If you see the man-pages git repo, you'll see that (almost) all man 
pages in sections 2 and 3 have a new LIBRARY section.

ioctl(2) pages now have this LIBRARY section:
<https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/man2/ioctl_fat.2>

This was based on FreeBSD's man pages:
<https://www.freebsd.org/cgi/man.cgi?query=stat&apropos=0&sektion=2&manpath=FreeBSD+13.1-RELEASE+and+Ports&arch=default&format=html>

>   (And that's especially necessary for stat(2), BTW!)

stat(2) now says 
<https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/man2/stat.2#n22>:

LIBRARY
        Standard C library (libc, -lc)


If you would like to improve on that, I'm open to ideas, or patches from 
programmers who know the syscalls much better than I do.

> 
> Many of the ioctl man pages (or other system call man pages, for that
> matter) also don't have an EXAMPLES section, either.
> 
> Perhaps it would be useful to have a discussion over what the
> standards are for man pages in section 2, and when we need to state
> things that seem to be rather obvious (like "you must have a C
> library") and when there should be things like an EXAMPLES section?

Now that you say it, I forgot to document the LIBRARY section in 
man-pages(7).  There's something about it, but I forgot to add a 
paragraph describing it in detail.

Regarding the EXAMPLES section, every page in man2 or man3 should have 
an example program, IMO.  Consider that there are programmers that may 
find it easier to learn a function by experimenting with a working 
example of C code, rather than a dense textual description in a language 
that may not be native to the programmer.

There are many pages that lack examples, but that's not something I 
would consider a good thing.

> 
> Some the suggestions you are making don't seem to be adhered to by
> the existing man pages, and more text is not always better.

The next release of the man-pages is certainly going to be an important 
one.  It may be hated by many, loved by many others.  I hope overall I 
did a significant improvement in both improving the transmission of 
information and simplifying maintenance.

> 
> https://www.npr.org/sections/13.7/2014/02/03/270680304/this-could-have-been-shorter

Sorry, but I'm not able to read that page.  It prompts the usual GPDR 
notice, and doesn't give me the option to reject cookies (only accept).

Anyway, I guess what it says.  I hope I wasn't too much verbose with my 
many changes.

Cheers,

Alex

-- 
Alejandro Colomar
<http://www.alejandro-colomar.es/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] Add manpage for get/set fsuuid ioctl for ext4 filesystem.

[Darrick J. Wong]

On Fri, Jul 22, 2022 at 04:32:45PM +0200, Alejandro Colomar wrote:
> Hi Ted,
> 
> On 7/22/22 15:55, Theodore Ts'o wrote:
> > On Fri, Jul 22, 2022 at 12:03:23PM +0200, Alejandro Colomar (man-pages) wrote:
> > > > 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?
> > 
> > I'll note that existing ioctl man pages don't have an explicit
> > statement that a libc is required --- nor do we do this for open(2),
> > stat(2), etc.

I think you and I missed that discussion:
https://lore.kernel.org/linux-man/20210911160117.552617-1-alx.manpages@gmail.com/ 

> That's because there hasn't been a man-pages release in around a year.
> If you see the man-pages git repo, you'll see that (almost) all man pages in
> sections 2 and 3 have a new LIBRARY section.
> 
> ioctl(2) pages now have this LIBRARY section:
> <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/man2/ioctl_fat.2>
> 
> This was based on FreeBSD's man pages:
> <https://www.freebsd.org/cgi/man.cgi?query=stat&apropos=0&sektion=2&manpath=FreeBSD+13.1-RELEASE+and+Ports&arch=default&format=html>
> >   (And that's especially necessary for stat(2), BTW!)
> 
> stat(2) now says <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/man2/stat.2#n22>:
> 
> LIBRARY
>        Standard C library (libc, -lc)
> 
> 
> If you would like to improve on that, I'm open to ideas, or patches from
> programmers who know the syscalls much better than I do.

I still think it's redundant to say that you have to link against the
standard C library -- it's a standard feature on Linux, and you have to
pass -nolibc to opt out of it.  Libraries that have to be opted-into
(e.g. -lpthread) should be documented here, but not stuff that's enabled
by default.

Oh well, you're the maintainer, it is your prerogative.

> > Many of the ioctl man pages (or other system call man pages, for that
> > matter) also don't have an EXAMPLES section, either.
> > 
> > Perhaps it would be useful to have a discussion over what the
> > standards are for man pages in section 2, and when we need to state
> > things that seem to be rather obvious (like "you must have a C
> > library") and when there should be things like an EXAMPLES section?
> 
> Now that you say it, I forgot to document the LIBRARY section in
> man-pages(7).  There's something about it, but I forgot to add a paragraph
> describing it in detail.

That would've helped, since I scanned
https://man7.org/linux/man-pages/man7/man-pages.7.html
and didn't see much about what goes in this section...

> Regarding the EXAMPLES section, every page in man2 or man3 should have an
> example program, IMO.  Consider that there are programmers that may find it
> easier to learn a function by experimenting with a working example of C
> code, rather than a dense textual description in a language that may not be
> native to the programmer.

Frankly I'd rather push people to have example code over documenting
that standard C library functions require the standard C library. :)

That said, I don't always enjoy the textbook examples that have been
slimmed down for manpages -- I prefer a link to a real implementation
in (say) the test suite so that I can see code that (one would hope)
exercises all the functionality exposed through the interface.

But I guess that's really up to the manpage author to decide.

> There are many pages that lack examples, but that's not something I would
> consider a good thing.
> 
> > 
> > Some the suggestions you are making don't seem to be adhered to by
> > the existing man pages, and more text is not always better.
> 
> The next release of the man-pages is certainly going to be an important one.
> It may be hated by many, loved by many others.  I hope overall I did a
> significant improvement in both improving the transmission of information
> and simplifying maintenance.

I'm not convinced that having to open *two* manpages just to figure out
how to call an ioctl is going to simplify maintenance unless the struct
is shared across more than one manpage, but I've already made that
point.

(There isn't any magical way to #include a manpage within another
manpage, is there?)

--D

> 
> > 
> > https://www.npr.org/sections/13.7/2014/02/03/270680304/this-could-have-been-shorter
> 
> Sorry, but I'm not able to read that page.  It prompts the usual GPDR
> notice, and doesn't give me the option to reject cookies (only accept).
> 
> Anyway, I guess what it says.  I hope I wasn't too much verbose with my many
> changes.
> 
> Cheers,
> 
> Alex
> 
> -- 
> Alejandro Colomar
> <http://www.alejandro-colomar.es/>

Re: [PATCH v2] Add manpage for get/set fsuuid ioctl for ext4 filesystem.

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 4135 bytes --]

Hi Darrick,

On 7/22/22 19:46, Darrick J. Wong wrote:
>> Now that you say it, I forgot to document the LIBRARY section in
>> man-pages(7).  There's something about it, but I forgot to add a paragraph
>> describing it in detail.
> 
> That would've helped, since I scanned
> https://man7.org/linux/man-pages/man7/man-pages.7.html
> and didn't see much about what goes in this section...

These changes have been introduced after the last release was made, so 
even if I had documented it, it wouldn't have reached <man7.org>.  I'd 
recommend you to install the man pages from source (`sudo make install`).

> 
>> Regarding the EXAMPLES section, every page in man2 or man3 should have an
>> example program, IMO.  Consider that there are programmers that may find it
>> easier to learn a function by experimenting with a working example of C
>> code, rather than a dense textual description in a language that may not be
>> native to the programmer.
> 
> Frankly I'd rather push people to have example code over documenting
> that standard C library functions require the standard C library. :)

Agreed :)

> 
> That said, I don't always enjoy the textbook examples that have been
> slimmed down for manpages -- I prefer a link to a real implementation
> in (say) the test suite so that I can see code that (one would hope)
> exercises all the functionality exposed through the interface.
> 
> But I guess that's really up to the manpage author to decide.

They're not exclusive.  It's welcome to point to a (stable) link showing 
a more complex example after showing a simple example that fits the page.

> 
>> There are many pages that lack examples, but that's not something I would
>> consider a good thing.
>>
>>>
>>> Some the suggestions you are making don't seem to be adhered to by
>>> the existing man pages, and more text is not always better.
>>
>> The next release of the man-pages is certainly going to be an important one.
>> It may be hated by many, loved by many others.  I hope overall I did a
>> significant improvement in both improving the transmission of information
>> and simplifying maintenance.
> 
> I'm not convinced that having to open *two* manpages just to figure out
> how to call an ioctl is going to simplify maintenance unless the struct
> is shared across more than one manpage, but I've already made that
> point.

Well, I'd say it simplifies maintenance in the case that another page 
adds information about this type: when I receive a patch, I'm not 
grepping all of the pages to see if one already documents a type, to 
decide to move it to a separate page.  It's likely to be forgotten, and 
the documentation about the type duplicated (and they are likely to get 
out of sync).

When I added the type pages, I found many types to be documented 
differently on different pages, needing to copy parts of every page to 
get the full picture, because none of them was complete.  That's 
especially what I'm trying to avoid.

Still, there are many more important types to document in the type 
pages, and if you consider that this one is very unlikely to be shared 
in the long term, then I don't strongly oppose to it being in the same 
page as the ioctl that uses it for now.

> 
> (There isn't any magical way to #include a manpage within another
> manpage, is there?)

Oh, there is.  It's the groff(7) .so request, which is basically the 
same as C's #include directive.  We actually use it a lot in the 
man-pages, to create link pages (a page whose only content is a .so 
request, which basically means its contents are the same as another 
pages').  See for example:

$ cat man2/lstat.2
.so man2/stat.2


Technically, it could be used differently, to include a man page as part 
of another page, but it hasn't been done ever, as it would probably 
complicate how man pages are stored, indexed, and searched in the 
filesystem (there's no </usr/share/man/include/> or </usr/inlucde/man/> 
directory or something like that).

Cheers,

Alex

-- 
Alejandro Colomar
<http://www.alejandro-colomar.es/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] Add manpage for get/set fsuuid ioctl for ext4 filesystem.

[Jeremy Bongio]

On Wed, Jul 20, 2022 at 5:12 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Wed, Jul 20, 2022 at 04:45:12PM -0700, Jeremy Bongio wrote:
> > Signed-off-by: Jeremy Bongio <bongiojp@gmail.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@gmail.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
> >