Date: Tue, 08 Nov 2005 19:24:06 -0800 From: Nate Lawson <nate@root.org> To: Craig Rodrigues <rodrigc@FreeBSD.org>, src-committers@FreeBSD.org, cvs-src@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/kern vfs_mount.c Message-ID: <43716BD6.5050604@root.org> In-Reply-To: <20051109022714.7710216A434@hub.freebsd.org> References: <20051109022714.7710216A434@hub.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Other comments below.
> 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
> --- src/sys/kern/vfs_mount.c:1.198 Tue Nov 8 04:13:39 2005
> +++ src/sys/kern/vfs_mount.c Wed Nov 9 02:26:38 2005
> @@ -390,6 +390,18 @@
> iov++;
> }
> error = vfs_donmount(td, uap->flags, auio);
> +
> + /* copyout the errmsg */
> + for (i = 0; (error != 0) && (i < iovcnt); i += 2) {
Extra parens.
> + const char *name = (const char *)auio->uio_iov[i].iov_base;
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.
> + break;
> + }
> + }
> +
> free(auio, M_IOV);
> return (error);
> }
> @@ -464,8 +476,15 @@
> vfs_donmount(struct thread *td, int fsflags, struct uio *fsoptions)
> {
> struct vfsoptlist *optlist;
> + struct iovec *iov_errmsg = NULL;
Initialization in declaration.
> char *fstype, *fspath;
> int error, fstypelen, fspathlen;
> + int i;
> +
> + for (i = 0; i < fsoptions->uio_iovcnt; i += 2) {
> + if (!strcmp((char *)fsoptions->uio_iov[i].iov_base, "errmsg"))
No need for a cast.
> + iov_errmsg = &fsoptions->uio_iov[i+1];
> + }
>
> error = vfs_buildopts(fsoptions, &optlist);
> if (error)
> @@ -480,12 +499,18 @@
> error = vfs_getopt(optlist, "fstype", (void **)&fstype, &fstypelen);
> if (error || fstype[fstypelen - 1] != '\0') {
> error = EINVAL;
> + if (iov_errmsg)
> + 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.
> goto bail;
> }
> fspathlen = 0;
> error = vfs_getopt(optlist, "fspath", (void **)&fspath, &fspathlen);
> if (error || fspath[fspathlen - 1] != '\0') {
> error = EINVAL;
> + if (iov_errmsg != NULL)
> + strncpy((char *)iov_errmsg->iov_base, "Invalid fspath",
> + iov_errmsg->iov_len);
> goto bail;
> }
>
> @@ -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.
> + ret = vfs_getopt(optlist, "errmsg", (void **)&errmsg, &len);
> + if(ret == 0 && len > 0)
space needed between if and ()
> + strncpy((char *)iov_errmsg->iov_base, errmsg,
> + iov_errmsg->iov_len);
> +
Extra empty line above. Unnecessary cast.
> + }
> +
> if (error)
> vfs_freeopts(optlist);
> return (error);
Hope this helps,
--
Nate
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?43716BD6.5050604>