On Sun, Aug 01, 2021 at 03:47:53PM +0200, Alejandro Colomar (man-pages) wrote:
> 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
It's fine, I'm happy to do them if you point them out especially if
there need to be content changes because then I have rework anyway. I
just don't want to wade through other emails. :)
> do that.
>
> However, if you render the page yourself (man ./mount_setattr.2), you will
> probably notice some formatting issues.
I'll obviously fix them if I see them. :)
>
> 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?
Sounds good.
>
> > +.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
Sounds good.
>
>
> >
> > +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.
ok
>
>
> >
> > + 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};
I like the ability to directly pass "attr" as pointer and I also like
that I can use "attr->" instead of "attr.". It's entirely convenience. :)
>
> >
> > + 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.
Sure, will change for mount_setattr() and move_mount() calls. I'd leave
the open_tree() and open().
Thanks!
Christian
