[REVIEW] patch for copyout on ioctl error

[Pawel Jakub Dawidek]

On Sun, Apr 07, 2013 at 09:29:08PM +0200, Martin Matuska wrote:
> Hi all,
> 
> ZFS expects a copyout of zfs_cmd_t on a ioctl error. Our sys_ioctl()
> doesn't copyout in this case.
> 
> I am suggesting the attached patch for review to fix (or workaround)
> this situation.
> 
> The idea is easy - zfs ioctl's won't send zfs_cmd_t anymore but a new
> struct zfs_iocparm_t consisting of just three elements:
> pointer to zfs_cmd_t
> size of zfs_cmd_t
> zfs_ioctl_version
> 
> Size of zfs_cmd_t is intended for the kernel to re-verify if talking to
> the correct version. zfs_ioctl_version is for future purposes to make it
> easier for the
> kernel to identify ioctl changes.
> 
> The kernel module will do the copyin/copyout of zfs_cmd_t itself (like
> illumos does). This makes future compatibility more simple and
> zfsdev_ioctl() behaves much more like in illumos. The patch retains
> compatibility with all previous states.

I wonder if this is not the right time to add a IOC_ flag to natively
support such behaviour in FreeBSD's ioctl. Not sure how much would that
help to reduce complexity. If not much, then it is probably not worth it.

The patch looks good, overall. One comment below.

> Index: sys/cddl/contrib/opensolaris/common/zfs/zfs_ioctl_compat.c
> ===================================================================
> --- sys/cddl/contrib/opensolaris/common/zfs/zfs_ioctl_compat.c	(revision 249226)
> +++ sys/cddl/contrib/opensolaris/common/zfs/zfs_ioctl_compat.c	(working copy)
> @@ -575,11 +575,21 @@ int
>  zcmd_ioctl_compat(int fd, int request, zfs_cmd_t *zc, const int cflag)
>  {
>  	int nc, ret;
> +	zfs_iocparm_t *zfs_iocparm;
>  	void *zc_c;
>  	unsigned long ncmd;
>  
>  	switch (cflag) {
>  	case ZFS_CMD_COMPAT_NONE:
> +		ncmd = _IOWR('Z', request, struct zfs_iocparm);
> +		zfs_iocparm = malloc(sizeof(zfs_iocparm_t));

There is no check if malloc(3) succeeded, but actually would suggest
eliminating malloc(3) altogether and allocating zfs_iocparm on the
stack...

> +		zfs_iocparm->zfs_cmd_size = sizeof(zfs_cmd_t);
> +		zfs_iocparm->zfs_ioctl_version = ZFS_IOCVER_CURRENT;
> +		ret = ioctl(fd, ncmd, zfs_iocparm);
> +		free(zfs_iocparm);
> +		return (ret);

...Here you could just: return (ioctl(fd, ncmd, &zfs_iocparm));

PS. In case we decide to grow zfs_iocparam structure in the future it
    would be better to place zfs_ioctl_version as the first field.

-- 
Pawel Jakub Dawidek                       http://www.wheelsystems.com
FreeBSD committer                         http://www.FreeBSD.org
Am I Evil? Yes, I Am!                     http://tupytaj.pl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/zfs-devel/attachments/20130409/d088577c/attachment.sig>