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

[Stefan Hajnoczi <stefanha@gmail.com>]

On Wed, Jul 13, 2011 at 06:37:05PM +0530, Supriya Kannery wrote:
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -651,6 +651,40 @@ unlink_and_fail:
>      return ret;
>  }
> 
> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
> +{
> +    BlockDriver *drv = bs->drv;
> +    int ret = 0;
> +
> +    /* 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;
> +    }
> +    bdrv_close(bs);
> +
> +
> +    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
> +    if (ret < 0) {
> +        /* Reopen failed. Try to open with original flags */
> +        error_report("Opening file with changed flags...");
> +        qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);

If the next open fails too then there will be two qerror_report() calls.
This causes a warning and the new qerror is dropped.  Please consolidate
the error reporting so there is only one error before returning from
this function.

> +
> +        ret = bdrv_open(bs, bs->filename, bs->open_flags, drv);

bs->open_flags has been clobbered by the previous bdrv_open().  It would
be best to take a copy of bs->open_flags before bdrv_close(bs) above.

> +        if (ret < 0) {
> +            /*
> +             * Reopen failed with orig and modified flags
> +            */
> +            error_report("Opening file with original flags...");
> +            qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> +            abort();
> +        }
> +    }
> +
> +    return ret;
> +}
> +
>  void bdrv_close(BlockDriverState *bs)
>  {
>      if (bs->drv) {
> @@ -691,6 +725,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;

Can you explain the scenario where this works?

Looking at do_change_block() the flags will be clobbered so saving them
away does not help.

> Index: qemu/blockdev.c
> ===================================================================
> --- qemu.orig/blockdev.c
> +++ qemu/blockdev.c
> @@ -793,3 +793,63 @@ 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;
> +
> +    /* 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) {
> +        error_report("Invalid parameter %s", driver);

error_report() only works for HMP.  Please use qerror_report() so both
HMP and QMP see the error.  Same issue further down.

Stefan