diff mbox

[PATCHv3,2/6] block: introduce bdrv_runtime_opts

Message ID 1414256153-10148-3-git-send-email-pl@kamp.de
State New
Headers show

Commit Message

Peter Lieven Oct. 25, 2014, 4:55 p.m. UTC 
This patch (orginally by Kevin) adds a bdrv_runtime_opts QemuOptsList.
The list will absorb all options that belong to the BDS (and not the
BlockBackend) and will be parsed and handled in bdrv_open_common.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Peter Lieven <pl@kamp.de>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c |   38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

Comments

Stefan Hajnoczi Oct. 28, 2014, 11:14 a.m. UTC | #1 
On Sat, Oct 25, 2014 at 06:55:49PM +0200, Peter Lieven wrote:
> This patch (orginally by Kevin) adds a bdrv_runtime_opts QemuOptsList.
> The list will absorb all options that belong to the BDS (and not the
> BlockBackend) and will be parsed and handled in bdrv_open_common.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c |   38 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 5 deletions(-)

Is this purely because the QemuOptsList API is more convenient than
qdict?

I don't see a deeper reason why we must use QemuOptsList here.

The code is fine, however:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Max Reitz Oct. 28, 2014, 11:41 a.m. UTC | #2 
On 2014-10-28 at 12:14, Stefan Hajnoczi wrote:
> On Sat, Oct 25, 2014 at 06:55:49PM +0200, Peter Lieven wrote:
>> This patch (orginally by Kevin) adds a bdrv_runtime_opts QemuOptsList.
>> The list will absorb all options that belong to the BDS (and not the
>> BlockBackend) and will be parsed and handled in bdrv_open_common.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c |   38 +++++++++++++++++++++++++++++++++-----
>>   1 file changed, 33 insertions(+), 5 deletions(-)
> Is this purely because the QemuOptsList API is more convenient than
> qdict?
>
> I don't see a deeper reason why we must use QemuOptsList here.

If I remember correctly, it was because the command line options are 
only strings in the QDict. To be able to use types other than strings, 
it has to be converted to QemuOpts.

Max
diff mbox

Patch

diff --git a/block.c b/block.c
index d13b87e..f05ea0c 100644
--- a/block.c
+++ b/block.c
@@ -27,6 +27,7 @@ 
 #include "block/block_int.h"
 #include "block/blockjob.h"
 #include "qemu/module.h"
+#include "qapi/qmp/qbool.h"
 #include "qapi/qmp/qjson.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/sysemu.h"
@@ -875,6 +876,19 @@  static void bdrv_assign_node_name(BlockDriverState *bs,
     QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
 }
 
+static QemuOptsList bdrv_runtime_opts = {
+    .name = "bdrv_common",
+    .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
+    .desc = {
+        {
+            .name = "node-name",
+            .type = QEMU_OPT_STRING,
+            .help = "Node name of the block device node",
+        },
+        { /* end of list */ }
+    },
+};
+
 /*
  * Common part for opening disk images and files
  *
@@ -886,6 +900,7 @@  static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     int ret, open_flags;
     const char *filename;
     const char *node_name = NULL;
+    QemuOpts *opts;
     Error *local_err = NULL;
 
     assert(drv != NULL);
@@ -906,19 +921,28 @@  static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
 
     trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
 
-    node_name = qdict_get_try_str(options, "node-name");
+    opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto fail_opts;
+    }
+
+    node_name = qemu_opt_get(opts, "node-name");
     bdrv_assign_node_name(bs, node_name, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        return -EINVAL;
+        ret = -EINVAL;
+        goto fail_opts;
     }
-    qdict_del(options, "node-name");
 
     /* bdrv_open() with directly using a protocol as drv. This layer is already
      * opened, so assign it to bs (while file becomes a closed BlockDriverState)
      * and return immediately. */
     if (file != NULL && drv->bdrv_file_open) {
         bdrv_swap(file, bs);
+        qemu_opts_del(opts);
         return 0;
     }
 
@@ -936,7 +960,8 @@  static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
                         ? "Driver '%s' can only be used for read-only devices"
                         : "Driver '%s' is not whitelisted",
                    drv->format_name);
-        return -ENOTSUP;
+        ret = -ENOTSUP;
+        goto fail_opts;
     }
 
     assert(bs->copy_on_read == 0); /* bdrv_new() and bdrv_close() make it so */
@@ -945,7 +970,8 @@  static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
             bdrv_enable_copy_on_read(bs);
         } else {
             error_setg(errp, "Can't use copy-on-read on read-only device");
-            return -EINVAL;
+            ret = -EINVAL;
+            goto fail_opts;
         }
     }
 
@@ -1010,6 +1036,8 @@  free_and_fail:
     g_free(bs->opaque);
     bs->opaque = NULL;
     bs->drv = NULL;
+fail_opts:
+    qemu_opts_del(opts);
     return ret;
 }