From owner-cvs-all@FreeBSD.ORG Wed Nov 9 03:24:18 2005 Return-Path: X-Original-To: cvs-all@FreeBSD.org Delivered-To: cvs-all@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 2714F16A41F; Wed, 9 Nov 2005 03:24:18 +0000 (GMT) (envelope-from nate@root.org) Received: from www.cryptography.com (li-22.members.linode.com [64.5.53.22]) by mx1.FreeBSD.org (Postfix) with ESMTP id C23F043D46; Wed, 9 Nov 2005 03:24:13 +0000 (GMT) (envelope-from nate@root.org) Received: from [10.0.0.250] (ppp-71-139-0-107.dsl.snfc21.pacbell.net [71.139.0.107]) by www.cryptography.com (8.12.8/8.12.8) with ESMTP id jA93OBDI015459 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NOT); Tue, 8 Nov 2005 19:24:12 -0800 Message-ID: <43716BD6.5050604@root.org> Date: Tue, 08 Nov 2005 19:24:06 -0800 From: Nate Lawson User-Agent: Mozilla Thunderbird 1.0.6 (Windows/20050716) X-Accept-Language: en-us, en MIME-Version: 1.0 To: Craig Rodrigues , src-committers@FreeBSD.org, cvs-src@FreeBSD.org, cvs-all@FreeBSD.org References: <20051109022714.7710216A434@hub.freebsd.org> In-Reply-To: <20051109022714.7710216A434@hub.freebsd.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Subject: Re: cvs commit: src/sys/kern vfs_mount.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Nov 2005 03:24:18 -0000 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