[Qemu-devel] [RFC PATCH 0/7] BlockBackends, nodes and guest devices

[Qemu-devel] [RFC PATCH 0/7] BlockBackends, nodes and guest devices

[Kevin Wolf]

I am relatively confident to say that everything that should use a
BlockBackend, does so by now. Almost all users create their own anonymous
BlockBackend internally and use that. The user configures the BB only
indirectly using the configuration methods of the user that the BB belongs to.

There is one exception, which are guest devices. There the user is expected to
manually set up a BlockBackend and pass it to the device. This requires that
users understand the difference between node and BlockBackends and manage both
kinds of objects. This is a rather nasty interface.

My goal is that we allow a user (management tool) to ignore that BlockBackends
exist as separate entities in qemu. Ideally we could fully make them an
implementation detail, but I'm not sure to which degree we can do that for
compatibility reasons. But what I'm pretty sure we can do is provide interfaces
that address everything using either node names or (qdev) device names, so that
you don't have to manage BlockBackends if you don't want to.

This involves several steps, and for most of them this series contains an
example patch that shows what this could look like:

1. Accept node-name in -device drive=... and create an internal anonymous BB
   for devices configured this way. This is the way to create devices that
   completely avoid legacy interfaces using the BB name.

2. Update all QMP commands touching block devices. There are two kinds of them,
   concerning either the guest device (which the BlockBackend is logically part
   of, even though it's not implemented this way) or the actual backend
   (BlockDriverState/node level)

   * Device level commands: Accept a guest device ID instead of BB name to
     identify the BlockBackend the command works on. As device IDs and BB names
     don't share a single namespace, we'll need a new QMP argument for this.

   * Node level commands: We need to complete the conversion that makes
     commands accept node names instead of BlockBackend names. In some places
     we intentionally allow only BlockBackends because we don't know if the
     command works in other places than the root. This is okay, but we can
     accept node names anyway. We just need to check that the node is a root
     node as expected.

3. Remove all BlockBackend options from blockdev-add. This has already happened
   partially (e.g. WCE flag), but at least id, rerror, werror are still there.
   This is a very incompatible change, but we declared blockdev-add
   experimental, so I think it's acceptable.

4. Add BlockBackend options as qdev properties to all block devices.

5. Add a way on the command line to create block nodes that have a node-name
   and don't have a BlockBackend. blockdev-add already supports this (and after
   implementing 3. it will be the only mode supported by blockdev-add), but we
   can't do this on the command line yet. You always get a BB with -drive.

   This might finally become the -blockdev we were talking about at the very
   beginning of the block layer generalisation work.

So this is my plan. It's pretty radical, but I think we really must do
something about our interfaces. Having nodes, BlockBackends and guest devices
to manage is just too much and doesn't really make sense. Making BlockBackends
visible in the external API essentially only as aliases for either node names
or guest devices (and that only for compatibility, not when using blockdev-add/
-blockdev) feels to me like the right thing to do.

But of course I'm aware that there probably isn't a clear right or wrong, and
that I might be missing important details, so this needs to be discussed in
advance before I go and implement the full thing instead of just small example
patches.

So please let me know what you guys think about this plan.

Kevin Wolf (7):
  block/qdev: Allow node name for drive properties
  block: Add blk_by_dev()
  qdev-monitor: Factor out find_device_state()
  qdev-monitor: Add blk_by_qdev_id()
  block: Accept device model name for blockdev-open/close-tray
  block: Accept node-name for block-stream
  block/qdev: Allow configuring WCE with qdev properties

 block/block-backend.c            | 19 +++++++++
 blockdev.c                       | 92 ++++++++++++++++++++++++++++------------
 hw/block/block.c                 | 16 +++++++
 hw/block/nvme.c                  |  1 +
 hw/block/virtio-blk.c            |  1 +
 hw/core/qdev-properties-system.c | 18 +++++++-
 hw/ide/qdev.c                    |  1 +
 hw/scsi/scsi-disk.c              |  1 +
 hw/usb/dev-storage.c             |  1 +
 include/hw/block/block.h         |  5 ++-
 include/sysemu/block-backend.h   |  2 +
 qapi/block-core.json             | 16 ++++---
 qdev-monitor.c                   | 34 +++++++++++++--
 qmp-commands.hx                  | 14 +++---
 14 files changed, 178 insertions(+), 43 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 1/7] block/qdev: Allow node name for drive properties

[Kevin Wolf]

If a node name instead of a BlockBackend name is specified as the driver
for a guest device, an anonymous BlockBackend is created now.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/core/qdev-properties-system.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index df38b8a..c5cc9cf 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -72,9 +72,18 @@ static void parse_drive(DeviceState *dev, const char *str, void **ptr,
                         const char *propname, Error **errp)
 {
     BlockBackend *blk;
+    bool blk_created = false;
 
     blk = blk_by_name(str);
     if (!blk) {
+        BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL);
+        if (bs) {
+            blk = blk_new();
+            blk_insert_bs(blk, bs);
+            blk_created = true;
+        }
+    }
+    if (!blk) {
         error_setg(errp, "Property '%s.%s' can't find value '%s'",
                    object_get_typename(OBJECT(dev)), propname, str);
         return;
@@ -91,9 +100,16 @@ static void parse_drive(DeviceState *dev, const char *str, void **ptr,
             error_setg(errp, "Drive '%s' is already in use by another device",
                        str);
         }
-        return;
+        goto fail;
     }
+
     *ptr = blk;
+
+fail:
+    if (blk_created) {
+        /* If we need to keep a reference, blk_attach_dev() took it */
+        blk_unref(blk);
+    }
 }
 
 static void release_drive(Object *obj, const char *name, void *opaque)
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 2/7] block: Add blk_by_dev()

[Kevin Wolf]

