On Tue, 8 Nov 2005, Nate Lawson wrote:
Craig Rodrigues wrote:
rodrigc 2005-11-09 02:26:38 UTC
FreeBSD src repository
Modified files:
sys/kern vfs_mount.c Log:
For nmount(), allow a text string error message to be propagated back
to user-space if a parameter named "errmsg" is passed into the iovec.
Used in conjunction with vfs_mount_error(), more useful error messages
than errno can be passed back to userspace when mounting a filesystem
fails.
Discussed with: phk, pjd
Revision Changes Path
1.199 +37 -2 src/sys/kern/vfs_mount.c
While I don't have ideas for a better general mechanism for this, I think it
sets a bad precedent. We can't have every complex syscall transporting its
own error message strings back to the user program. And we can't expand errno
to be the union of every single API-specific error either.
I agree.
Other comments below.
You pointed out mounds of style bugs but missed a few.
Index: src/sys/kern/vfs_mount.c
diff -u src/sys/kern/vfs_mount.c:1.198 src/sys/kern/vfs_mount.c:1.199
Unnecessary cast, variable declared in fn body. Why not just use
auio->uio_iov[i].iov_base in the strcmp directly?
+ if (!strcmp(name, "errmsg")) {
+ copyout(auio->uio_iov[i+1].iov_base,
+ uap->iovp[i+1].iov_base, uap->iovp[i+1].iov_len);
+
Extra empty line above.
!strcmp() is an especially obfuscated boolean operation on a non-boolean.
Missing spaces around binary operator `+'.
char *fstype, *fspath;
int error, fstypelen, fspathlen;
+ int i;
Different style for new declaration (not with the other 3 ints).
@@ -480,12 +499,18 @@
error = vfs_getopt(optlist, "fstype", (void **)&fstype, &fstypelen);
if (error || fstype[fstypelen - 1] != '\0') {
error = EINVAL;
+ if (iov_errmsg)
Implicit comparison of pointer with NULL.
+ strncpy((char *)iov_errmsg->iov_base, "Invalid
fstype",
+ iov_errmsg->iov_len);
Is iov_len >= strlen(iov_errmsg->iov_base)? If not, it won't copy the
terminating NUL. Also, if iov_len is big, strncpy wastes time zero-filling
the tail. Perhaps use strlcpy instead.
Here the string is "Invalid fstype". The user buffer should be large
enough to hold all possible messages, but might not be. The error
handling for when it has a silly size is not very important.
I don't quite understand what iov_base points to. I think it is malloced,
but without M_ZERO, and it is copied to userland in the copyout() above.
Since the latter uses iov_len and not strlen(), the residue must be filled
somehow to prevent leaking kernel context, and strncpy() is the easiest
way to do this.
@@ -503,6 +528,17 @@
error = vfs_domount(td, fstype, fspath, fsflags, optlist);
mtx_unlock(&Giant);
bail:
+ if (error && iov_errmsg != NULL) {
+ /* save the errmsg */
+ char *errmsg;
+ int len, ret;
Variables declared in fn body.
Implicit comparison of non-boolean integer with 0.
Comment not an English sentence.
Comment not attached to code.
+ }
+ if (error)
vfs_freeopts(optlist);
return (error);
Implicit comparison of non-boolean integer with 0.
Indentation makes no sense (are some lines missing?).
Bruce
_______________________________________________
cvs-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/cvs-all
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
