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 <supri...@in.ibm.com>
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
