diff mbox

[01/50] blockdev: Allow creation of BDS trees without BB

Message ID 1422288204-29271-2-git-send-email-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz Jan. 26, 2015, 4:02 p.m. UTC 
If the "id" field is missing from the options given to blockdev-add,
just omit the BlockBackend and create the BlockDriverState tree alone.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c                 | 38 +++++++++++++++++++++++++-------------
 tests/qemu-iotests/087     | 20 --------------------
 tests/qemu-iotests/087.out | 12 ------------
 3 files changed, 25 insertions(+), 45 deletions(-)

Comments

Eric Blake Jan. 27, 2015, 6:22 p.m. UTC | #1 
On 01/26/2015 09:02 AM, Max Reitz wrote:
> If the "id" field is missing from the options given to blockdev-add,
> just omit the BlockBackend and create the BlockDriverState tree alone.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c                 | 38 +++++++++++++++++++++++++-------------
>  tests/qemu-iotests/087     | 20 --------------------
>  tests/qemu-iotests/087.out | 12 ------------
>  3 files changed, 25 insertions(+), 45 deletions(-)

I'd feel more comfortable if this patch also touched
qapi/block-core.json to mention the change in policy on
BlockdevOptionsBase.  Also, should 'node-name' be mandatory when
omitting 'id'?

But the idea makes sense.
Max Reitz Jan. 27, 2015, 6:26 p.m. UTC | #2 
On 2015-01-27 at 13:22, Eric Blake wrote:
> On 01/26/2015 09:02 AM, Max Reitz wrote:
>> If the "id" field is missing from the options given to blockdev-add,
>> just omit the BlockBackend and create the BlockDriverState tree alone.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   blockdev.c                 | 38 +++++++++++++++++++++++++-------------
>>   tests/qemu-iotests/087     | 20 --------------------
>>   tests/qemu-iotests/087.out | 12 ------------
>>   3 files changed, 25 insertions(+), 45 deletions(-)
> I'd feel more comfortable if this patch also touched
> qapi/block-core.json to mention the change in policy on
> BlockdevOptionsBase.

Will do.

> Also, should 'node-name' be mandatory when
> omitting 'id'?

Seems good to me. Not specifying a node-name doesn't make a whole lot of 
sense, except for when you really badly want qemu to create useless 
structures in memory.

Max
diff mbox

Patch

diff --git a/blockdev.c b/blockdev.c
index 7573746..81fa7bd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2844,17 +2844,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().
@@ -2882,14 +2877,31 @@  void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 
     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;
+
+        bs = NULL;
+        ret = bdrv_open(&bs, NULL, NULL, qdict, BDRV_O_RDWR | BDRV_O_CACHE_WB,
+                        NULL, errp);
+        if (ret < 0) {
+            goto fail;
+        }
     }
 
-    if (bdrv_key_required(blk_bs(blk))) {
-        blk_unref(blk);
+    if (bs && bdrv_key_required(bs)) {
+        if (blk) {
+            blk_unref(blk);
+        } else {
+            bdrv_unref(bs);
+        }
         error_setg(errp, "blockdev-add doesn't support encrypted devices");
         goto fail;
     }
diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index 8694749..4b2eddc 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -54,26 +54,6 @@  size=128M
 _make_test_img $size
 
 echo
-echo === Missing ID ===
-echo
-
-run_qemu <<EOF
-{ "execute": "qmp_capabilities" }
-{ "execute": "blockdev-add",
-  "arguments": {
-      "options": {
-        "driver": "$IMGFMT",
-        "file": {
-            "driver": "file",
-            "filename": "$TEST_IMG"
-        }
-      }
-    }
-  }
-{ "execute": "quit" }
-EOF
-
-echo
 echo === Duplicate ID ===
 echo
 
diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
index 0ba2e43..681ef93 100644
--- a/tests/qemu-iotests/087.out
+++ b/tests/qemu-iotests/087.out
@@ -1,18 +1,6 @@ 
 QA output created by 087
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 
-=== Missing ID ===
-
-Testing:
-QMP_VERSION
-{"return": {}}
-{"error": {"class": "GenericError", "desc": "Block device needs an ID"}}
-{"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}}
-
-
 === Duplicate ID ===
 
 Testing: