[PATCH] Consistently use zfs_ioctl()

[Martin Matuska]

Dn(a 26. 7. 2011 15:21, Pawel Jakub Dawidek  wrote / napísal(a):
> On Mon, Jul 25, 2011 at 01:26:36PM -0700, Xin LI wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA256
>>
>> Hi,
>>
>> Here is a patch that makes libzfs use zfs_ioctl consistently. Some
>> functionality in libzfs calls ioctl() directly with raw ZFS ioctls,
>> which causes some kernel message like:
>>
>> WARNING pid 30031 (initial thread): ioctl sign-extension ioctl
>> ffffffffd5985a3a
>>
>> Objections? I have kept #sun'ed out blocks out.
>
> zfs_ioctl() does something with history records, I think. Are you sure
> zfs_ioctl() is 1-to-1 equivalent of plain ioctl()?
>
> Some minor nits inline.
Yes, zfs_ioctl deals with history and calls ioctl the very same way:
cddl/contrib/opensolaris/lib/libzfs/common/libzfs_util.c:
int
zfs_ioctl(libzfs_handle_t *hdl, unsigned long request, zfs_cmd_t *zc)
{
        int error;
 
        zc->zc_history = (uint64_t)(uintptr_t)hdl->libzfs_log_str;
        error = ioctl(hdl->libzfs_fd, request, zc);
        if (hdl->libzfs_log_str) {
                free(hdl->libzfs_log_str);
                hdl->libzfs_log_str = NULL;
        }
        zc->zc_history = 0;
 
        return (error);
}
 
Now to ioctl(): we are not calling ioctl directly. Ioctl() is actually
zcmd_ioctl() - see bottom of the following code:
 
cddl/contrib/opensolaris/lib/libzfs/common/libzfs_impl.h:
static int zfs_kernel_version = 0;
 
/*
 * This is FreeBSD version of ioctl, because Solaris' ioctl() updates
 * zc_nvlist_dst_size even if an error is returned, on FreeBSD if an
 * error is returned zc_nvlist_dst_size won't be updated.
 */
static __inline int
zcmd_ioctl(int fd, unsigned long cmd, zfs_cmd_t *zc)
{
        size_t oldsize, zfs_kernel_version_size;
        int version, ret, cflag = ZFS_CMD_COMPAT_NONE;
 
        zfs_kernel_version_size = sizeof(zfs_kernel_version);
        if (zfs_kernel_version == 0) {
                sysctlbyname("vfs.zfs.version.spa", &zfs_kernel_version,
                    &zfs_kernel_version_size, NULL, 0);
        }
 
        if (zfs_kernel_version == SPA_VERSION_15 ||
            zfs_kernel_version == SPA_VERSION_14 ||
            zfs_kernel_version == SPA_VERSION_13)
                cflag = ZFS_CMD_COMPAT_V15;
 
        oldsize = zc->zc_nvlist_dst_size;
        ret = zcmd_ioctl_compat(fd, cmd, zc, cflag);
 
        if (ret == 0 && oldsize < zc->zc_nvlist_dst_size) {
                ret = -1;
                errno = ENOMEM;
        }
 
        return (ret);
}
#define ioctl(fd, cmd, zc)      zcmd_ioctl((fd), (cmd), (zc))
 
This function calls my zcmd_ioctl_compat in:
sys/cddl/contrib/opensolaris/common/zfs/zfs_ioctl_compat.c
 
Probably there is a problem in this function (my compat code)? But for
ZFS_CMD_COMPAT_NONE it is just passed to the kernel as it was before.
 
>
>
>> Index: cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.c
>> ===================================================================
>> --- cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.c
(revision 224337)
>> +++ cddl/contrib/opensolaris/lib/libzfs/common/libzfs_dataset.c
(working copy)
>> @@ -2382,6 +2382,7 @@ zfs_prop_get_userquota_common(zfs_handle_t *zhp, c
>> {
>> int err;
>> zfs_cmd_t zc = { 0 };
>> + libzfs_handle_t *hdl = zhp->zfs_hdl;
>>
>> (void) strncpy(zc.zc_name, zhp->zfs_name, sizeof (zc.zc_name));
>>
>> @@ -2392,7 +2393,7 @@ zfs_prop_get_userquota_common(zfs_handle_t *zhp, c
>> if (err)
>> return (err);
>>
>> - err = ioctl(zhp->zfs_hdl->libzfs_fd, ZFS_IOC_USERSPACE_ONE, &zc);
>> + err = zfs_ioctl(hdl, ZFS_IOC_USERSPACE_ONE, &zc);
>
> Do we really need additional variable that is used only once?
> Wouldn't it be better to just replace ioctl(zhp->zfs_hdl->libzfs_fd)
> with zfs_ioctl(zhp->zfs_hdl)?
I agree, we don't and just for one use we waste resources.
>
>> @@ -2458,11 +2459,12 @@ zfs_do_list_ioctl(zfs_handle_t *zhp, unsigned
long
>> {
>> int rc;
>> uint64_t orig_cookie;
>> + libzfs_handle_t *hdl = zhp->zfs_hdl;
>>
>> orig_cookie = zc->zc_cookie;
>> top:
>> (void) strlcpy(zc->zc_name, zhp->zfs_name, sizeof (zc->zc_name));
>> - rc = ioctl(zhp->zfs_hdl->libzfs_fd, arg, zc);
>> + rc = zfs_ioctl(hdl, arg, zc);
>
> Same here.
>
>> @@ -3924,6 +3926,7 @@ zfs_userspace(zfs_handle_t *zhp, zfs_userquota_pro
>> zfs_userspace_cb_t func, void *arg)
>> {
>> zfs_cmd_t zc = { 0 };
>> + libzfs_handle_t *hdl = zhp->zfs_hdl;
>> int error;
>> zfs_useracct_t buf[100];
>>
>> @@ -3937,7 +3940,7 @@ zfs_userspace(zfs_handle_t *zhp, zfs_userquota_pro
>> zfs_useracct_t *zua = buf;
>>
>> zc.zc_nvlist_dst_size = sizeof (buf);
>> - error = ioctl(zhp->zfs_hdl->libzfs_fd,
>> + error = zfs_ioctl(hdl,
>> ZFS_IOC_USERSPACE_MANY, &zc);
>
> And here.
>
 
 
-- 
Martin Matuska
FreeBSD committer
http://blog.vx.sk