See the manpage addition for rationale
Details
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; | |
| 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 |
I now use this error code to fill errmsg at the call site.
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. | |
| 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. | |
| 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 |
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. | |