This finds a BlockBackend given the device model that is attached to it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c          | 19 +++++++++++++++++++
 include/sysemu/block-backend.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 34500e6..307cb17 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -544,6 +544,25 @@ void *blk_get_attached_dev(BlockBackend *blk)
 }
 
 /*
+ * Return the BlockBackend which has the device model @dev attached if it
+ * exists, else null.
+ *
+ * @dev must not be null.
+ */
+BlockBackend *blk_by_dev(void *dev)
+{
+    BlockBackend *blk = NULL;
+
+    assert(dev != NULL);
+    while ((blk = blk_all_next(blk)) != NULL) {
+        if (blk->dev == dev) {
+            return blk;
+        }
+    }
+    return NULL;
+}
+
+/*
  * Set @blk's device model callbacks to @ops.
  * @opaque is the opaque argument to pass to the callbacks.
  * This is for use by device models.
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index c04af8e..aec1471 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -110,6 +110,7 @@ int blk_attach_dev(BlockBackend *blk, void *dev);
 void blk_attach_dev_nofail(BlockBackend *blk, void *dev);
 void blk_detach_dev(BlockBackend *blk, void *dev);
 void *blk_get_attached_dev(BlockBackend *blk);
+BlockBackend *blk_by_dev(void *dev);
 void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque);
 int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
                           int count);
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 3/7] qdev-monitor: Factor out find_device_state()

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qdev-monitor.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index e19617f..bc0213f 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -801,7 +801,7 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
     object_unref(OBJECT(dev));
 }
 
-void qmp_device_del(const char *id, Error **errp)
+static DeviceState *find_device_state(const char *id, Error **errp)
 {
     Object *obj;
 
@@ -819,15 +819,23 @@ void qmp_device_del(const char *id, Error **errp)
     if (!obj) {
         error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                   "Device '%s' not found", id);
-        return;
+        return NULL;
     }
 
     if (!object_dynamic_cast(obj, TYPE_DEVICE)) {
         error_setg(errp, "%s is not a hotpluggable device", id);
-        return;
+        return NULL;
     }
 
-    qdev_unplug(DEVICE(obj), errp);
+    return DEVICE(obj);
+}
+
+void qmp_device_del(const char *id, Error **errp)
+{
+    DeviceState *dev = find_device_state(id, errp);
+    if (dev != NULL) {
+        qdev_unplug(dev, errp);
+    }
 }
 
 void qdev_machine_init(void)
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 4/7] qdev-monitor: Add blk_by_qdev_id()

[Kevin Wolf]

This finds the BlockBackend attached to the device model identified by
its qdev ID.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/sysemu/block-backend.h |  1 +
 qdev-monitor.c                 | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index aec1471..79226d3 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -111,6 +111,7 @@ void blk_attach_dev_nofail(BlockBackend *blk, void *dev);
 void blk_detach_dev(BlockBackend *blk, void *dev);
 void *blk_get_attached_dev(BlockBackend *blk);
 BlockBackend *blk_by_dev(void *dev);
+BlockBackend *blk_by_qdev_id(const char *id, Error **errp);
 void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops, void *opaque);
 int blk_pread_unthrottled(BlockBackend *blk, int64_t offset, uint8_t *buf,
                           int count);
diff --git a/qdev-monitor.c b/qdev-monitor.c
index bc0213f..4f78ecb 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -28,6 +28,7 @@
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
 #include "qemu/help_option.h"
+#include "sysemu/block-backend.h"
 
 /*
  * Aliases were a bad idea from the start.  Let's keep them
@@ -838,6 +839,23 @@ void qmp_device_del(const char *id, Error **errp)
     }
 }
 
+BlockBackend *blk_by_qdev_id(const char *id, Error **errp)
+{
+    DeviceState *dev;
+    BlockBackend *blk;
+
+    dev = find_device_state(id, errp);
+    if (dev == NULL) {
+        return NULL;
+    }
+
+    blk = blk_by_dev(dev);
+    if (!blk) {
+        error_setg(errp, "Device does not have a block device backend");
+    }
+    return blk;
+}
+
 void qdev_machine_init(void)
 {
     qdev_get_peripheral_anon();
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 5/7] block: Accept device model name for blockdev-open/close-tray

[Kevin Wolf]

This is an example conversion of a QMP command that operates on the
BlockBackend level to accept both the device model name (which is
supposed to become the primary interface) and the BlockBackend name.

Naming suggestions for the new QMP field are welcome. The obvious one
would be "device", but that's already taken...

We'll also want QAPI to understand that exactly one of the two fields
must be given, so we can remove/don't have to add explicit code in the
command implementations to check that.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c           | 60 +++++++++++++++++++++++++++++++++++++++-------------
 qapi/block-core.json | 14 ++++++++----
 qmp-commands.hx      | 12 +++++++----
 3 files changed, 63 insertions(+), 23 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 3a104a0..24c4d0a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -56,7 +56,8 @@
 static QTAILQ_HEAD(, BlockDriverState) monitor_bdrv_states =
     QTAILQ_HEAD_INITIALIZER(monitor_bdrv_states);
 
-static int do_open_tray(const char *device, bool force, Error **errp);
+static int do_open_tray(const char *blk_name, const char *qdev_id,
+                        bool force, Error **errp);
 
 static const char *const if_name[IF_COUNT] = {
     [IF_NONE] = "none",
@@ -1172,6 +1173,29 @@ fail:
     return dinfo;
 }
 
+static BlockBackend *qmp_get_blk(const char *blk_name, const char *qdev_id,
+                                 Error **errp)
+{
+    BlockBackend *blk;
+
+    if (!blk_name == !qdev_id) {
+        error_setg(errp, "Need exactly one of 'device' and 'id'");
+        return NULL;
+    }
+
+    if (qdev_id) {
+        blk = blk_by_qdev_id(qdev_id, errp);
+    } else {
+        blk = blk_by_name(blk_name);
+        if (blk == NULL) {
+            error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                      "Device '%s' not found", blk_name);
+        }
+    }
+
+    return blk;
+}
+
 void hmp_commit(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
@@ -2285,7 +2309,7 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
         force = false;
     }
 
-    rc = do_open_tray(device, force, &local_err);
+    rc = do_open_tray(device, NULL, force, &local_err);
     if (rc && rc != -ENOSYS) {
         error_propagate(errp, local_err);
         return;
@@ -2330,15 +2354,15 @@ void qmp_block_passwd(bool has_device, const char *device,
  * If the guest was asked to open the tray, return -EINPROGRESS.
  * Else, return 0.
  */
-static int do_open_tray(const char *device, bool force, Error **errp)
+static int do_open_tray(const char *blk_name, const char *qdev_id,
+                        bool force, Error **errp)
 {
     BlockBackend *blk;
+    const char *device = qdev_id ?: blk_name;
     bool locked;
 
-    blk = blk_by_name(device);
+    blk = qmp_get_blk(blk_name, qdev_id, errp);
     if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
         return -ENODEV;
     }
 
@@ -2374,7 +2398,9 @@ static int do_open_tray(const char *device, bool force, Error **errp)
     return 0;
 }
 
