[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 01/37] blockdev: Allow creation of BDS trees

From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v2 01/37] blockdev: Allow creation of BDS trees without BB
Date: Wed, 04 Mar 2015 16:44:48 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0

On 2015-03-04 at 08:39, Kevin Wolf wrote:
Am 09.02.2015 um 18:11 hat Max Reitz geschrieben:
If the "id" field is missing from the options given to blockdev-add,
just omit the BlockBackend and create the BlockDriverState tree alone.

However, if "id" is missing, "node-name" must be specified; otherwise,
the BDS tree would no longer be accessible.

Signed-off-by: Max Reitz <address@hidden>
  blockdev.c                 | 44 +++++++++++++++++++++++++++++++-------------
  qapi/block-core.json       | 13 +++++++++----
  tests/qemu-iotests/087     |  2 +-
  tests/qemu-iotests/087.out |  4 ++--
  4 files changed, 43 insertions(+), 20 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 6eedcf5..6d67c80 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2822,17 +2822,12 @@ out:
  void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
      QmpOutputVisitor *ov = qmp_output_visitor_new();
-    BlockBackend *blk;
+    BlockDriverState *bs;
+    BlockBackend *blk = NULL;
      QObject *obj;
      QDict *qdict;
      Error *local_err = NULL;
- /* Require an ID in the top level */
-    if (!options->has_id) {
-        error_setg(errp, "Block device needs an ID");
-        goto fail;
-    }
      /* TODO Sort it out in raw-posix and drive_new(): Reject aio=native with
       * cache.direct=false instead of silently switching to aio=threads, except
       * when called from drive_new().
@@ -2860,14 +2855,37 @@ void qmp_blockdev_add(BlockdevOptions *options, Error 
qdict_flatten(qdict); - blk = blockdev_init(NULL, qdict, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        goto fail;
+    if (options->has_id) {
+        blk = blockdev_init(NULL, qdict, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            goto fail;
+        }
+        bs = blk_bs(blk);
+    } else {
+        int ret;
+        if (!qdict_get_try_str(qdict, "node-name")) {
+            error_setg(errp, "'id' and/or 'node-name' need to be specified for 
+                       "the root node");
+            goto fail;
+        }
+        bs = NULL;
+        ret = bdrv_open(&bs, NULL, NULL, qdict, BDRV_O_RDWR | BDRV_O_CACHE_WB,
+                        NULL, errp);
Now all the qdict entries that aren't parsed by bdrv_open() but
converted into flags by blockdev_init() are broken if you don't give an
id. This includes read-only, discard, the cache options - in other
words, enough to make this "support" completely useless.

I guess I need to dig out my cache mode series and get it ready to be
merged. Once this is in, the parsing of the other necessary options
should be easy to move into bdrv_open().

Unless you have a good reason why we should merge this patch in its
hardly functional state now, I would consider those conversions a
dependency of this one.

Because I will not send out another version of this series for the next two weeks, I'll just give an overview of where I'm currently at (for each of the patches you replied to):

We need this patch before "block: Make bdrv_is_inserted() recursive", because that patch makes the problems apparent which are fixed in my bdrv_close_all() series (071 fails, because flushing the qcow2 metadata suddenly no longer results in an error; this is because bdrv_is_inserted() now returns false because the file BDS under qcow2 has been closed (because it has a BB), so it's no longer inserted). Therefore, we need patch 2 before said patch to work around the problem (until it's fixed by the respective series, which depends on this one, however).

I could probably split up patch 22 and pull the first part up front, along with patch 23, but this would require work, which I don't deem necessary. As I said before, in my opinion this patch does not break any existing valid usage of blockdev-add, it's just that there are some use cases we want to have after this patch that do not work yet. But they will be added in a later patch in this series, so I don't find this an issue.

Therefore, for this patch, I'll add a note into the commit message stating that there will be a follow-up patch allowing options that are not parsed by bdrv_open() (like caching) to be given.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]