cvs commit: src/sys/kern vfs_mount.c

[Nate Lawson]

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