-void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
+void qmp_blockdev_open_tray(bool has_device, const char *device,
+                            bool has_id, const char *id,
+                            bool has_force, bool force,
                             Error **errp)
 {
     Error *local_err = NULL;
@@ -2383,7 +2409,9 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
     if (!has_force) {
         force = false;
     }
-    rc = do_open_tray(device, force, &local_err);
+    rc = do_open_tray(has_device ? device : NULL,
+                      has_id ? id : NULL,
+                      force, &local_err);
     if (rc && rc != -ENOSYS && rc != -EINPROGRESS) {
         error_propagate(errp, local_err);
         return;
@@ -2391,19 +2419,21 @@ void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
     error_free(local_err);
 }
 
-void qmp_blockdev_close_tray(const char *device, Error **errp)
+void qmp_blockdev_close_tray(bool has_device, const char *device,
+                             bool has_id, const char *id,
+                             Error **errp)
 {
     BlockBackend *blk;
 
-    blk = blk_by_name(device);
+    blk = qmp_get_blk(has_device ? device : NULL,
+                      has_id ? id : NULL,
+                      errp);
     if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
         return;
     }
 
     if (!blk_dev_has_removable_media(blk)) {
-        error_setg(errp, "Device '%s' is not removable", device);
+        error_setg(errp, "Device '%s' is not removable", device ?: id);
         return;
     }
 
@@ -2599,7 +2629,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
         goto fail;
     }
 
-    rc = do_open_tray(device, false, &err);
+    rc = do_open_tray(device, NULL, false, &err);
     if (rc && rc != -ENOSYS) {
         error_propagate(errp, err);
         goto fail;
@@ -2621,7 +2651,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
 
     blk_apply_root_state(blk, medium_bs);
 
-    qmp_blockdev_close_tray(device, errp);
+    qmp_blockdev_close_tray(true, device, false, NULL, errp);
 
 fail:
     /* If the medium has been inserted, the device has its own reference, so
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 98a20d2..15669b3 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2201,7 +2201,9 @@
 #   to it
 # - if the guest device does not have an actual tray
 #
-# @device: block device name
+# @device:  block device name (deprecated, use @id instead)
+#
+# @id:      the name or QOM path of the guest device
 #
 # @force:  #optional if false (the default), an eject request will be sent to
 #          the guest if it has locked the tray (and the tray will not be opened
@@ -2211,7 +2213,8 @@
 # Since: 2.5
 ##
 { 'command': 'blockdev-open-tray',
-  'data': { 'device': 'str',
+  'data': { '*device': 'str',
+            '*id': 'str',
             '*force': 'bool' } }
 
 ##
@@ -2223,12 +2226,15 @@
 #
 # If the tray was already closed before, this will be a no-op.
 #
-# @device: block device name
+# @device:  block device name (deprecated, use @id instead)
+#
+# @id:      the name or QOM path of the guest device
 #
 # Since: 2.5
 ##
 { 'command': 'blockdev-close-tray',
-  'data': { 'device': 'str' } }
+  'data': { '*device': 'str',
+            '*id': 'str' } }
 
 ##
 # @x-blockdev-remove-medium:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b444c20..ae0105f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4255,7 +4255,7 @@ EQMP
 
     {
         .name       = "blockdev-open-tray",
-        .args_type  = "device:s,force:b?",
+        .args_type  = "device:s?,id:s?,force:b?",
         .mhandler.cmd_new = qmp_marshal_blockdev_open_tray,
     },
 
@@ -4280,7 +4280,9 @@ which no such event will be generated, these include:
 
 Arguments:
 
-- "device": block device name (json-string)
+- "device": block device name (deprecated, use @id instead)
+            (json-string, optional)
+- "id": the name or QOM path of the guest device (json-string, optional)
 - "force": if false (the default), an eject request will be sent to the guest if
            it has locked the tray (and the tray will not be opened immediately);
            if true, the tray will be opened regardless of whether it is locked
@@ -4303,7 +4305,7 @@ EQMP
 
     {
         .name       = "blockdev-close-tray",
-        .args_type  = "device:s",
+        .args_type  = "device:s?,id:s?",
         .mhandler.cmd_new = qmp_marshal_blockdev_close_tray,
     },
 
@@ -4319,7 +4321,9 @@ If the tray was already closed before, this will be a no-op.
 
 Arguments:
 
-- "device": block device name (json-string)
+- "device": block device name (deprecated, use @id instead)
+            (json-string, optional)
+- "id": the name or QOM path of the guest device (json-string, optional)
 
 Example:
 
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 6/7] block: Accept node-name for block-stream

[Kevin Wolf]

In order to remove the necessity to use BlockBackend names in the
external API, we want to allow node-names everywhere. This is an example
conversion that accepts a node-name without lifting the restriction that
we're operating at a root node. Changing QMP commands so that they can
work in the middle of the graph is a completely orthogonal thing.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c           | 32 ++++++++++++++++++++------------
 qapi/block-core.json |  2 +-
 qmp-commands.hx      |  2 +-
 3 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 24c4d0a..bd93190 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1196,6 +1196,23 @@ static BlockBackend *qmp_get_blk(const char *blk_name, const char *qdev_id,
     return blk;
 }
 
+static BlockDriverState *qmp_get_root_bs(const char *name, Error **errp)
+{
+    BlockDriverState *bs;
+
+    bs = bdrv_lookup_bs(name, name, errp);
+    if (bs == NULL) {
+        return NULL;
+    }
+
+    if (!bdrv_has_blk(bs)) {
+        error_setg(errp, "Need a root block node");
+        return NULL;
+    }
+
+    return bs;
+}
+
 void hmp_commit(Monitor *mon, const QDict *qdict)
 {
     const char *device = qdict_get_str(qdict, "device");
@@ -3041,7 +3058,6 @@ void qmp_block_stream(const char *device,
                       bool has_on_error, BlockdevOnError on_error,
                       Error **errp)
 {
-    BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *base_bs = NULL;
     AioContext *aio_context;
@@ -3052,22 +3068,14 @@ void qmp_block_stream(const char *device,
         on_error = BLOCKDEV_ON_ERROR_REPORT;
     }
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
-                  "Device '%s' not found", device);
+    bs = qmp_get_root_bs(device, errp);
+    if (!bs) {
         return;
     }
 
-    aio_context = blk_get_aio_context(blk);
+    aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
-    if (!blk_is_available(blk)) {
-        error_setg(errp, "Device '%s' has no medium", device);
-        goto out;
-    }
-    bs = blk_bs(blk);
-
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) {
         goto out;
     }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 15669b3..60fa70a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1413,7 +1413,7 @@
 # On successful completion the image file is updated to drop the backing file
 # and the BLOCK_JOB_COMPLETED event is emitted.
 #
-# @device: the device name
+# @device: the device name or node-name of a root node
 #
 # @base:   #optional the common backing file name
 #
diff --git a/qmp-commands.hx b/qmp-commands.hx
index ae0105f..d4c7c93 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1118,7 +1118,7 @@ Copy data from a backing file into a block device.
 
 Arguments:
 
-- "device": The device's ID, must be unique (json-string)
+- "device": The device name or node-name of a root node (json-string)
 - "base": The file name of the backing image above which copying starts
           (json-string, optional)
 - "backing-file": The backing file string to write into the active layer. This
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH 7/7] block/qdev: Allow configuring WCE with qdev properties

[Kevin Wolf]

As cache.writeback is a BlockBackend property and as such more related
to the guest device than the BlockDriverState, we already removed it
from the blockdev-add interface. This patch adds the new way to set it,
as a qdev property of the corresponding guest device.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/block/block.c         | 16 ++++++++++++++++
 hw/block/nvme.c          |  1 +
 hw/block/virtio-blk.c    |  1 +
 hw/ide/qdev.c            |  1 +
 hw/scsi/scsi-disk.c      |  1 +
 hw/usb/dev-storage.c     |  1 +
 include/hw/block/block.h |  5 ++++-
 7 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index 97a59d4..396b0d5 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -51,6 +51,22 @@ void blkconf_blocksizes(BlockConf *conf)
     }
 }
 
+void blkconf_apply_backend_options(BlockConf *conf)
+{
+    BlockBackend *blk = conf->blk;
+    bool wce;
+
+    switch (conf->wce) {
+    case ON_OFF_AUTO_ON:    wce = true; break;
+    case ON_OFF_AUTO_OFF:   wce = false; break;
+    case ON_OFF_AUTO_AUTO:  wce = blk_enable_write_cache(blk); break;
+    default:
+        abort();
+    }
+
+    blk_set_enable_write_cache(blk, wce);
+}
+
 void blkconf_geometry(BlockConf *conf, int *ptrans,
                       unsigned cyls_max, unsigned heads_max, unsigned secs_max,
                       Error **errp)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9faad29..8234669 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -803,6 +803,7 @@ static int nvme_init(PCIDevice *pci_dev)
         return -1;
     }
     blkconf_blocksizes(&n->conf);
+    blkconf_apply_backend_options(&n->conf);
 
     pci_conf = pci_dev->config;
     pci_conf[PCI_INTERRUPT_PIN] = 1;
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 284e646..6dc394b 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -880,6 +880,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
     blkconf_blocksizes(&conf->conf);
+    blkconf_apply_backend_options(&conf->conf);
 
     virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
                 sizeof(struct virtio_blk_config));
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 6842a55..e2badc3 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -180,6 +180,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
             return -1;
         }
     }
+    blkconf_apply_backend_options(&dev->conf);
 
     if (ide_init_drive(s, dev->conf.blk, kind,
                        dev->version, dev->serial, dev->model, dev->wwn,
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 36f8a85..8c26a4e 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2309,6 +2309,7 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
             return;
         }
     }
+    blkconf_apply_backend_options(&dev->conf);
 
     if (s->qdev.conf.discard_granularity == -1) {
         s->qdev.conf.discard_granularity =
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 9fd00df..44a7475 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -637,6 +637,7 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
 
     blkconf_serial(&s->conf, &dev->serial);
     blkconf_blocksizes(&s->conf);
+    blkconf_apply_backend_options(&s->conf);
 
     /*
      * Hack alert: this pretends to be a block device, but it's really
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 984660e..e511dc3 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -25,6 +25,7 @@ typedef struct BlockConf {
     uint32_t discard_granularity;
     /* geometry, not all devices use this */
     uint32_t cyls, heads, secs;
+    OnOffAuto wce;
 } BlockConf;
 
 static inline unsigned int get_physical_block_exp(BlockConf *conf)
@@ -49,7 +50,8 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
     DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),  \
     DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
     DEFINE_PROP_UINT32("discard_granularity", _state, \
-                       _conf.discard_granularity, -1)
+                       _conf.discard_granularity, -1), \
+    DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce, ON_OFF_AUTO_AUTO)
 
 #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)      \
     DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \
@@ -63,6 +65,7 @@ void blkconf_geometry(BlockConf *conf, int *trans,
                       unsigned cyls_max, unsigned heads_max, unsigned secs_max,
                       Error **errp);
 void blkconf_blocksizes(BlockConf *conf);
+void blkconf_apply_backend_options(BlockConf *conf);
 
 /* Hard disk geometry */
 
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH 0/7] BlockBackends, nodes and guest devices

[Paolo Bonzini]

On 23/06/2016 16:36, Kevin Wolf wrote:
> But of course I'm aware that there probably isn't a clear right or wrong, and
> that I might be missing important details, so this needs to be discussed in
> advance before I go and implement the full thing instead of just small example
> patches.
> 
> So please let me know what you guys think about this plan.

Apart from the details that we cannot see yet from 10.000 feet, I think
this makes so much sense.  Good job coming up with the plan!

Paolo

Re: [Qemu-devel] [RFC PATCH 1/7] block/qdev: Allow node name for drive properties

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2158 bytes --]

