[Qemu-devel] [V5 Patch 0/4]Qemu: Set host cache from cmdline and monitor

[Qemu-devel] [V5 Patch 0/4]Qemu: Set host cache from cmdline and monitor

[Supriya Kannery]

  Currently cache setting of a block device cannot be changed 
without restarting a running VM. Following patchset is for 
enabling dynamic change of host cache setting for block devices 
through qemu monitor. Code changes are based on patches 
from Christoph Hellwig and Prerna Saxena.

  This patchset introduces monitor command 'block_set'
using which various parameters for block devices can be 
changed dynamically. Dynamic changing of host cache 
setting is implemented for now. This patchset also 
introduces 'hostcache', a new option for setting host
cache from qemu command line. 'Hostcache and 'cache' options 
cannot be used simultaneously from commandline.

Changes from patchset V4:
 1. Defined a new qerror class for incorrect command
    syntax.
 2. Changed some of the error_report() calls to
    qerror_report() calls


New block command added:
"block_set" 
    -- Sets block device parameters while guest is running.
Usage:
 block_set <device> <param> <value> 
   <device> = block device
   <param>  = parameter (say, "hostcache")
   <value>  = on/off

New 'hostcache' option added to -drive:
 -drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
        ....    
"       [,readonly=on|off][,hostcache=on|off]\n"

 1/4 Enhance "info block" to display hostcache setting
 2/4 New qerrors for file reopen, data sync and command syntax
 3/4 Command "block_set" for dynamic params change for block device
 4/4 'hostcache' option added to -drive in qemu commandline

 qemu/block.c         |   75 +++++++++++++++++++++++++++++++++++++++++++++
 qemu/block.h         |    2 +
 qemu/blockdev.c      |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu/blockdev.h      |    1
 qemu/hmp-commands.hx |   14 +++++++++++
 qemu/qemu-config.c   |   17 ++++++++++
 qemu/qemu-option.c   |   25 ++++++++++++++++++++
 qemu/qemu-option.h   |    2 +
 qemu/qemu-options.hx |    2 -
 qemu/qerror.c        |   12 ++++++++++
 qemu/qerror.h        |    8 ++++++
 qemu/qmp-commands.hx |   30 +++++++++++++++++++++++
 12 files changed, 257 insertions(+), 2 deletions(-)
~                                                   

[Qemu-devel] [V5 Patch 1/4]Qemu: Enhance "info block" to display host cache setting

[Supriya Kannery]

Enhance "info block" to display hostcache setting for each
block device.

Example:
(qemu) info block
ide0-hd0: type=hd removable=0 file=../rhel6-32.qcow2 ro=0 drv=qcow2
encrypted=0

Enhanced to display "hostcache" setting:
(qemu) info block
ide0-hd0: type=hd removable=0 hostcache=true file=../rhel6-32.qcow2
ro=0 drv=qcow2 encrypted=0

Signed-off-by: Supriya Kannery <supriyak@in.ibm.com>

---
 block.c         |   21 +++++++++++++++++----
 qmp-commands.hx |    2 ++
 2 files changed, 19 insertions(+), 4 deletions(-)

Index: qemu/block.c
===================================================================
--- qemu.orig/block.c
+++ qemu/block.c
@@ -1713,6 +1713,14 @@ static void bdrv_print_dict(QObject *obj
         monitor_printf(mon, " locked=%d", qdict_get_bool(bs_dict, "locked"));
     }
 
