[REVIEW] patch for copyout on ioctl error

[Martin Matuska]

On 9. 4. 2013 0:30, Pawel Jakub Dawidek wrote:
> 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.
>
Thanks for the review, I will integrate the recommendations.

Martin

-- 
Martin Matuska
FreeBSD committer
http://blog.vx.sk