On 06/23/2016 08:36 AM, Kevin Wolf wrote:
> If a node name instead of a BlockBackend name is specified as the driver
> for a guest device, an anonymous BlockBackend is created now.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/core/qdev-properties-system.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> index df38b8a..c5cc9cf 100644
> --- a/hw/core/qdev-properties-system.c
> +++ b/hw/core/qdev-properties-system.c
> @@ -72,9 +72,18 @@ static void parse_drive(DeviceState *dev, const char *str, void **ptr,
>                          const char *propname, Error **errp)
>  {
>      BlockBackend *blk;
> +    bool blk_created = false;
>  
>      blk = blk_by_name(str);
>      if (!blk) {
> +        BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL);

So BB name takes priority, but if one is not found, you try a BDS lookup
(node name) and create an anonymous BB to match.  Seems okay.

> +        if (bs) {
> +            blk = blk_new();
> +            blk_insert_bs(blk, bs);
> +            blk_created = true;
> +        }
> +    }
> +    if (!blk) {
>          error_setg(errp, "Property '%s.%s' can't find value '%s'",
>                     object_get_typename(OBJECT(dev)), propname, str);
>          return;

This return is safe, but looks a bit odd...

> @@ -91,9 +100,16 @@ static void parse_drive(DeviceState *dev, const char *str, void **ptr,
>              error_setg(errp, "Drive '%s' is already in use by another device",
>                         str);
>          }
> -        return;
> +        goto fail;

...given that you had to convert this return to a goto.

>      }
> +
>      *ptr = blk;
> +
> +fail:
> +    if (blk_created) {
> +        /* If we need to keep a reference, blk_attach_dev() took it */
> +        blk_unref(blk);
> +    }
>  }
>  
>  static void release_drive(Object *obj, const char *name, void *opaque)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [RFC PATCH 2/7] block: Add blk_by_dev()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 478 bytes --]

On 06/23/2016 08:36 AM, Kevin Wolf wrote:
> This finds a BlockBackend given the device model that is attached to it.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c          | 19 +++++++++++++++++++
>  include/sysemu/block-backend.h |  1 +
>  2 files changed, 20 insertions(+)


Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [RFC PATCH 3/7] qdev-monitor: Factor out find_device_state()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 350 bytes --]

On 06/23/2016 08:36 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qdev-monitor.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [RFC PATCH 1/7] block/qdev: Allow node name for drive properties

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 2671 bytes --]

Am 24.06.2016 um 19:35 hat Eric Blake geschrieben:
> On 06/23/2016 08:36 AM, Kevin Wolf wrote:
> > If a node name instead of a BlockBackend name is specified as the driver
> > for a guest device, an anonymous BlockBackend is created now.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  hw/core/qdev-properties-system.c | 18 +++++++++++++++++-
> >  1 file changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
> > index df38b8a..c5cc9cf 100644
> > --- a/hw/core/qdev-properties-system.c
> > +++ b/hw/core/qdev-properties-system.c
> > @@ -72,9 +72,18 @@ static void parse_drive(DeviceState *dev, const char *str, void **ptr,
> >                          const char *propname, Error **errp)
> >  {
> >      BlockBackend *blk;
> > +    bool blk_created = false;
> >  
> >      blk = blk_by_name(str);
> >      if (!blk) {
> > +        BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL);
> 
> So BB name takes priority, but if one is not found, you try a BDS lookup
> (node name) and create an anonymous BB to match.  Seems okay.

Well, there's no real priority here because BBs and BDSes share the same
namespace, so it can only be one or the other. I just can't pass both to
bdrv_lookup_bs() because I need the BB and not just its root BDS if it's
a BB name, so I have to check one after the other.

> > +        if (bs) {
> > +            blk = blk_new();
> > +            blk_insert_bs(blk, bs);
> > +            blk_created = true;
> > +        }
> > +    }
> > +    if (!blk) {
> >          error_setg(errp, "Property '%s.%s' can't find value '%s'",
> >                     object_get_typename(OBJECT(dev)), propname, str);
> >          return;
> 
> This return is safe, but looks a bit odd...
> 
> > @@ -91,9 +100,16 @@ static void parse_drive(DeviceState *dev, const char *str, void **ptr,
> >              error_setg(errp, "Drive '%s' is already in use by another device",
> >                         str);
> >          }
> > -        return;
> > +        goto fail;
> 
> ...given that you had to convert this return to a goto.

Hm, okay. I think it's pretty common to have early error paths return
directly and later ones use goto. But blk_unref(NULL) is allowed, so in
theory I could change that.

Kevin

> >      }
> > +
> >      *ptr = blk;
> > +
> > +fail:
> > +    if (blk_created) {
> > +        /* If we need to keep a reference, blk_attach_dev() took it */
> > +        blk_unref(blk);
> > +    }
> >  }
> >  
> >  static void release_drive(Object *obj, const char *name, void *opaque)

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [RFC PATCH 4/7] qdev-monitor: Add blk_by_qdev_id()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 493 bytes --]

On 06/23/2016 08:36 AM, Kevin Wolf wrote:
> This finds the BlockBackend attached to the device model identified by
> its qdev ID.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/sysemu/block-backend.h |  1 +
>  qdev-monitor.c                 | 18 ++++++++++++++++++
>  2 files changed, 19 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [RFC PATCH 5/7] block: Accept device model name for blockdev-open/close-tray

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2695 bytes --]

On 06/23/2016 08:36 AM, Kevin Wolf wrote:
> This is an example conversion of a QMP command that operates on the
> BlockBackend level to accept both the device model name (which is
> supposed to become the primary interface) and the BlockBackend name.
> 
> Naming suggestions for the new QMP field are welcome. The obvious one
> would be "device", but that's already taken...
> 
> We'll also want QAPI to understand that exactly one of the two fields
> must be given, so we can remove/don't have to add explicit code in the
> command implementations to check that.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c           | 60 +++++++++++++++++++++++++++++++++++++++-------------
>  qapi/block-core.json | 14 ++++++++----
>  qmp-commands.hx      | 12 +++++++----
>  3 files changed, 63 insertions(+), 23 deletions(-)
> 

> +++ b/qapi/block-core.json
> @@ -2201,7 +2201,9 @@
>  #   to it
>  # - if the guest device does not have an actual tray
>  #
> -# @device: block device name
> +# @device:  block device name (deprecated, use @id instead)
> +#
> +# @id:      the name or QOM path of the guest device
>  #

Is this something we want to rush into 2.7, or is it big enough to
convert all the commands that we may want to wait for 2.8?  Either way,
we'll probably want to add a '(since 2.x)' hint, and maybe come up with
a standard way to document mutually-exclusive members given that
Marc-Andre is trying to automate documentation from the .json file.

>  # @force:  #optional if false (the default), an eject request will be sent to
>  #          the guest if it has locked the tray (and the tray will not be opened
> @@ -2211,7 +2213,8 @@
>  # Since: 2.5
>  ##
>  { 'command': 'blockdev-open-tray',
> -  'data': { 'device': 'str',
> +  'data': { '*device': 'str',
> +            '*id': 'str',
>              '*force': 'bool' } }
>  

Your idea about having some particular QAPI shorthand for marking two
mutually-exclusive members may indeed be worth addressing, since we're
going to have more cases of it.  But off-hand, I don't know what syntax
would be best, particularly if we want to keep QAPI close to JSON.


> +++ b/qmp-commands.hx
> @@ -4255,7 +4255,7 @@ EQMP
>  
>      {
>          .name       = "blockdev-open-tray",
> -        .args_type  = "device:s,force:b?",
> +        .args_type  = "device:s?,id:s?,force:b?",
>          .mhandler.cmd_new = qmp_marshal_blockdev_open_tray,

On the bright side, once Marc-Andre's work goes in, we won't have
qmp-commands.hx to worry about.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [RFC PATCH 6/7] block: Accept node-name for block-stream

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

On 06/23/2016 08:36 AM, Kevin Wolf wrote:
> In order to remove the necessity to use BlockBackend names in the
> external API, we want to allow node-names everywhere. This is an example
> conversion that accepts a node-name without lifting the restriction that
> we're operating at a root node. Changing QMP commands so that they can
> work in the middle of the graph is a completely orthogonal thing.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c           | 32 ++++++++++++++++++++------------
>  qapi/block-core.json |  2 +-
>  qmp-commands.hx      |  2 +-
>  3 files changed, 22 insertions(+), 14 deletions(-)
> 

Looks reasonable; this one's not as controversial as 5/7 :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [RFC PATCH 7/7] block/qdev: Allow configuring WCE with qdev properties

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

On 06/23/2016 08:36 AM, Kevin Wolf wrote:
> As cache.writeback is a BlockBackend property and as such more related
> to the guest device than the BlockDriverState, we already removed it
> from the blockdev-add interface. This patch adds the new way to set it,
> as a qdev property of the corresponding guest device.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

Commit message could do with an example (command line or QMP command) on
setting the property, but the idea makes sense.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [RFC PATCH 5/7] block: Accept device model name for blockdev-open/close-tray

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 4488 bytes --]

Am 24.06.2016 um 22:39 hat Eric Blake geschrieben:
> On 06/23/2016 08:36 AM, Kevin Wolf wrote:
> > This is an example conversion of a QMP command that operates on the
> > BlockBackend level to accept both the device model name (which is
> > supposed to become the primary interface) and the BlockBackend name.
> > 
> > Naming suggestions for the new QMP field are welcome. The obvious one
> > would be "device", but that's already taken...
> > 
> > We'll also want QAPI to understand that exactly one of the two fields
> > must be given, so we can remove/don't have to add explicit code in the
> > command implementations to check that.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  blockdev.c           | 60 +++++++++++++++++++++++++++++++++++++++-------------
> >  qapi/block-core.json | 14 ++++++++----
> >  qmp-commands.hx      | 12 +++++++----
> >  3 files changed, 63 insertions(+), 23 deletions(-)
> > 
> 
> > +++ b/qapi/block-core.json
> > @@ -2201,7 +2201,9 @@
> >  #   to it
> >  # - if the guest device does not have an actual tray
> >  #
> > -# @device: block device name
> > +# @device:  block device name (deprecated, use @id instead)
> > +#
> > +# @id:      the name or QOM path of the guest device
> >  #
> 
> Is this something we want to rush into 2.7, or is it big enough to
> convert all the commands that we may want to wait for 2.8?  Either way,
> we'll probably want to add a '(since 2.x)' hint, and maybe come up with
> a standard way to document mutually-exclusive members given that
> Marc-Andre is trying to automate documentation from the .json file.

I would definitely want to convert all commands at the same time. But if
we know what we want, I think we can still make 2.7.

The QAPI syntax won't be ready, I guess, but we can keep the two
optional fields and just document the requirements for now, and then
clean it up in 2.8. That would be an invisible change for clients. Hm,
except introspection maybe.

> >  # @force:  #optional if false (the default), an eject request will be sent to
> >  #          the guest if it has locked the tray (and the tray will not be opened
> > @@ -2211,7 +2213,8 @@
> >  # Since: 2.5
> >  ##
> >  { 'command': 'blockdev-open-tray',
> > -  'data': { 'device': 'str',
> > +  'data': { '*device': 'str',
> > +            '*id': 'str',
> >              '*force': 'bool' } }
> >  
> 
> Your idea about having some particular QAPI shorthand for marking two
> mutually-exclusive members may indeed be worth addressing, since we're
> going to have more cases of it.  But off-hand, I don't know what syntax
> would be best, particularly if we want to keep QAPI close to JSON.

Hm, I wonder if this is closer to a union or an alternate. Maybe like
this:

{ 'alternate': 'DeviceOrID',
  'data': { 'device': 'str' ,
            'id', 'str'},
  'discrimiate-by-key': true }

{ 'command': 'blockdev-open-tray',
  'data': { 'device': 'DeviceOrID', '*force': 'bool' } }

This would accept a command like:

{ "execute": "blockdev-open-tray", "arguments": { "id": "hda" } }

And turn it into a C struct:
{
    .device = {
        .type = DEVICE_OR_ID_KIND_ID,
        .u = {
            .device = NULL,
            .id = "hda",
        }
    },
    .has_force = false,
    .force = <undefined>
}

Though that might end up not being easier to implement in the command
handlers than what we have today. But it would be a precise description
of the protocol in the schema at least.

With two strings, it would be nicer to generate only a single string
field and the type in addition, but if we want to keep it generic and
allow e.g. an alternate of two different kinds of dicts or a string, I
think we need the structure as above.

Hm...

Different dicts makes me wonder if we want to have alternate dicts
embedded in the parent (kind of like flat unions).


So yes, that looks to me as if the QAPI part isn't for 2.7...

> > +++ b/qmp-commands.hx
> > @@ -4255,7 +4255,7 @@ EQMP
> >  
> >      {
> >          .name       = "blockdev-open-tray",
> > -        .args_type  = "device:s,force:b?",
> > +        .args_type  = "device:s?,id:s?,force:b?",
> >          .mhandler.cmd_new = qmp_marshal_blockdev_open_tray,
> 
> On the bright side, once Marc-Andre's work goes in, we won't have
> qmp-commands.hx to worry about.

Oh, nice. I forgot this part at first and wondered why it didn't work.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [RFC PATCH 0/7] BlockBackends, nodes and guest devices

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4461 bytes --]

On 23.06.2016 16:36, Kevin Wolf wrote:
> I am relatively confident to say that everything that should use a
> BlockBackend, does so by now. Almost all users create their own anonymous
> BlockBackend internally and use that. The user configures the BB only
> indirectly using the configuration methods of the user that the BB belongs to.
> 
> There is one exception, which are guest devices. There the user is expected to
> manually set up a BlockBackend and pass it to the device. This requires that
> users understand the difference between node and BlockBackends and manage both
> kinds of objects. This is a rather nasty interface.
> 
> My goal is that we allow a user (management tool) to ignore that BlockBackends
> exist as separate entities in qemu. Ideally we could fully make them an
> implementation detail, but I'm not sure to which degree we can do that for
> compatibility reasons. But what I'm pretty sure we can do is provide interfaces
> that address everything using either node names or (qdev) device names, so that
> you don't have to manage BlockBackends if you don't want to.
> 
> This involves several steps, and for most of them this series contains an
> example patch that shows what this could look like:
> 
> 1. Accept node-name in -device drive=... and create an internal anonymous BB
>    for devices configured this way. This is the way to create devices that
>    completely avoid legacy interfaces using the BB name.
> 
> 2. Update all QMP commands touching block devices. There are two kinds of them,
>    concerning either the guest device (which the BlockBackend is logically part
>    of, even though it's not implemented this way) or the actual backend
>    (BlockDriverState/node level)
> 
>    * Device level commands: Accept a guest device ID instead of BB name to
>      identify the BlockBackend the command works on. As device IDs and BB names
>      don't share a single namespace, we'll need a new QMP argument for this.
> 
>    * Node level commands: We need to complete the conversion that makes
>      commands accept node names instead of BlockBackend names. In some places
>      we intentionally allow only BlockBackends because we don't know if the
>      command works in other places than the root. This is okay, but we can
>      accept node names anyway. We just need to check that the node is a root
>      node as expected.
> 
> 3. Remove all BlockBackend options from blockdev-add. This has already happened
>    partially (e.g. WCE flag), but at least id, rerror, werror are still there.
>    This is a very incompatible change, but we declared blockdev-add
>    experimental, so I think it's acceptable.
> 
> 4. Add BlockBackend options as qdev properties to all block devices.
> 
> 5. Add a way on the command line to create block nodes that have a node-name
>    and don't have a BlockBackend. blockdev-add already supports this (and after
>    implementing 3. it will be the only mode supported by blockdev-add), but we
>    can't do this on the command line yet. You always get a BB with -drive.
> 
>    This might finally become the -blockdev we were talking about at the very
>    beginning of the block layer generalisation work.
> 
> So this is my plan. It's pretty radical, but I think we really must do
> something about our interfaces. Having nodes, BlockBackends and guest devices
> to manage is just too much and doesn't really make sense. Making BlockBackends
> visible in the external API essentially only as aliases for either node names
> or guest devices (and that only for compatibility, not when using blockdev-add/
> -blockdev) feels to me like the right thing to do.
> 
> But of course I'm aware that there probably isn't a clear right or wrong, and
> that I might be missing important details, so this needs to be discussed in
> advance before I go and implement the full thing instead of just small example
> patches.
> 
> So please let me know what you guys think about this plan.

I agree that the current interface is confusing because of the BB layer
that the user (unnecessarily) needs to care about. I don't think we
absolutely have to do something about it, but your plan is reasonable
and I do consider it an improvement.

I'm not sure if WCE truly is a guest device property, but adding it as a
guest device property and then syncing it to the BB makes sense (as done
in patch 7).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [RFC PATCH 0/7] BlockBackends, nodes and guest devices

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 429 bytes --]

Am 27.06.2016 um 18:13 hat Max Reitz geschrieben:
> I'm not sure if WCE truly is a guest device property, but adding it as a
> guest device property and then syncing it to the BB makes sense (as done
> in patch 7).

It is truly a guest device property. Physical hardware can have a
volatile write cache and software can actively enable or disable it. We
support this in all relevant guest devices (SCSI, virtio-blk, IDE).

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [RFC PATCH 0/7] BlockBackends, nodes and guest devices

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

On 27.06.2016 18:38, Kevin Wolf wrote:
> Am 27.06.2016 um 18:13 hat Max Reitz geschrieben:
>> I'm not sure if WCE truly is a guest device property, but adding it as a
>> guest device property and then syncing it to the BB makes sense (as done
>> in patch 7).
> 
> It is truly a guest device property. Physical hardware can have a
> volatile write cache and software can actively enable or disable it. We
> support this in all relevant guest devices (SCSI, virtio-blk, IDE).

Yes, but I consider it a host property, too, in case you just want
write-through on your host. That's why I think syncing the property
makes sense.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [Qemu-block] [RFC PATCH 0/7] BlockBackends, nodes and guest devices

[John Snow]

No oxford comma in the subject? :)

