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

[Kevin Wolf]

Am 17.06.2011 18:38, schrieb Supriya Kannery:
> 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 parameters, can be integrated 
> in similar lines.
> 
> Signed-off-by: Supriya Kannery <address@hidden>

Coding style is off in this one as well.

> Index: qemu/blockdev.c
> ===================================================================
> --- qemu.orig/blockdev.c
> +++ qemu/blockdev.c
> @@ -797,3 +797,35 @@ 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)
> +{
> +     const char *device = qdict_get_str(qdict, "device");
> +     const char *name = qdict_get_str(qdict, "name");
> +     int enable = qdict_get_bool(qdict, "enable");
> +     BlockDriverState *bs;
> +
> +     bs = bdrv_find(device);
> +     if (!bs) {
> +             qerror_report(QERR_DEVICE_NOT_FOUND, device);
> +             return -1;
> +     }
> +
> +     if (!(strcmp(name, "hostcache"))) {

The bracket after ! isn't necessary.

> +             if (bdrv_is_inserted(bs)) {
> +                     /* cache change applicable only if device inserted */
> +                     return bdrv_change_hostcache(bs, enable);
> +             } else {
> +                     qerror_report(QERR_DEVICE_NOT_INSERTED, device);
> +                     return -1;
> +             }

I'm not so sure about this one. Why shouldn't I change the cache mode
for a device which is currently? The next thing I want to do could be
inserting a medium and using it with the new cache mode.

> +     }
> +
> +     return 0;
> +}
> +
> Index: qemu/block.c
> ===================================================================
> --- qemu.orig/block.c
> +++ qemu/block.c
> @@ -651,6 +651,33 @@ unlink_and_fail:
>      return ret;
>  }
>  
> +int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
> +{
> +     BlockDriver *drv = bs->drv;
> +     int ret = 0;
> +
> +     /* No need to reopen as no change in flags */
> +     if (bdrv_flags == bs->open_flags)
> +             return 0;

There could be other reasons for reopening besides changing flags, e.g.
invalidating cached metadata.

> +
> +     /* Quiesce IO for the given block device */
> +     qemu_aio_flush();
> +     bdrv_flush(bs);

Missing error handling.

> +
> +     bdrv_close(bs);

Here, too.

> +     ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
> +
> +     /*
> +     * A failed attempt to reopen the image file must lead to 'abort()'
> +     */
> +     if (ret != 0) {
> +             qerror_report(QERR_REOPEN_FILE_FAILED, bs->filename);
> +             abort();
> +     }

Maybe we can retry with the old flags at least before aborting?

Also I would like to see a (Linux specific) version that uses the old fd
for the reopen, so that we can handle files that aren't accessible with
their old name any more. This would mean adding a .bdrv_reopen callback
in raw-posix.

> +
> +     return ret;
> +}
> +
>  void bdrv_close(BlockDriverState *bs)
>  {
>      if (bs->drv) {
> @@ -691,6 +718,20 @@ 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;
> +
> +     /* Reopen file with changed set of flags */
> +     return bdrv_reopen(bs, bdrv_flags);
> +}

Hm, interesting. Now we can get a O_DIRECT | O_SYNC mode with the
monitor. We should probably expose the same functionality for the
command line, too.

Kevin