Re: [PATCH v3] mount_setattr.2: New manual page documenting the mount_setattr() system call

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Christian,

This time I'm going to point out issues about the contents only, not groff fixes nor even punctuation fixes. I'll fix those myself and CC you when I do that.

However, if you render the page yourself (man ./mount_setattr.2), you will probably notice some formatting issues.

Please see some comments below.

Thanks,

Alex

On 7/31/21 12:15 PM, Christian Brauner wrote:

+.SH ERRORS
+.TP
+.B EBADF
+.I dfd
+is not a valid file descriptor.
+.TP
+.B EBADF
+An invalid file descriptor value was specified in
+.I userns_fd.

Why a different wording compared to the above?  Aren't they the same?

userns_fd is not a valid descriptor.

That would be consistent with the first EBADF, and would keep the meaning, right?

+.TP
+.B EBUSY
+The caller tried to change the mount to
+.BR MOUNT_ATTR_RDONLY
+but the mount had writers.

This is not so clear. I think I understood it, but maybe using language similar to that of mount(2) is clearer:

EBUSY source cannot be remounted read‐only, because it still holds files open for writing.

Something like?:

The caller tried to change the mount to MOUNT_ATTR_ONLY but the mount still has files open for writing



+static const struct option longopts[] = {
+    {"map-mount",       required_argument,  0,  'a'},
+    {"recursive",       no_argument,        0,  'b'},
+    {"read-only",       no_argument,        0,  'c'},
+    {"block-setid",     no_argument,        0,  'd'},
+    {"block-devices",   no_argument,        0,  'e'},
+    {"block-exec",      no_argument,        0,  'f'},
+    {"no-access-time",  no_argument,        0,  'g'},
+    { NULL,             0,                  0,   0 },
+};

The third field is an 'int *' (https://www.gnu.org/software/libc/manual/html_node/Getopt-Long-Options.html). Please, use NULL instead of 0.



+    struct mount_attr *attr = &(struct mount_attr){};

Wow!  Interesting usage of compound literals.
I had to check that this has automatic storage duration (I would have said that it has static s.d., but no).

I'm curious: why use that instead of just?:

struct mount_attr attr = {0};


+    if (ret < 0)
+        exit_log("%m - Failed to change mount attributes\en");


Although I typically use myself that same < 0 check,
manual pages typically use == -1 when a function returns -1 on error (which most syscalls do), and Michael prefers that.



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



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux