Date: Wed, 9 Nov 2005 18:15:20 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Nate Lawson <nate@root.org> Cc: Craig Rodrigues <rodrigc@FreeBSD.org>, cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/kern vfs_mount.c Message-ID: <20051109173930.T67931@delplex.bde.org> In-Reply-To: <43716BD6.5050604@root.org> References: <20051109022714.7710216A434@hub.freebsd.org> <43716BD6.5050604@root.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20051109173930.T67931>