On 06/23/2016 10:36 AM, Kevin Wolf wrote:
> I am relatively confident to say that everything that should use a
> BlockBackend, does so by now. Almost all users create their own anonymous
> BlockBackend internally and use that. The user configures the BB only
> indirectly using the configuration methods of the user that the BB belongs to.
> 
> There is one exception, which are guest devices. There the user is expected to
> manually set up a BlockBackend and pass it to the device. This requires that
> users understand the difference between node and BlockBackends and manage both
> kinds of objects. This is a rather nasty interface.
> 
> My goal is that we allow a user (management tool) to ignore that BlockBackends
> exist as separate entities in qemu. Ideally we could fully make them an
> implementation detail, but I'm not sure to which degree we can do that for
> compatibility reasons. But what I'm pretty sure we can do is provide interfaces
> that address everything using either node names or (qdev) device names, so that
> you don't have to manage BlockBackends if you don't want to.
> 
> This involves several steps, and for most of them this series contains an
> example patch that shows what this could look like:
> 
> 1. Accept node-name in -device drive=... and create an internal anonymous BB
>    for devices configured this way. This is the way to create devices that
>    completely avoid legacy interfaces using the BB name.
> 
> 2. Update all QMP commands touching block devices. There are two kinds of them,
>    concerning either the guest device (which the BlockBackend is logically part
>    of, even though it's not implemented this way) or the actual backend
>    (BlockDriverState/node level)
> 
>    * Device level commands: Accept a guest device ID instead of BB name to
>      identify the BlockBackend the command works on. As device IDs and BB names
>      don't share a single namespace, we'll need a new QMP argument for this.
> 
>    * Node level commands: We need to complete the conversion that makes
>      commands accept node names instead of BlockBackend names. In some places
>      we intentionally allow only BlockBackends because we don't know if the
>      command works in other places than the root. This is okay, but we can
>      accept node names anyway. We just need to check that the node is a root
>      node as expected.
> 
> 3. Remove all BlockBackend options from blockdev-add. This has already happened
>    partially (e.g. WCE flag), but at least id, rerror, werror are still there.
>    This is a very incompatible change, but we declared blockdev-add
>    experimental, so I think it's acceptable.
> 
> 4. Add BlockBackend options as qdev properties to all block devices.
> 
> 5. Add a way on the command line to create block nodes that have a node-name
>    and don't have a BlockBackend. blockdev-add already supports this (and after
>    implementing 3. it will be the only mode supported by blockdev-add), but we
>    can't do this on the command line yet. You always get a BB with -drive.
> 
>    This might finally become the -blockdev we were talking about at the very
>    beginning of the block layer generalisation work.
> 
> So this is my plan. It's pretty radical, but I think we really must do
> something about our interfaces. Having nodes, BlockBackends and guest devices
> to manage is just too much and doesn't really make sense. Making BlockBackends
> visible in the external API essentially only as aliases for either node names
> or guest devices (and that only for compatibility, not when using blockdev-add/
> -blockdev) feels to me like the right thing to do.
> 
> But of course I'm aware that there probably isn't a clear right or wrong, and
> that I might be missing important details, so this needs to be discussed in
> advance before I go and implement the full thing instead of just small example
> patches.
> 
> So please let me know what you guys think about this plan.
> 
> Kevin Wolf (7):
>   block/qdev: Allow node name for drive properties
>   block: Add blk_by_dev()
>   qdev-monitor: Factor out find_device_state()
>   qdev-monitor: Add blk_by_qdev_id()
>   block: Accept device model name for blockdev-open/close-tray
>   block: Accept node-name for block-stream
>   block/qdev: Allow configuring WCE with qdev properties
> 
>  block/block-backend.c            | 19 +++++++++
>  blockdev.c                       | 92 ++++++++++++++++++++++++++++------------
>  hw/block/block.c                 | 16 +++++++
>  hw/block/nvme.c                  |  1 +
>  hw/block/virtio-blk.c            |  1 +
>  hw/core/qdev-properties-system.c | 18 +++++++-
>  hw/ide/qdev.c                    |  1 +
>  hw/scsi/scsi-disk.c              |  1 +
>  hw/usb/dev-storage.c             |  1 +
>  include/hw/block/block.h         |  5 ++-
>  include/sysemu/block-backend.h   |  2 +
>  qapi/block-core.json             | 16 ++++---
>  qdev-monitor.c                   | 34 +++++++++++++--
>  qmp-commands.hx                  | 14 +++---
>  14 files changed, 178 insertions(+), 43 deletions(-)
> 

