Page MenuHomeFreeBSD

nmount: Introduce the "onto_fsid" option
Needs ReviewPublic

Authored by arrowd on Wed, Sep 23, 3:39 PM.
Tags
None
Referenced Files
F174212824: D59930.id187536.diff
Thu, Oct 1, 10:03 AM
F174205254: D59930.id187675.diff
Thu, Oct 1, 8:45 AM
F174169242: D59930.diff
Thu, Oct 1, 2:48 AM
F174158127: D59930.diff
Thu, Oct 1, 12:27 AM
F174132605: D59930.id187756.diff
Wed, Sep 30, 8:07 PM
F174132135: D59930.id187536.diff
Wed, Sep 30, 8:03 PM
F174116273: D59930.diff
Wed, Sep 30, 5:56 PM
F174102211: D59930.diff
Wed, Sep 30, 3:45 PM
Subscribers

Details

Summary

See the manpage addition for rationale

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 77211
Build 74094: arc lint + arc unit

Event Timeline

lib/libsys/mount.2
109
sys/kern/vfs_mount.c
816

I suggest to follow the existing style and initialize new vars in the block below.

983

Why do you need to strdup() the value? If you convert it into the fsid_t there, instead of doing it later, it should be good enough.

1723
1724

But I do not see a need to check for mp == NULL first.

if (mp != vp->v_mount) {
        error = ENOENT; /* Might be set the error string ? */
        vput(vp);
        vrele(nd.ni_dvp); /* Because of WANTPARENT */
}
 if (mp != NULL)
           vfs_rel(mp);
if (error != 0)
       goto out;
arrowd marked 2 inline comments as done.
  • Address comments
sys/kern/vfs_mount.c
983

strndup makes sure the value is NULL-terminated. vfs_buildopts makes sure that opt names are NULL-terminated, but copies values verbatim, as far as I understand.

There is no "snscanf" to pass the buffer length to it, so I'm using strndup to create a buffer that is 100% safe to pass to sscanf. Or am I being overly cautious here?

1724

error = ENOENT; /* Might be set the error string ? */

I now use this error code to fill errmsg at the call site.

vrele(nd.ni_dvp); /* Because of WANTPARENT */

vrele is already called in the "out" path.

lib/libsys/mount.2
111

I do not quite understand this sentence. Do you mean that there exists such requirement already (I do not think so)? Or do you mean that the patch introduces this requirement (again, seems to be not)?

sys/kern/vfs_mount.c
1037

ENOENT can be reported from vfs_domount() for other reasons, most likely due to non-existent fspath.

ziaee added inline comments.
lib/libsys/mount.2
94–96

This is an inappropriate use of column. Column requires a second width specifier for the second column. What you want for this usage is tag. This will render exactly the same as what you have now on terminal, except it won't be broken in the many other output formats. Also, the linter should complain.

I know, line 86 is broken too and this syntax was just copied from that, but we should not add a second fire.

ziaee requested changes to this revision.Wed, Sep 23, 9:12 PM
This revision now requires changes to proceed.Wed, Sep 23, 9:12 PM
arrowd marked an inline comment as done.
  • Address manpage comments
lib/libsys/mount.2
111
onto_fsid may also be passed when doing a mount update. In this case the caller should pass a FSID of the FS being updated, not the one it covers.

Would that be better?

sys/kern/vfs_mount.c
1037

I didn't use brain when choosing a error code. Do you have a suggestion for it?

lib/libsys/mount.2
111

Is it the same parameter name (I do not remember)? Then would it be better to choose a different name for the new parameter, they have quite different semantic.

sys/kern/vfs_mount.c
1037

May be pass &errmsg to vfs_domount() and make the function fill errmsg, to not depend on the specific error code?

lib/libsys/mount.2
111

Maybe change the parameter's name to fit both cases? How about check_fsid?

lib/libsys/mount.2
111

If the name for update is changed, it breaks existing usage. Just use the different parameter name for your case.

sys/kern/vfs_mount.c
1037

This code short-circuits to bail only in ENODEV and errno-to-be-chosen-for-check-fsid cases. We certainly can fill errmsg inside vfs_domount(), but we'll still need to have

if (error == ENODEV || error == ESOMETHINGELSE)
    goto bail;

So I don't think adding yet another argument to vfs_domount() worth it. Would do you think about filling check_fsid_p with -1s to signal a FSID-related error?

I'm beginning to think all these Li's in the manpage are actually Fa "Function argument or parameter".

lib/libsys/mount.2
90–91

patch currently fails to apply, but this won't work here. Roff requests have to happen on a line starting with a dot. maybe something like this. But I don't know that Li is really correct for these. Aren't these arguments for nmount()? I think that would make the appropriate markup Fa.

tests/sys/vfs/nmount_check_fsid.c
1–24 ↗(On Diff #187756)

We updated the preferred license for new files last year, take a look at style.9 or the license guide on freebsd.org.

lib/libsys/mount.2
90–91

Aren't these arguments for nmount()?

These are values for an argument. No idea if Fa is applicable for these.

lib/libsys/mount.2
96

FSID is not a property of a vnode, it is property of the mount point where the vnode is located.

112

I think this should be formulated much shorter.

State what the parameter does, instead of how to use it. Like

If check_fsid option is specified, and the FSID of the mount point where the covered vnode belongs to does not match the value of the option, the call fails with the error EXXX.

Then if really wanting you might describe the race but I do not see much point.

sys/kern/vfs_mount.c
815
1000
1004

But AFAIR sscanf() returns the number of parsed items. So for me it looks like you are discarding all correctly formatted parameter values.

1037

I do think that moving the filling of the errmsg string is better done in vfs_domount() when we know for sure what happen.
You may return any error code that does not trigger the vfs_should_downgrade_to_ro_mount() to try to remount the mp into ro. Then 'goto bail' or even any filtering for that error is not needed at all.