+    if (qdict_haskey(bs_dict, "open_flags")) {
+        int open_flags = qdict_get_int(bs_dict, "open_flags");
+        if (open_flags & BDRV_O_NOCACHE)
+            monitor_printf(mon, " hostcache=0");
+        else
+            monitor_printf(mon, " hostcache=1");
+    }
+
     if (qdict_haskey(bs_dict, "inserted")) {
         QDict *qdict = qobject_to_qdict(qdict_get(bs_dict, "inserted"));
 
@@ -1749,13 +1757,18 @@ void bdrv_info(Monitor *mon, QObject **r
         QObject *bs_obj;
 
         bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
-                                    "'removable': %i, 'locked': %i }",
-                                    bs->device_name, bs->removable,
-                                    bs->locked);
+                                     "'removable': %i, 'locked': %i, "
+                                     "'hostcache': %s }",
+                                     bs->device_name, bs->removable,
+                                     bs->locked,
+                                     (bs->open_flags & BDRV_O_NOCACHE) ?
+                                     "false" : "true");
+
+        QDict *bs_dict = qobject_to_qdict(bs_obj);
+        qdict_put(bs_dict, "open_flags", qint_from_int(bs->open_flags));
 
         if (bs->drv) {
             QObject *obj;
-            QDict *bs_dict = qobject_to_qdict(bs_obj);
 
             obj = qobject_from_jsonf("{ 'file': %s, 'ro': %i, 'drv': %s, "
                                      "'encrypted': %i }",
Index: qemu/qmp-commands.hx
===================================================================
--- qemu.orig/qmp-commands.hx
+++ qemu/qmp-commands.hx
@@ -1131,6 +1131,7 @@ Each json-object contain the following:
          - Possible values: "unknown"
 - "removable": true if the device is removable, false otherwise (json-bool)
 - "locked": true if the device is locked, false otherwise (json-bool)
+- "hostcache": true if hostcache enabled, false otherwise (json-bool)
 - "inserted": only present if the device is inserted, it is a json-object
    containing the following:
          - "file": device file name (json-string)
@@ -1152,6 +1153,7 @@ Example:
          {
             "device":"ide0-hd0",
             "locked":false,
+            "hostcache":false,
             "removable":false,
             "inserted":{
                "ro":false,

Re: [Qemu-devel] [V5 Patch 1/4]Qemu: Enhance "info block" to display host cache setting

[Stefan Hajnoczi]

On Wed, Jul 27, 2011 at 12:30 PM, Supriya Kannery
<supriyak@linux.vnet.ibm.com> wrote:
> Enhance "info block" to display hostcache setting for each
> block device.
>
> Example:
> (qemu) info block
> ide0-hd0: type=hd removable=0 file=../rhel6-32.qcow2 ro=0 drv=qcow2
> encrypted=0
>
> Enhanced to display "hostcache" setting:
> (qemu) info block
> ide0-hd0: type=hd removable=0 hostcache=true file=../rhel6-32.qcow2
> ro=0 drv=qcow2 encrypted=0

This description is outdated, should be hostcache=1 instead of hostcache=true.

> @@ -1749,13 +1757,18 @@ void bdrv_info(Monitor *mon, QObject **r
>         QObject *bs_obj;
>
>         bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
> -                                    "'removable': %i, 'locked': %i }",
> -                                    bs->device_name, bs->removable,
> -                                    bs->locked);
> +                                     "'removable': %i, 'locked': %i, "
> +                                     "'hostcache': %s }",
> +                                     bs->device_name, bs->removable,
> +                                     bs->locked,
> +                                     (bs->open_flags & BDRV_O_NOCACHE) ?
> +                                     "false" : "true");

Please use the same bool-from-int format specifier ('%i') as the other
fields for consistency.  The value can be !(bs->open_flags &
BDRV_O_NOCACHE).

> +
> +        QDict *bs_dict = qobject_to_qdict(bs_obj);
> +        qdict_put(bs_dict, "open_flags", qint_from_int(bs->open_flags));
>
>         if (bs->drv) {
>             QObject *obj;
> -            QDict *bs_dict = qobject_to_qdict(bs_obj);
>
>             obj = qobject_from_jsonf("{ 'file': %s, 'ro': %i, 'drv': %s, "
>                                      "'encrypted': %i }",

My copy of the code has the following a few lines further down:
qdict_put_obj(bs_dict, "inserted", obj);

Removing bs_dict would break the build.  Am I missing something?

> Index: qemu/qmp-commands.hx
> ===================================================================
> --- qemu.orig/qmp-commands.hx
> +++ qemu/qmp-commands.hx
> @@ -1131,6 +1131,7 @@ Each json-object contain the following:
>          - Possible values: "unknown"
>  - "removable": true if the device is removable, false otherwise (json-bool)
>  - "locked": true if the device is locked, false otherwise (json-bool)
> +- "hostcache": true if hostcache enabled, false otherwise (json-bool)

Let's explain what "hostcache" means:

- "hostcache": true if host page cache is enabled, false otherwise (json-bool)

Stefan

Re: [Qemu-devel] [V5 Patch 1/4]Qemu: Enhance "info block" to display host cache setting

[Supriya Kannery]

On 07/27/2011 07:49 PM, Stefan Hajnoczi wrote:
> On Wed, Jul 27, 2011 at 12:30 PM, Supriya Kannery
> <supriyak@linux.vnet.ibm.com>  wrote:
>> Enhance "info block" to display hostcache setting for each
>> block device.
>>
>> Example:
>> (qemu) info block
>> ide0-hd0: type=hd removable=0 file=../rhel6-32.qcow2 ro=0 drv=qcow2
>> encrypted=0
>>
>> Enhanced to display "hostcache" setting:
>> (qemu) info block
>> ide0-hd0: type=hd removable=0 hostcache=true file=../rhel6-32.qcow2
>> ro=0 drv=qcow2 encrypted=0
>
> This description is outdated, should be hostcache=1 instead of hostcache=true.
>
>> @@ -1749,13 +1757,18 @@ void bdrv_info(Monitor *mon, QObject **r
>>          QObject *bs_obj;
>>
>>          bs_obj = qobject_from_jsonf("{ 'device': %s, 'type': 'unknown', "
>> -                                    "'removable': %i, 'locked': %i }",
>> -                                    bs->device_name, bs->removable,
>> -                                    bs->locked);
>> +                                     "'removable': %i, 'locked': %i, "
>> +                                     "'hostcache': %s }",
>> +                                     bs->device_name, bs->removable,
>> +                                     bs->locked,
>> +                                     (bs->open_flags&  BDRV_O_NOCACHE) ?
>> +                                     "false" : "true");
>
> Please use the same bool-from-int format specifier ('%i') as the other
> fields for consistency.  The value can be !(bs->open_flags&
> BDRV_O_NOCACHE).
>

ok

>> +
>> +        QDict *bs_dict = qobject_to_qdict(bs_obj);
>> +        qdict_put(bs_dict, "open_flags", qint_from_int(bs->open_flags));
>>
>>          if (bs->drv) {
>>              QObject *obj;
>> -            QDict *bs_dict = qobject_to_qdict(bs_obj);
>>
>>              obj = qobject_from_jsonf("{ 'file': %s, 'ro': %i, 'drv': %s, "
>>                                       "'encrypted': %i }",
>
> My copy of the code has the following a few lines further down:
> qdict_put_obj(bs_dict, "inserted", obj);
>
> Removing bs_dict would break the build.  Am I missing something?
>

qdict_put_obj() for "inserted" is still there further down in the code.
Here,
   QDict *bs_dict = qobject_to_qdict(bs_obj);
is moved up to enable placing of "open_flags" into the dict. We need 
"open_flags" in dict to calculate hostcache value for printing in
monitor. Code for placing "inserted" is not touched.

>> Index: qemu/qmp-commands.hx
>> ===================================================================
>> --- qemu.orig/qmp-commands.hx
>> +++ qemu/qmp-commands.hx
>> @@ -1131,6 +1131,7 @@ Each json-object contain the following:
>>           - Possible values: "unknown"
>>   - "removable": true if the device is removable, false otherwise (json-bool)
>>   - "locked": true if the device is locked, false otherwise (json-bool)
>> +- "hostcache": true if hostcache enabled, false otherwise (json-bool)
>
> Let's explain what "hostcache" means:
>
> - "hostcache": true if host page cache is enabled, false otherwise (json-bool)
>

ok

-Thanks, Supriya

> Stefan

[Qemu-devel] [V5 Patch 2/4]Qemu: qerrors for file reopen, data sync and cmd syntax

[Supriya Kannery]

New error classes defined for file reopen failure, data
sync error and incorrect command syntax

Signed-off-by: Supriya Kannery <supriyak@in.ibm.com>

---
 qerror.c |   12 ++++++++++++
 qerror.h |    8 ++++++++
 2 files changed, 20 insertions(+)

Index: qemu/qerror.c
===================================================================
--- qemu.orig/qerror.c
+++ qemu/qerror.c
@@ -97,6 +97,14 @@ static const QErrorStringTable qerror_ta
         .desc      = "Device '%(device)' is not removable",
     },
     {
+        .error_fmt = QERR_DATA_SYNC_FAILED,
+        .desc      = "Syncing of data failed for device '%(device)'",
+    },
+    {
+        .error_fmt = QERR_REOPEN_FILE_FAILED,
+        .desc      = "Could not reopen '%(filename)'",
+    },
+    {
         .error_fmt = QERR_DEVICE_NO_BUS,
         .desc      = "Device '%(device)' has no child bus",
     },
@@ -230,6 +238,10 @@ static const QErrorStringTable qerror_ta
         .error_fmt = QERR_QGA_COMMAND_FAILED,
         .desc      = "Guest agent command failed, error was '%(message)'",
     },
+    {
+        .error_fmt = QERR_INCORRECT_COMMAND_SYNTAX,
+        .desc      = "Usage: %(syntax)",
+    },
     {}
 };
 
Index: qemu/qerror.h
===================================================================
--- qemu.orig/qerror.h
+++ qemu/qerror.h
@@ -85,6 +85,9 @@ QError *qobject_to_qerror(const QObject 
 #define QERR_DEVICE_NOT_FOUND \
     "{ 'class': 'DeviceNotFound', 'data': { 'device': %s } }"
 
+#define QERR_DATA_SYNC_FAILED \
+    "{ 'class': 'DataSyncFailed', 'data': { 'device': %s } }"
+
 #define QERR_DEVICE_NOT_REMOVABLE \
     "{ 'class': 'DeviceNotRemovable', 'data': { 'device': %s } }"
 
@@ -142,6 +145,9 @@ QError *qobject_to_qerror(const QObject 
 #define QERR_OPEN_FILE_FAILED \
     "{ 'class': 'OpenFileFailed', 'data': { 'filename': %s } }"
 
+#define QERR_REOPEN_FILE_FAILED \
+    "{ 'class': 'ReopenFileFailed', 'data': { 'filename': %s } }"
+
 #define QERR_PROPERTY_NOT_FOUND \
     "{ 'class': 'PropertyNotFound', 'data': { 'device': %s, 'property': %s } }"
 
@@ -193,4 +199,6 @@ QError *qobject_to_qerror(const QObject 
 #define QERR_QGA_COMMAND_FAILED \
     "{ 'class': 'QgaCommandFailed', 'data': { 'message': %s } }"
 
+#define QERR_INCORRECT_COMMAND_SYNTAX \
+    "{ 'class': 'IncorrectCommandSyntax', 'data': { 'syntax': %s } }"
 #endif /* QERROR_H */

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

[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 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
+
 
     {
         .name       = "eject",
Index: qemu/qemu-config.c
===================================================================
--- qemu.orig/qemu-config.c
+++ qemu/qemu-config.c
@@ -517,6 +517,19 @@ QemuOptsList *qemu_find_opts(const char 
     return find_list(vm_config_groups, group);
 }
 
+/* Validate given opts list with that of defined vm_group */
+int qemu_validate_opts(QemuOpts *opts, const char *group)
+{
+    QemuOptsList *vm_group;
+
+    vm_group  = qemu_find_opts(group);
+    if (vm_group == NULL) {
+        return -1;
+    }
+
+    return qemu_opts_validate(opts, &vm_group->desc[0]);
+}
+
 void qemu_add_opts(QemuOptsList *list)
 {
     int entries, i;
Index: qemu/qemu-option.c
===================================================================
--- qemu.orig/qemu-option.c
+++ qemu/qemu-option.c
@@ -599,6 +599,31 @@ static void qemu_opt_del(QemuOpt *opt)
     qemu_free(opt);
 }
 
+/*
+ * Delete specified parameter with name "name" from opts
+ * Return
+ *     0 - Deletion Successful
+ *    -1 - Param doesn't exist in opts
+ *     1 - Deletion Successful and opts is empty.
+*/
+
+int qemu_opt_delete(QemuOpts *opts, const char *name)
+{
+    QemuOpt *opt = qemu_opt_find(opts, name);
+    if (opt == NULL) {
+        return -1;
+    }
+
+    qemu_opt_del(opt);
+
+    if ((&opts->head)->tqh_first == NULL) {
+        /* opt queue is empty */
+        return 1;
+    }
+
+    return 0;
+}
+
 int qemu_opt_set(QemuOpts *opts, const char *name, const char *value)
 {
     QemuOpt *opt;
Index: qemu/qemu-option.h
===================================================================
--- qemu.orig/qemu-option.h
+++ qemu/qemu-option.h
@@ -121,6 +121,7 @@ int qemu_opts_set(QemuOptsList *list, co
                   const char *name, const char *value);
 const char *qemu_opts_id(QemuOpts *opts);
 void qemu_opts_del(QemuOpts *opts);
+int qemu_opt_delete(QemuOpts *opts, const char *name);
 int qemu_opts_validate(QemuOpts *opts, const QemuOptDesc *desc);
 int qemu_opts_do_parse(QemuOpts *opts, const char *params, const char *firstname);
 QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params, int permit_abbrev);
@@ -131,5 +132,6 @@ typedef int (*qemu_opts_loopfunc)(QemuOp
 int qemu_opts_print(QemuOpts *opts, void *dummy);
 int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func, void *opaque,
                       int abort_on_failure);
+int qemu_validate_opts(QemuOpts *opts, const char *id);
 
 #endif
Index: qemu/qmp-commands.hx
===================================================================
--- qemu.orig/qmp-commands.hx
+++ qemu/qmp-commands.hx
@@ -727,7 +727,35 @@ Example:
 
 EQMP
 
+
     {
+        .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,
+    },
+
+SQMP
+block_set
+---------
+
+Change various block device parameters (eg: hostcache=on/off)
+
+Arguments:
+
+- "device": the device's ID, must be unique (json-string)
+- device parameters to be changed (eg: "hostcache": "off")
+
+Example:
+
+-> { "execute": "block_set", "arguments": { "device": "ide0-hd0", "hostcache": "off"} }
+<- { "return": {} }
+
+EQMP
+
+	{
         .name       = "balloon",
         .args_type  = "value:M",
         .params     = "target",

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

[Anthony Liguori]

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

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

[Stefan Hajnoczi]

On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> 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.

The idea behind block_set was to have a unified interface for changing
block device parameters at runtime.  This prevents us from reinventing
new commands from scratch.  For example, block I/O throttling is
already queued up to add run-time parameters.

Without a unified command we have a bulkier QMP/HMP interface,
duplicated code, and possibly inconsistencies in syntax between the
commands.  Isn't the best way to avoid these problems a unified
interface?

I understand the lack of type safety concern but in this case we
already have to manually pull parsed arguments (i.e. cast to specific
types and deal with invalid input).  To me this is a reason *for*
using a unified interface like block_set.

Stefan

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

[Anthony Liguori]

On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote:
> On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>> 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.
>
> The idea behind block_set was to have a unified interface for changing
> block device parameters at runtime.  This prevents us from reinventing
> new commands from scratch.  For example, block I/O throttling is
> already queued up to add run-time parameters.
>
> Without a unified command we have a bulkier QMP/HMP interface,
> duplicated code, and possibly inconsistencies in syntax between the
> commands.  Isn't the best way to avoid these problems a unified
> interface?
>
> I understand the lack of type safety concern but in this case we
> already have to manually pull parsed arguments (i.e. cast to specific
> types and deal with invalid input).  To me this is a reason *for*
> using a unified interface like block_set.

Think about it from a client perspective.  How do I determine which 
properties are supported by this version of QEMU?  I have no way to 
identify programmatically what arguments are valid for block_set.

OTOH, if you have strong types like block_set_hostcache, query-commands 
tells me exactly what's supported.

Regards,

Anthony Liguori

>
> Stefan
>

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

[Stefan Hajnoczi]

On Wed, Jul 27, 2011 at 5:02 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote:
>>
>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori<anthony@codemonkey.ws>
>>  wrote:
>>>>
>>>> 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.
>>
>> The idea behind block_set was to have a unified interface for changing
>> block device parameters at runtime.  This prevents us from reinventing
>> new commands from scratch.  For example, block I/O throttling is
>> already queued up to add run-time parameters.
>>
>> Without a unified command we have a bulkier QMP/HMP interface,
>> duplicated code, and possibly inconsistencies in syntax between the
>> commands.  Isn't the best way to avoid these problems a unified
>> interface?
>>
>> I understand the lack of type safety concern but in this case we
>> already have to manually pull parsed arguments (i.e. cast to specific
>> types and deal with invalid input).  To me this is a reason *for*
>> using a unified interface like block_set.
>
> Think about it from a client perspective.  How do I determine which
> properties are supported by this version of QEMU?  I have no way to identify
> programmatically what arguments are valid for block_set.
>
> OTOH, if you have strong types like block_set_hostcache, query-commands
> tells me exactly what's supported.

Use query-block and see if 'hostcache' is there.  If yes, then the
hostcache parameter is available.  If we allow BlockDrivers to have
their own runtime parameters then query-commands does not tell you
anything because the specific BlockDriver may or may not support that
runtime parameter - you need to use query-block.

Stefan

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

[Stefan Hajnoczi]

On Thu, Jul 28, 2011 at 10:29 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Wed, Jul 27, 2011 at 5:02 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote:
>>>
>>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori<anthony@codemonkey.ws>
>>>  wrote:
>>>>>
>>>>> 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.
>>>
>>> The idea behind block_set was to have a unified interface for changing
>>> block device parameters at runtime.  This prevents us from reinventing
>>> new commands from scratch.  For example, block I/O throttling is
>>> already queued up to add run-time parameters.
>>>
>>> Without a unified command we have a bulkier QMP/HMP interface,
>>> duplicated code, and possibly inconsistencies in syntax between the
>>> commands.  Isn't the best way to avoid these problems a unified
>>> interface?
>>>
>>> I understand the lack of type safety concern but in this case we
>>> already have to manually pull parsed arguments (i.e. cast to specific
>>> types and deal with invalid input).  To me this is a reason *for*
>>> using a unified interface like block_set.
>>
>> Think about it from a client perspective.  How do I determine which
>> properties are supported by this version of QEMU?  I have no way to identify
>> programmatically what arguments are valid for block_set.
>>
>> OTOH, if you have strong types like block_set_hostcache, query-commands
>> tells me exactly what's supported.
>
> Use query-block and see if 'hostcache' is there.  If yes, then the
> hostcache parameter is available.  If we allow BlockDrivers to have
> their own runtime parameters then query-commands does not tell you
> anything because the specific BlockDriver may or may not support that
> runtime parameter - you need to use query-block.

Let's reach agreement here.  The choices are:

1. Top-level block_set command.  Supported parameters are discovered
by looking query-block output.

2. Top-level command for each parameter (e.g. block_set_hostcache).
Supported parameters are easily discoverable via query-commands.  If
individual block devices support different sets of parameters then
they may have to return -ENOTSUPP.

I like the block_set approach.

Anthony, Kevin, Supriya: Any thoughts?

Stefan

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

[Anthony Liguori]

On 08/01/2011 10:22 AM, Stefan Hajnoczi wrote:
> On Thu, Jul 28, 2011 at 10:29 AM, Stefan Hajnoczi<stefanha@gmail.com>  wrote:
>> On Wed, Jul 27, 2011 at 5:02 PM, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote:
>>>>
>>>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori<anthony@codemonkey.ws>
>>>>   wrote:
>>>>>>
>>>>>> 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.
>>>>
>>>> The idea behind block_set was to have a unified interface for changing
>>>> block device parameters at runtime.  This prevents us from reinventing
>>>> new commands from scratch.  For example, block I/O throttling is
>>>> already queued up to add run-time parameters.
>>>>
>>>> Without a unified command we have a bulkier QMP/HMP interface,
>>>> duplicated code, and possibly inconsistencies in syntax between the
>>>> commands.  Isn't the best way to avoid these problems a unified
>>>> interface?
>>>>
>>>> I understand the lack of type safety concern but in this case we
>>>> already have to manually pull parsed arguments (i.e. cast to specific
>>>> types and deal with invalid input).  To me this is a reason *for*
>>>> using a unified interface like block_set.
>>>
>>> Think about it from a client perspective.  How do I determine which
>>> properties are supported by this version of QEMU?  I have no way to identify
>>> programmatically what arguments are valid for block_set.
>>>
>>> OTOH, if you have strong types like block_set_hostcache, query-commands
>>> tells me exactly what's supported.
>>
>> Use query-block and see if 'hostcache' is there.  If yes, then the
>> hostcache parameter is available.  If we allow BlockDrivers to have
>> their own runtime parameters then query-commands does not tell you
>> anything because the specific BlockDriver may or may not support that
>> runtime parameter - you need to use query-block.
>
> Let's reach agreement here.  The choices are:
>
> 1. Top-level block_set command.  Supported parameters are discovered
> by looking query-block output.

I'm strongly opposed to this.  There needs to be a single consistent way 
to determine supported operations with QMP.

And that single mechanism already exists--query_commands.

> 2. Top-level command for each parameter (e.g. block_set_hostcache).
> Supported parameters are easily discoverable via query-commands.  If
> individual block devices support different sets of parameters then
> they may have to return -ENOTSUPP.
>
> I like the block_set approach.
>
> Anthony, Kevin, Supriya: Any thoughts?

For the sake of overall QMP sanity, I think block_set_hostcache is 
really our only option.

Regards,

Anthony Liguori

> Stefan
>

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

[Stefan Hajnoczi]

On Mon, Aug 1, 2011 at 4:28 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 08/01/2011 10:22 AM, Stefan Hajnoczi wrote:
>>
>> On Thu, Jul 28, 2011 at 10:29 AM, Stefan Hajnoczi<stefanha@gmail.com>
>>  wrote:
>>>
>>> On Wed, Jul 27, 2011 at 5:02 PM, Anthony Liguori<anthony@codemonkey.ws>
>>>  wrote:
>>>>
>>>> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote:
>>>>>
>>>>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori<anthony@codemonkey.ws>
>>>>>  wrote:
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>> The idea behind block_set was to have a unified interface for changing
>>>>> block device parameters at runtime.  This prevents us from reinventing
>>>>> new commands from scratch.  For example, block I/O throttling is
>>>>> already queued up to add run-time parameters.
>>>>>
>>>>> Without a unified command we have a bulkier QMP/HMP interface,
>>>>> duplicated code, and possibly inconsistencies in syntax between the
>>>>> commands.  Isn't the best way to avoid these problems a unified
>>>>> interface?
>>>>>
>>>>> I understand the lack of type safety concern but in this case we
>>>>> already have to manually pull parsed arguments (i.e. cast to specific
>>>>> types and deal with invalid input).  To me this is a reason *for*
>>>>> using a unified interface like block_set.
>>>>
>>>> Think about it from a client perspective.  How do I determine which
>>>> properties are supported by this version of QEMU?  I have no way to
>>>> identify
>>>> programmatically what arguments are valid for block_set.
>>>>
>>>> OTOH, if you have strong types like block_set_hostcache, query-commands
>>>> tells me exactly what's supported.
>>>
>>> Use query-block and see if 'hostcache' is there.  If yes, then the
>>> hostcache parameter is available.  If we allow BlockDrivers to have
>>> their own runtime parameters then query-commands does not tell you
>>> anything because the specific BlockDriver may or may not support that
>>> runtime parameter - you need to use query-block.
>>
>> Let's reach agreement here.  The choices are:
>>
>> 1. Top-level block_set command.  Supported parameters are discovered
>> by looking query-block output.
>
> I'm strongly opposed to this.  There needs to be a single consistent way to
> determine supported operations with QMP.
>
> And that single mechanism already exists--query_commands.

Okay, works for me.

Supriya: this means that there should be a block_set_hostcache command
and you don't need to worry about a generic block_set command.

Stefan

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

[Kevin Wolf]

Am 01.08.2011 17:28, schrieb Anthony Liguori:
> On 08/01/2011 10:22 AM, Stefan Hajnoczi wrote:
>> On Thu, Jul 28, 2011 at 10:29 AM, Stefan Hajnoczi<stefanha@gmail.com>  wrote:
>>> On Wed, Jul 27, 2011 at 5:02 PM, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>>> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote:
>>>>>
>>>>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori<anthony@codemonkey.ws>
>>>>>   wrote:
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>> The idea behind block_set was to have a unified interface for changing
>>>>> block device parameters at runtime.  This prevents us from reinventing
>>>>> new commands from scratch.  For example, block I/O throttling is
>>>>> already queued up to add run-time parameters.
>>>>>
>>>>> Without a unified command we have a bulkier QMP/HMP interface,
>>>>> duplicated code, and possibly inconsistencies in syntax between the
>>>>> commands.  Isn't the best way to avoid these problems a unified
>>>>> interface?
>>>>>
>>>>> I understand the lack of type safety concern but in this case we
>>>>> already have to manually pull parsed arguments (i.e. cast to specific
>>>>> types and deal with invalid input).  To me this is a reason *for*
>>>>> using a unified interface like block_set.
>>>>
>>>> Think about it from a client perspective.  How do I determine which
>>>> properties are supported by this version of QEMU?  I have no way to identify
>>>> programmatically what arguments are valid for block_set.
>>>>
>>>> OTOH, if you have strong types like block_set_hostcache, query-commands
>>>> tells me exactly what's supported.
>>>
>>> Use query-block and see if 'hostcache' is there.  If yes, then the
>>> hostcache parameter is available.  If we allow BlockDrivers to have
>>> their own runtime parameters then query-commands does not tell you
>>> anything because the specific BlockDriver may or may not support that
>>> runtime parameter - you need to use query-block.
>>
>> Let's reach agreement here.  The choices are:
>>
>> 1. Top-level block_set command.  Supported parameters are discovered
>> by looking query-block output.
> 
> I'm strongly opposed to this.  There needs to be a single consistent way 
> to determine supported operations with QMP.
> 
> And that single mechanism already exists--query_commands.
> 
>> 2. Top-level command for each parameter (e.g. block_set_hostcache).
>> Supported parameters are easily discoverable via query-commands.  If
>> individual block devices support different sets of parameters then
>> they may have to return -ENOTSUPP.
>>
>> I like the block_set approach.
>>
>> Anthony, Kevin, Supriya: Any thoughts?
> 
> For the sake of overall QMP sanity, I think block_set_hostcache is 
> really our only option.

Ideally we should have blockdev_add, and blockdev_set would just take
the same arguments and update the given driver.

But we don't have blockdev_add today, so whatever works for your as a
temporary solution...

Kevin

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

[Anthony Liguori]

On 08/01/2011 10:44 AM, Kevin Wolf wrote:
> Am 01.08.2011 17:28, schrieb Anthony Liguori:
>>> 2. Top-level command for each parameter (e.g. block_set_hostcache).
>>> Supported parameters are easily discoverable via query-commands.  If
>>> individual block devices support different sets of parameters then
>>> they may have to return -ENOTSUPP.
>>>
>>> I like the block_set approach.
>>>
>>> Anthony, Kevin, Supriya: Any thoughts?
>>
>> For the sake of overall QMP sanity, I think block_set_hostcache is
>> really our only option.
>
> Ideally we should have blockdev_add, and blockdev_set would just take
> the same arguments and update the given driver.

Ideally we'd have a backend_add, backend_set, etc.

But in the absence of that, we should provide the best interface we can 
with the current tools we have.

For now, using high level commands is the best we can do.

Regards,

Anthony Liguori

>
> But we don't have blockdev_add today, so whatever works for your as a
> temporary solution...
>
> Kevin
>

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

[Supriya Kannery]

On 08/01/2011 09:14 PM, Anthony Liguori wrote:
> On 08/01/2011 10:44 AM, Kevin Wolf wrote:
>> Am 01.08.2011 17:28, schrieb Anthony Liguori:
>>>> 2. Top-level command for each parameter (e.g. block_set_hostcache).
>>>> Supported parameters are easily discoverable via query-commands. If
>>>> individual block devices support different sets of parameters then
>>>> they may have to return -ENOTSUPP.
>>>>
>>>> I like the block_set approach.
>>>>
>>>> Anthony, Kevin, Supriya: Any thoughts?
>>>
>>> For the sake of overall QMP sanity, I think block_set_hostcache is
>>> really our only option.
>>
>> Ideally we should have blockdev_add, and blockdev_set would just take
>> the same arguments and update the given driver.
>
> Ideally we'd have a backend_add, backend_set, etc.
>
> But in the absence of that, we should provide the best interface we can
> with the current tools we have.
>
> For now, using high level commands is the best we can do.

Will be modifying code to have 'block_set_hostcache' command 
implemented. Along with that, planning to implement 
'query-block_set_hostcache', that returns current hostcache setting
for all the applicable block devices.

I am not able to find how "query-commands" is helping out
to programmatically find out all the supported parameters
of a specific command. When I tried out, "query-commands"
is listing all the supported command names. "query-xx" is
returning current settings related to command 'xx',
but not any information related to supported parameters
of 'xx'.
Am I missing something?

>
> Regards,
>
> Anthony Liguori
>
>>
>> But we don't have blockdev_add today, so whatever works for your as a
>> temporary solution...
>>
>> Kevin
>>
>
>

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

[Stefan Hajnoczi]

On Thu, Aug 4, 2011 at 9:32 AM, Supriya Kannery
<supriyak@linux.vnet.ibm.com> wrote:
> On 08/01/2011 09:14 PM, Anthony Liguori wrote:
>>
>> On 08/01/2011 10:44 AM, Kevin Wolf wrote:
>>>
>>> Am 01.08.2011 17:28, schrieb Anthony Liguori:
>>>>>
>>>>> 2. Top-level command for each parameter (e.g. block_set_hostcache).
>>>>> Supported parameters are easily discoverable via query-commands. If
>>>>> individual block devices support different sets of parameters then
>>>>> they may have to return -ENOTSUPP.
>>>>>
>>>>> I like the block_set approach.
>>>>>
>>>>> Anthony, Kevin, Supriya: Any thoughts?
>>>>
>>>> For the sake of overall QMP sanity, I think block_set_hostcache is
>>>> really our only option.
>>>
>>> Ideally we should have blockdev_add, and blockdev_set would just take
>>> the same arguments and update the given driver.
>>
>> Ideally we'd have a backend_add, backend_set, etc.
>>
>> But in the absence of that, we should provide the best interface we can
>> with the current tools we have.
>>
>> For now, using high level commands is the best we can do.
>
> Will be modifying code to have 'block_set_hostcache' command implemented.
> Along with that, planning to implement 'query-block_set_hostcache', that
> returns current hostcache setting
> for all the applicable block devices.

I don't think a query command is necessary since query-block already
reports this information.

Stefan

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

[Supriya Kannery]

On 08/04/2011 02:01 PM, Stefan Hajnoczi wrote:
> On Thu, Aug 4, 2011 at 9:32 AM, Supriya Kannery
> <supriyak@linux.vnet.ibm.com>  wrote:
>> On 08/01/2011 09:14 PM, Anthony Liguori wrote:
>>> On 08/01/2011 10:44 AM, Kevin Wolf wrote:
>>>> Am 01.08.2011 17:28, schrieb Anthony Liguori:
>>>>>> 2. Top-level command for each parameter (e.g. block_set_hostcache).
>>>>>> Supported parameters are easily discoverable via query-commands. If
>>>>>> individual block devices support different sets of parameters then
>>>>>> they may have to return -ENOTSUPP.
>>>>>>

>>>
>>> For now, using high level commands is the best we can do.
>> Will be modifying code to have 'block_set_hostcache' command implemented.
>> Along with that, planning to implement 'query-block_set_hostcache', that
>> returns current hostcache setting
>> for all the applicable block devices.
>
> I don't think a query command is necessary since query-block already
> reports this information.
>

ok

> Stefan

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

[Supriya Kannery]

On 08/04/2011 02:02 PM, Supriya Kannery wrote:
> On 08/01/2011 09:14 PM, Anthony Liguori wrote:
>> On 08/01/2011 10:44 AM, Kevin Wolf wrote:
>>> Am 01.08.2011 17:28, schrieb Anthony Liguori:
>>>>> 2. Top-level command for each parameter (e.g. block_set_hostcache).
>>>>> Supported parameters are easily discoverable via query-commands. If
>>>>> individual block devices support different sets of parameters then
>>>>> they may have to return -ENOTSUPP.
>>>>>
>
> I am not able to find how "query-commands" is helping out
> to programmatically find out all the supported parameters
> of a specific command. When I tried out, "query-commands"
> is listing all the supported command names. "query-xx" is
> returning current settings related to command 'xx',
> but not any information related to supported parameters
> of 'xx'.
> Am I missing something?
>

ok, I got it. "query commands" is not supposed to list out
supported params for each command. Each block device parameter
setting, when implemented as separate high level command, will
get listed through query-commands. And this helps user to
identify supported block parameters that can be changed.


>>
>> Regards,
>>
>> Anthony Liguori
>>
>>>
>>> But we don't have blockdev_add today, so whatever works for your as a
>>> temporary solution...
>>>
>>> Kevin
>>>
>>
>>
>
>

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

[Stefan Hajnoczi]

On Mon, Aug 1, 2011 at 4:44 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 01.08.2011 17:28, schrieb Anthony Liguori:
>> On 08/01/2011 10:22 AM, Stefan Hajnoczi wrote:
>>> On Thu, Jul 28, 2011 at 10:29 AM, Stefan Hajnoczi<stefanha@gmail.com>  wrote:
>>>> On Wed, Jul 27, 2011 at 5:02 PM, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>>>> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote:
>>>>>>
>>>>>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony Liguori<anthony@codemonkey.ws>
>>>>>>   wrote:
>>>>>>>>
>>>>>>>> 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.
>>>>>>
>>>>>> The idea behind block_set was to have a unified interface for changing
>>>>>> block device parameters at runtime.  This prevents us from reinventing
>>>>>> new commands from scratch.  For example, block I/O throttling is
>>>>>> already queued up to add run-time parameters.
>>>>>>
>>>>>> Without a unified command we have a bulkier QMP/HMP interface,
>>>>>> duplicated code, and possibly inconsistencies in syntax between the
>>>>>> commands.  Isn't the best way to avoid these problems a unified
>>>>>> interface?
>>>>>>
>>>>>> I understand the lack of type safety concern but in this case we
>>>>>> already have to manually pull parsed arguments (i.e. cast to specific
>>>>>> types and deal with invalid input).  To me this is a reason *for*
>>>>>> using a unified interface like block_set.
>>>>>
>>>>> Think about it from a client perspective.  How do I determine which
>>>>> properties are supported by this version of QEMU?  I have no way to identify
>>>>> programmatically what arguments are valid for block_set.
>>>>>
>>>>> OTOH, if you have strong types like block_set_hostcache, query-commands
>>>>> tells me exactly what's supported.
>>>>
>>>> Use query-block and see if 'hostcache' is there.  If yes, then the
>>>> hostcache parameter is available.  If we allow BlockDrivers to have
>>>> their own runtime parameters then query-commands does not tell you
>>>> anything because the specific BlockDriver may or may not support that
>>>> runtime parameter - you need to use query-block.
>>>
>>> Let's reach agreement here.  The choices are:
>>>
>>> 1. Top-level block_set command.  Supported parameters are discovered
>>> by looking query-block output.
>>
>> I'm strongly opposed to this.  There needs to be a single consistent way
>> to determine supported operations with QMP.
>>
>> And that single mechanism already exists--query_commands.
>>
>>> 2. Top-level command for each parameter (e.g. block_set_hostcache).
>>> Supported parameters are easily discoverable via query-commands.  If
>>> individual block devices support different sets of parameters then
>>> they may have to return -ENOTSUPP.
>>>
>>> I like the block_set approach.
>>>
>>> Anthony, Kevin, Supriya: Any thoughts?
>>
>> For the sake of overall QMP sanity, I think block_set_hostcache is
>> really our only option.
>
> Ideally we should have blockdev_add, and blockdev_set would just take
> the same arguments and update the given driver.
>
> But we don't have blockdev_add today, so whatever works for your as a
> temporary solution...

Anthony's point is that blockdev_set does not fit with QMP command
discoverability.

Stefan

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

[Anthony Liguori]

On 08/01/2011 10:44 AM, Stefan Hajnoczi wrote:
> On Mon, Aug 1, 2011 at 4:44 PM, Kevin Wolf<kwolf@redhat.com>  wrote:
>> Am 01.08.2011 17:28, schrieb Anthony Liguori:
>>> On 08/01/2011 10:22 AM, Stefan Hajnoczi wrote:
>>>> I like the block_set approach.
>>>>
>>>> Anthony, Kevin, Supriya: Any thoughts?
>>>
>>> For the sake of overall QMP sanity, I think block_set_hostcache is
>>> really our only option.
>>
>> Ideally we should have blockdev_add, and blockdev_set would just take
>> the same arguments and update the given driver.
>>
>> But we don't have blockdev_add today, so whatever works for your as a
>> temporary solution...
>
> Anthony's point is that blockdev_set does not fit with QMP command
> discoverability.

It doesn't, but if we had a 'plug_set' that worked for devices and any 
type of backends, we could have a single introspection mechanism.

But we should really try to avoid having every backend implement their 
own ways of doing things.

Regards,

Anthony Liguori

>
> Stefan
>

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

[Supriya Kannery]

On 07/27/2011 09:32 PM, Anthony Liguori wrote:
> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote:
>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony
>> Liguori<anthony@codemonkey.ws> wrote:
>>>> 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.
>>
>> The idea behind block_set was to have a unified interface for changing
>> block device parameters at runtime. This prevents us from reinventing
>> new commands from scratch. For example, block I/O throttling is
>> already queued up to add run-time parameters.
>>
>> Without a unified command we have a bulkier QMP/HMP interface,
>> duplicated code, and possibly inconsistencies in syntax between the
>> commands. Isn't the best way to avoid these problems a unified
>> interface?
>>
>> I understand the lack of type safety concern but in this case we
>> already have to manually pull parsed arguments (i.e. cast to specific
>> types and deal with invalid input). To me this is a reason *for*
>> using a unified interface like block_set.
>
> Think about it from a client perspective. How do I determine which
> properties are supported by this version of QEMU? I have no way to
> identify programmatically what arguments are valid for block_set.
>

    User can programmatically find out valid parameters for
block_set. Internally, validation of prop=value is done against -drive
parameter list and then, only the valid/implemented commands (for now, 
hostcache) are accepted from that list. Please find execution output 
attached:
========================================================================
(qemu) info block
ide0-hd0: removable=0 hostcache=1 file=../rhel6-32.qcow2 ro=0 drv=qcow2 
encrypted=0
floppy0: removable=1 locked=0 hostcache=0 file=test.img ro=0 drv=raw 
encrypted=0
ide1-cd0: removable=1 locked=0 hostcache=1 [not inserted]
sd0: removable=1 locked=0 hostcache=1 [not inserted]
(qemu) block
block_resize  block_set     block_passwd
(qemu) block_set
block_set: block device name expected
(qemu) block
block_resize  block_set     block_passwd
(qemu) help block_set
block_set device [prop=value][,...] -- Change block device parameters 
[hostcache=on/off]
(qemu) block_set ide
Device 'ide' not found
(qemu) block_set ide0-hd0
Usage: block_set device [prop=value][,...]
(qemu) block_set ide0-hd0 cache
Invalid parameter 'cache'
(qemu) block_set ide0-hd0 cache=on
Parameter 'hostcache' expects on/off
(qemu) block_set ide0-hd0 hostcache=on
(qemu) block_set ide0-hd0 hostcache=off
(qemu) info block
ide0-hd0: removable=0 hostcache=0 file=../rhel6-32.qcow2 ro=0 drv=qcow2 
encrypted=0
floppy0: removable=1 locked=0 hostcache=0 file=test.img ro=0 drv=raw 
encrypted=0
ide1-cd0: removable=1 locked=0 hostcache=1 [not inserted]
sd0: removable=1 locked=0 hostcache=1 [not inserted]
========================================================================

  When we add further parameters, "usage" string can be enhanced to
include those parameters for informing user.

- Thanks, Supriya

> OTOH, if you have strong types like block_set_hostcache, query-commands
> tells me exactly what's supported.
>
> Regards,
>
> Anthony Liguori
>
>>
>> Stefan
>>
>
>

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

[Anthony Liguori]

On 07/28/2011 05:13 AM, Supriya Kannery wrote:
> On 07/27/2011 09:32 PM, Anthony Liguori wrote:
>> On 07/27/2011 09:31 AM, Stefan Hajnoczi wrote:
>>> On Wed, Jul 27, 2011 at 1:58 PM, Anthony
>>> Liguori<anthony@codemonkey.ws> wrote:
>>>>> 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.
>>>
>>> The idea behind block_set was to have a unified interface for changing
>>> block device parameters at runtime. This prevents us from reinventing
>>> new commands from scratch. For example, block I/O throttling is
>>> already queued up to add run-time parameters.
>>>
>>> Without a unified command we have a bulkier QMP/HMP interface,
>>> duplicated code, and possibly inconsistencies in syntax between the
>>> commands. Isn't the best way to avoid these problems a unified
>>> interface?
>>>
>>> I understand the lack of type safety concern but in this case we
>>> already have to manually pull parsed arguments (i.e. cast to specific
>>> types and deal with invalid input). To me this is a reason *for*
>>> using a unified interface like block_set.
>>
>> Think about it from a client perspective. How do I determine which
>> properties are supported by this version of QEMU? I have no way to
>> identify programmatically what arguments are valid for block_set.
>>
>
> User can programmatically find out valid parameters for
> block_set. Internally, validation of prop=value is done against -drive
> parameter list and then, only the valid/implemented commands (for now,
> hostcache) are accepted from that list. Please find execution output
> attached:
> ========================================================================
> (qemu) info block
> ide0-hd0: removable=0 hostcache=1 file=../rhel6-32.qcow2 ro=0 drv=qcow2
> encrypted=0
> floppy0: removable=1 locked=0 hostcache=0 file=test.img ro=0 drv=raw
> encrypted=0
> ide1-cd0: removable=1 locked=0 hostcache=1 [not inserted]
> sd0: removable=1 locked=0 hostcache=1 [not inserted]
> (qemu) block
> block_resize block_set block_passwd
> (qemu) block_set

That's HMP btw, not QMP.

> block_set: block device name expected
> (qemu) block
> block_resize block_set block_passwd
> (qemu) help block_set
> block_set device [prop=value][,...] -- Change block device parameters
> [hostcache=on/off]

Parsing help text is not introspection!

Regards,

Anthony Liguori

> (qemu) block_set ide
> Device 'ide' not found
> (qemu) block_set ide0-hd0
> Usage: block_set device [prop=value][,...]
> (qemu) block_set ide0-hd0 cache
> Invalid parameter 'cache'
> (qemu) block_set ide0-hd0 cache=on
> Parameter 'hostcache' expects on/off
> (qemu) block_set ide0-hd0 hostcache=on
> (qemu) block_set ide0-hd0 hostcache=off
> (qemu) info block
> ide0-hd0: removable=0 hostcache=0 file=../rhel6-32.qcow2 ro=0 drv=qcow2
> encrypted=0
> floppy0: removable=1 locked=0 hostcache=0 file=test.img ro=0 drv=raw
> encrypted=0
> ide1-cd0: removable=1 locked=0 hostcache=1 [not inserted]
> sd0: removable=1 locked=0 hostcache=1 [not inserted]
> ========================================================================
>
> When we add further parameters, "usage" string can be enhanced to
> include those parameters for informing user.
>
> - Thanks, Supriya
>
>> OTOH, if you have strong types like block_set_hostcache, query-commands
>> tells me exactly what's supported.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>>
>>> Stefan
>>>
>>
>>
>
>

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

[Michael Tokarev]

27.07.2011 15:30, 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();
> +        }

Can we please avoid this stuff completely?  Just keep the
old device open still, until you're sure new one is ok.

Or else it will be quite dangerous command in many cases.
For example, after -runas/-chroot, or additional selinux
settings or whatnot.  And in this case, instead of merely
returning an error, we'll see abort().  Boom.

Thanks,

/mjt

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

[Anthony Liguori]

On 07/27/2011 08:43 AM, Michael Tokarev wrote:
>> 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();
>> +        }
>
> Can we please avoid this stuff completely?  Just keep the
> old device open still, until you're sure new one is ok.

I may be misremembering, but I thought Christoph had mentioned wanting 
to write a kernel patch to toggle O_DIRECT through fcntl().

This seems like the only way to make this not racy to me.

Regards,

Anthony Liguori

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

[Stefan Hajnoczi]

2011/7/27 Michael Tokarev <mjt@tls.msk.ru>:
> 27.07.2011 15:30, 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();
>> +        }
>
> Can we please avoid this stuff completely?  Just keep the
> old device open still, until you're sure new one is ok.
>
> Or else it will be quite dangerous command in many cases.
> For example, after -runas/-chroot, or additional selinux
> settings or whatnot.  And in this case, instead of merely
> returning an error, we'll see abort().  Boom.

Slight complication for image formats that use a dirty bit here.  QED
has a dirty bit.  VMDK also specifies one but we don't implement it
today.

If the image file is dirty then all its metadata will be scanned for
consistency when it is opened.  This increases the bdrv_open() time
and hence the downtime of the VM.  So it is not ideal to open the
image file twice, even though there is no consistency problem.

I'll think about this some more, there are a couple of solutions like
keeping only the file descriptor around, introducing a flush command
that makes sure the file is in a clean state, or changing QED to not
do this.

Stefan

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

[Kevin Wolf]

Am 27.07.2011 16:51, schrieb Stefan Hajnoczi:
> 2011/7/27 Michael Tokarev <mjt@tls.msk.ru>:
>> 27.07.2011 15:30, 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();
>>> +        }
>>
>> Can we please avoid this stuff completely?  Just keep the
>> old device open still, until you're sure new one is ok.
>>
>> Or else it will be quite dangerous command in many cases.
>> For example, after -runas/-chroot, or additional selinux
>> settings or whatnot.  And in this case, instead of merely
>> returning an error, we'll see abort().  Boom.
> 
> Slight complication for image formats that use a dirty bit here.  QED
> has a dirty bit.  VMDK also specifies one but we don't implement it
> today.
> 
> If the image file is dirty then all its metadata will be scanned for
> consistency when it is opened.  This increases the bdrv_open() time
> and hence the downtime of the VM.  So it is not ideal to open the
> image file twice, even though there is no consistency problem.

In general I really like understatements, but opening an image file
twice isn't only "not ideal", but should be considered verboten.

We're still doing it during migration and it means that in upstream qemu
any non-raw images will be corrupted.

> I'll think about this some more, there are a couple of solutions like
> keeping only the file descriptor around, introducing a flush command
> that makes sure the file is in a clean state, or changing QED to not
> do this.

Changing the format drivers doesn't really look like the right solution.

Keeping the fd around looks okay, we can probably achieve this by
introducing a bdrv_reopen function. It means that we may need to reopen
the format layer, but it can't really fail.

Kevin

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

[Stefan Hajnoczi]

On Thu, Jul 28, 2011 at 11:23 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 27.07.2011 16:51, schrieb Stefan Hajnoczi:
>> 2011/7/27 Michael Tokarev <mjt@tls.msk.ru>:
>>> 27.07.2011 15:30, 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();
>>>> +        }
>>>
>>> Can we please avoid this stuff completely?  Just keep the
>>> old device open still, until you're sure new one is ok.
>>>
>>> Or else it will be quite dangerous command in many cases.
>>> For example, after -runas/-chroot, or additional selinux
>>> settings or whatnot.  And in this case, instead of merely
>>> returning an error, we'll see abort().  Boom.
>>
>> Slight complication for image formats that use a dirty bit here.  QED
>> has a dirty bit.  VMDK also specifies one but we don't implement it
>> today.
>>
>> If the image file is dirty then all its metadata will be scanned for
>> consistency when it is opened.  This increases the bdrv_open() time
>> and hence the downtime of the VM.  So it is not ideal to open the
>> image file twice, even though there is no consistency problem.
>
> In general I really like understatements, but opening an image file
> twice isn't only "not ideal", but should be considered verboten.

Point taken.

>> I'll think about this some more, there are a couple of solutions like
>> keeping only the file descriptor around, introducing a flush command
>> that makes sure the file is in a clean state, or changing QED to not
>> do this.
>
> Changing the format drivers doesn't really look like the right solution.
>
> Keeping the fd around looks okay, we can probably achieve this by
> introducing a bdrv_reopen function. It means that we may need to reopen
> the format layer, but it can't really fail.

My concern is that this assumes a single file descriptor.  For vmdk
there may be multiple split files.

I'm almost starting to think bdrv_reopen() should be recursive down
the BlockDriverState tree.

Stefan

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

[Kevin Wolf]

Am 28.07.2011 15:10, schrieb Stefan Hajnoczi:
> On Thu, Jul 28, 2011 at 11:23 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 27.07.2011 16:51, schrieb Stefan Hajnoczi:
>>> I'll think about this some more, there are a couple of solutions like
>>> keeping only the file descriptor around, introducing a flush command
>>> that makes sure the file is in a clean state, or changing QED to not
>>> do this.
>>
>> Changing the format drivers doesn't really look like the right solution.
>>
>> Keeping the fd around looks okay, we can probably achieve this by
>> introducing a bdrv_reopen function. It means that we may need to reopen
>> the format layer, but it can't really fail.
> 
> My concern is that this assumes a single file descriptor.  For vmdk
> there may be multiple split files.
> 
> I'm almost starting to think bdrv_reopen() should be recursive down
> the BlockDriverState tree.

Yes, VMDK would have to call bdrv_reopen() for each file that it uses.

Kevin

[Qemu-devel] [V5 Patch 4/4]Qemu: Add commandline -drive option 'hostcache'

[Supriya Kannery]

qemu command option 'hostcache' added to -drive for block devices.
While starting a VM from qemu commandline, this option can be used 
for setting host cache usage for block data access. It is not 
allowed to specify both 'hostcache' and 'cache' options in the same 
commandline. User has to specify only one among these.

Signed-off-by: Supriya Kannery <supriyak@in.ibm.com>

---
 blockdev.c      |   13 +++++++++++++
 qemu-config.c   |    4 ++++
 qemu-options.hx |    2 +-
 3 files changed, 18 insertions(+), 1 deletion(-)

Index: qemu/blockdev.c
===================================================================
--- qemu.orig/blockdev.c
+++ qemu/blockdev.c
@@ -238,6 +238,7 @@ DriveInfo *drive_init(QemuOpts *opts, in
     DriveInfo *dinfo;
     int snapshot = 0;
     int ret;
+    int hostcache = 0;
 
     translation = BIOS_ATA_TRANSLATION_AUTO;
     media = MEDIA_DISK;
@@ -320,7 +321,19 @@ DriveInfo *drive_init(QemuOpts *opts, in
 	}
     }
 
+    if ((hostcache = qemu_opt_get_bool(opts, "hostcache", -1)) != -1) {
+        if (!hostcache) {
+            bdrv_flags |= BDRV_O_NOCACHE;
+        } else {
+            bdrv_flags &= ~BDRV_O_NOCACHE;
+        }
+    }
+
     if ((buf = qemu_opt_get(opts, "cache")) != NULL) {
+        if (hostcache != -1) {
+            error_report("'hostcache' and 'cache' cannot co-exist");
+            return NULL;
+        }
         if (!strcmp(buf, "off") || !strcmp(buf, "none")) {
             bdrv_flags |= BDRV_O_NOCACHE | BDRV_O_CACHE_WB;
         } else if (!strcmp(buf, "writeback")) {
Index: qemu/qemu-options.hx
===================================================================
--- qemu.orig/qemu-options.hx
+++ qemu/qemu-options.hx
@@ -135,7 +135,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
     "       [,cyls=c,heads=h,secs=s[,trans=t]][,snapshot=on|off]\n"
     "       [,cache=writethrough|writeback|none|unsafe][,format=f]\n"
     "       [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
-    "       [,readonly=on|off]\n"
+    "       [,readonly=on|off][,hostcache=on|off]\n"
     "                use 'file' as a drive image\n", QEMU_ARCH_ALL)
 STEXI
 @item -drive @var{option}[,@var{option}[,@var{option}[,...]]]
Index: qemu/qemu-config.c
===================================================================
--- qemu.orig/qemu-config.c
+++ qemu/qemu-config.c
@@ -84,6 +84,10 @@ static QemuOptsList qemu_drive_opts = {
             .name = "readonly",
             .type = QEMU_OPT_BOOL,
             .help = "open drive file as read-only",
+        },{
+            .name = "hostcache",
+            .type = QEMU_OPT_BOOL,
+            .help = "set or reset hostcache (on/off)"
         },
         { /* end of list */ }
     },