1-4: Reviewed-by: John Snow <jsnow@redhat.com>
5: Looks good, pending discussion on the right thing to name "ID", but
the patch itself looks perfectly cromulent.
6: Causes only a minor regression in 030 due to different error class
names, but R-B otherwise.
7: No opinion. Looks sane mechanically but I don't know enough about
core block properties to have a meaningful opinion. "ACK."

For non-RFC, some new iotests would be good.

Thanks,
--js

Re: [Qemu-devel] [Qemu-block] [RFC PATCH 0/7] BlockBackends, nodes and guest devices

[Kevin Wolf]

Am 12.07.2016 um 02:13 hat John Snow geschrieben:
> No oxford comma in the subject? :)

It's already hard enough to keep the German comma rules straight and
avoid confusing the pre-reform rules with the post-reform ones. I don't
think I should bother with the comma rules of a language where even the
native speakers can't seem to agree. :-)

(But thanks, I wasn't aware that this disagreement even exists before
reading it up now.)

> On 06/23/2016 10:36 AM, Kevin Wolf wrote:
> 1-4: Reviewed-by: John Snow <jsnow@redhat.com>
> 5: Looks good, pending discussion on the right thing to name "ID", but
> the patch itself looks perfectly cromulent.
> 6: Causes only a minor regression in 030 due to different error class
> names, but R-B otherwise.
> 7: No opinion. Looks sane mechanically but I don't know enough about
> core block properties to have a meaningful opinion. "ACK."

Thanks!

> For non-RFC, some new iotests would be good.

Anything specific you have in mind to be tested?

The problem with everything related to devices is that it requires
running a qemu process and most likely a specific machine type. But I
guess we can just skip some tests if we don't have the right binary.

Kevin

Re: [Qemu-devel] [Qemu-block] [RFC PATCH 0/7] BlockBackends, nodes and guest devices

[John Snow]

On 07/12/2016 04:14 AM, Kevin Wolf wrote:
> Am 12.07.2016 um 02:13 hat John Snow geschrieben:
>> No oxford comma in the subject? :)
> 
> It's already hard enough to keep the German comma rules straight and
> avoid confusing the pre-reform rules with the post-reform ones. I don't
> think I should bother with the comma rules of a language where even the
> native speakers can't seem to agree. :-)
> 
> (But thanks, I wasn't aware that this disagreement even exists before
> reading it up now.)
> 

Only teasing, of course.

(I was searching for this thread via the patches tool and used the
oxford comma myself and had to figure out why it wasn't showing up. Not
a real critique!)

>> On 06/23/2016 10:36 AM, Kevin Wolf wrote:
>> 1-4: Reviewed-by: John Snow <jsnow@redhat.com>
>> 5: Looks good, pending discussion on the right thing to name "ID", but
>> the patch itself looks perfectly cromulent.
>> 6: Causes only a minor regression in 030 due to different error class
>> names, but R-B otherwise.
>> 7: No opinion. Looks sane mechanically but I don't know enough about
>> core block properties to have a meaningful opinion. "ACK."
> 
> Thanks!
> 
>> For non-RFC, some new iotests would be good.
> 
> Anything specific you have in mind to be tested?
> 

Just basic usage that demonstrates the new API for any converted
commands, and something that uses the new WCE options.

Would like to see the code in patch one tested at least lightly, to
prevent us from goofing up the assertions in the cleanup in the future.

> The problem with everything related to devices is that it requires
> running a qemu process and most likely a specific machine type. But I
> guess we can just skip some tests if we don't have the right binary.
> 
> Kevin
> 

Wishing more and more for python-based qtests. (ptests !?)

--js