Re: [Qemu-devel] [V5 Patch 3/4]Qemu: Command "block_set" for dynamic block params change

[Anthony Liguori <anthony@codemonkey.ws>]

On 07/27/2011 06:30 AM, Supriya Kannery wrote:
> New command "block_set" added for dynamically changing any of the block
> device parameters. For now, dynamic setting of hostcache params using this
> command is implemented. Other block device parameter changes, can be
> integrated in similar lines.
>
> Signed-off-by: Supriya Kannery<supriyak@in.ibm.com>
>
> ---
>   block.c         |   54 +++++++++++++++++++++++++++++++++++++++++++++++++
>   block.h         |    2 +
>   blockdev.c      |   61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   blockdev.h      |    1
>   hmp-commands.hx |   14 ++++++++++++
>   qemu-config.c   |   13 +++++++++++
>   qemu-option.c   |   25 ++++++++++++++++++++++
>   qemu-option.h   |    2 +
>   qmp-commands.hx |   28 +++++++++++++++++++++++++
>   9 files changed, 200 insertions(+)
>
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -651,6 +651,34 @@ unlink_and_fail:
>       return ret;
>   }
>
> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
> +{
> +    BlockDriver *drv = bs->drv;
> +    int ret = 0, open_flags;
> +
> +    /* Quiesce IO for the given block device */
> +    qemu_aio_flush();
> +    if (bdrv_flush(bs)) {
> +        qerror_report(QERR_DATA_SYNC_FAILED, bs->device_name);
> +        return ret;
> +    }
> +    open_flags = bs->open_flags;
> +    bdrv_close(bs);
> +
> +    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
> +    if (ret<  0) {
> +        /* Reopen failed. Try to open with original flags */
> +        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> +        ret = bdrv_open(bs, bs->filename, open_flags, drv);
> +        if (ret<  0) {
> +            /* Reopen failed with orig and modified flags */
> +            abort();
> +        }
> +    }
> +
> +    return ret;
> +}
> +
>   void bdrv_close(BlockDriverState *bs)
>   {
>       if (bs->drv) {
> @@ -691,6 +719,32 @@ void bdrv_close_all(void)
>       }
>   }
>
> +int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache)
> +{
> +    int bdrv_flags = bs->open_flags;
> +
> +    /* set hostcache flags (without changing WCE/flush bits) */
> +    if (enable_host_cache) {
> +        bdrv_flags&= ~BDRV_O_NOCACHE;
> +    } else {
> +        bdrv_flags |= BDRV_O_NOCACHE;
> +    }
> +
> +    /* If no change in flags, no need to reopen */
> +    if (bdrv_flags == bs->open_flags) {
> +        return 0;
> +    }
> +
> +    if (bdrv_is_inserted(bs)) {
> +        /* Reopen file with changed set of flags */
> +        return bdrv_reopen(bs, bdrv_flags);
> +    } else {
> +        /* Save hostcache change for future use */
> +        bs->open_flags = bdrv_flags;
> +        return 0;
> +    }
> +}
> +
>   /* make a BlockDriverState anonymous by removing from bdrv_state list.
>      Also, NULL terminate the device_name to prevent double remove */
>   void bdrv_make_anon(BlockDriverState *bs)
> Index: qemu/block.h
> ===================================================================
> --- qemu.orig/block.h
> +++ qemu/block.h
> @@ -71,6 +71,7 @@ void bdrv_delete(BlockDriverState *bs);
>   int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
>   int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
>                 BlockDriver *drv);
> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags);
>   void bdrv_close(BlockDriverState *bs);
>   int bdrv_attach(BlockDriverState *bs, DeviceState *qdev);
>   void bdrv_detach(BlockDriverState *bs, DeviceState *qdev);
> @@ -97,6 +98,7 @@ void bdrv_commit_all(void);
>   int bdrv_change_backing_file(BlockDriverState *bs,
>       const char *backing_file, const char *backing_fmt);
>   void bdrv_register(BlockDriver *bdrv);
> +int bdrv_change_hostcache(BlockDriverState *bs, bool enable_host_cache);
>
>
>   typedef struct BdrvCheckResult {
> Index: qemu/blockdev.c
> ===================================================================
> --- qemu.orig/blockdev.c
> +++ qemu/blockdev.c
> @@ -793,3 +793,64 @@ int do_block_resize(Monitor *mon, const
>
>       return 0;
>   }
> +
> +
> +/*
> + * Handle changes to block device settings, like hostcache,
> + * while guest is running.
> +*/
> +int do_block_set(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    BlockDriverState *bs = NULL;
> +    QemuOpts *opts;
> +    int enable;
> +    const char *device, *driver;
> +    int ret;
> +    char usage[50];
> +
> +    /* Validate device */
> +    device = qdict_get_str(qdict, "device");
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        qerror_report(QERR_DEVICE_NOT_FOUND, device);
> +        return -1;
> +    }
> +
> +    opts = qemu_opts_from_qdict(qemu_find_opts("device"), qdict);
> +    if (opts == NULL) {
> +        return -1;
> +    }
> +
> +    /* If input not in "param=value" format, display error */
> +    driver = qemu_opt_get(opts, "driver");
> +    if (driver != NULL) {
> +        qerror_report(QERR_INVALID_PARAMETER, driver);
> +        return -1;
> +    }
> +
> +    /* Before validating parameters, remove "device" option */
> +    ret = qemu_opt_delete(opts, "device");
> +    if (ret == 1) {
> +        strcpy(usage,"block_set device [prop=value][,...]");
> +        qerror_report(QERR_INCORRECT_COMMAND_SYNTAX, usage);
> +        return 0;
> +    }
> +
> +    /* Validate parameters with "-drive" parameter list */
> +    ret = qemu_validate_opts(opts, "drive");
> +    if (ret == -1) {
> +        return -1;
> +    }
> +
> +    /* Check for 'hostcache' parameter */
> +    enable = qemu_opt_get_bool(opts, "hostcache", -1);
> +    if (enable != -1) {
> +        return bdrv_change_hostcache(bs, enable);
> +    } else {
> +        qerror_report(QERR_INVALID_PARAMETER_VALUE, "hostcache","on/off");
> +    }
> +
> +    return 0;
> +
> +}
> +
> Index: qemu/blockdev.h
> ===================================================================
> --- qemu.orig/blockdev.h
> +++ qemu/blockdev.h
> @@ -65,5 +65,6 @@ int do_change_block(Monitor *mon, const
>   int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
>   int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
>   int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
> +int do_block_set(Monitor *mon, const QDict *qdict, QObject **ret_data);
>
>   #endif
> Index: qemu/hmp-commands.hx
> ===================================================================
> --- qemu.orig/hmp-commands.hx
> +++ qemu/hmp-commands.hx
> @@ -70,6 +70,20 @@ but should be used with extreme caution.
>   resizes image files, it can not resize block devices like LVM volumes.
>   ETEXI
>
> +    {
> +        .name       = "block_set",
> +        .args_type  = "device:B,device:O",
> +        .params     = "device [prop=value][,...]",
> +        .help       = "Change block device parameters [hostcache=on/off]",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = do_block_set,
> +    },
> +STEXI
> +@item block_set @var{config}
> +@findex block_set
> +Change block device parameters (eg: hostcache=on/off) while guest is running.
> +ETEXI
> +

block_set_hostcache() please.

Multiplexing commands is generally a bad idea.  It weakens typing.  In 
the absence of a generic way to set block device properties, 
implementing properties as generic in the QMP layer seems like a bad 
idea to me.

Regards,

Anthony Liguori