block: Fix -blockdev for certain non-string scalars

Configuration flows through the block subsystem in a rather peculiar
way.  Configuration made with -drive enters it as QemuOpts.
Configuration made with -blockdev / blockdev-add enters it as QAPI
type BlockdevOptions.  The block subsystem uses QDict, QemuOpts and
QAPI types internally.  The precise flow is next to impossible to
explain (I tried for this commit message, but gave up after wasting
several hours).  What I can explain is a flaw in the BlockDriver
interface that leads to this bug:

    $ qemu-system-x86_64 -blockdev node-name=n1,driver=nfs,server.type=inet,server.host=localhost,path=/foo/bar,user=1234
    qemu-system-x86_64: -blockdev node-name=n1,driver=nfs,server.type=inet,server.host=localhost,path=/foo/bar,user=1234: Internal error: parameter user invalid

QMP blockdev-add is broken the same way.

Here's what happens.  The block layer passes configuration represented
as flat QDict (with dotted keys) to BlockDriver methods
.bdrv_file_open().  The QDict's members are typed according to the
QAPI schema.

nfs_file_open() converts it to QAPI type BlockdevOptionsNfs, with
qdict_crumple() and a qobject input visitor.

This visitor comes in two flavors.  The plain flavor requires scalars
to be typed according to the QAPI schema.  That's the case here.  The
keyval flavor requires string scalars.  That's not the case here.
nfs_file_open() uses the latter, and promptly falls apart for members
@user, @group, @tcp-syn-count, @readahead-size, @page-cache-size,
@debug.

Switching to the plain flavor would fix -blockdev, but break -drive,
because there the scalars arrive in nfs_file_open() as strings.

The proper fix would be to replace the QDict by QAPI type
BlockdevOptions in the BlockDriver interface.  Sadly, that's beyond my
reach right now.

Next best would be to fix the block layer to always pass correctly
typed QDicts to the BlockDriver methods.  Also beyond my reach.

What I can do is throw another hack onto the pile: have
nfs_file_open() convert all members to string, so use of the keyval
flavor actually works, by replacing qdict_crumple() by new function
qdict_crumple_for_keyval_qiv().

The pattern "pass result of qdict_crumple() to
qobject_input_visitor_new_keyval()" occurs several times more:

* qemu_rbd_open()

  Same issue as nfs_file_open(), but since BlockdevOptionsRbd has only
  string members, its only a latent bug.  Fix it anyway.

* parallels_co_create_opts(), qcow_co_create_opts(),
  qcow2_co_create_opts(), bdrv_qed_co_create_opts(),
  sd_co_create_opts(), vhdx_co_create_opts(), vpc_co_create_opts()

  These work, because they create the QDict with
  qemu_opts_to_qdict_filtered(), which creates only string scalars.
  The function sports a TODO comment asking for better typing; that's
  going to be fun.  Use qdict_crumple_for_keyval_qiv() to be safe.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>

diff --git a/block/nfs.c b/block/nfs.c
index 3170b059b3adb60d036cdf12abd5e973ac2e8092..6935b611cc51e5ddbd1033f153c1ee1edb5eb7a8 100644 (file)
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -561,7 +561,7 @@ static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QDict *options,
     const QDictEntry *e;
     Error *local_err = NULL;
 
-    crumpled = qdict_crumple(options, errp);
+    crumpled = qdict_crumple_for_keyval_qiv(options, errp);
     if (crumpled == NULL) {
         return NULL;
     }

diff --git a/block/parallels.c b/block/parallels.c
index c1d9643498028699001207d8f1fbd331d28e78f6..695899fc4bb6dc19ffc569b40b41afef02024421 100644 (file)
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -653,7 +653,7 @@ static int coroutine_fn parallels_co_create_opts(const char *filename,
     qdict_put_str(qdict, "driver", "parallels");
     qdict_put_str(qdict, "file", bs->node_name);
 
-    qobj = qdict_crumple(qdict, errp);
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
     qobject_unref(qdict);
     qdict = qobject_to(QDict, qobj);
     if (qdict == NULL) {

diff --git a/block/qcow.c b/block/qcow.c
index 8c08908fd88774565e29fe17eb36cebec850dc4b..860b058240c688ae89a95069254ae9fd40966ec2 100644 (file)
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -997,7 +997,7 @@ static int coroutine_fn qcow_co_create_opts(const char *filename,
     qdict_put_str(qdict, "driver", "qcow");
     qdict_put_str(qdict, "file", bs->node_name);
 
-    qobj = qdict_crumple(qdict, errp);
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
     qobject_unref(qdict);
     qdict = qobject_to(QDict, qobj);
     if (qdict == NULL) {

diff --git a/block/qcow2.c b/block/qcow2.c
index d2d955f984871c9d9509a774be25ee9623608665..0a27afa2ef82ca688ccc3c4d143a8654f2468d42 100644 (file)
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3152,7 +3152,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt
     qdict_put_str(qdict, "file", bs->node_name);
 
     /* Now get the QAPI type BlockdevCreateOptions */
-    qobj = qdict_crumple(qdict, errp);
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
     qobject_unref(qdict);
     qdict = qobject_to(QDict, qobj);
     if (qdict == NULL) {

diff --git a/block/qed.c b/block/qed.c
index 324a953cbcfa8176e9df80a1ad6105518153a457..88fa36d4099dd0c1fa3672f4b7dfb3eac098d597 100644 (file)
--- a/block/qed.c
+++ b/block/qed.c
@@ -763,7 +763,7 @@ static int coroutine_fn bdrv_qed_co_create_opts(const char *filename,
     qdict_put_str(qdict, "driver", "qed");
     qdict_put_str(qdict, "file", bs->node_name);
 
-    qobj = qdict_crumple(qdict, errp);
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
     qobject_unref(qdict);
     qdict = qobject_to(QDict, qobj);
     if (qdict == NULL) {

diff --git a/block/rbd.c b/block/rbd.c
index 9659c7361ff908b8feb92c0750e3443d457c9e98..09720e97c0ae76ff7de1d909708cb4c7d34041ad 100644 (file)
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -647,7 +647,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* Convert the remaining options into a QAPI object */
-    crumpled = qdict_crumple(options, errp);
+    crumpled = qdict_crumple_for_keyval_qiv(options, errp);
     if (crumpled == NULL) {
         r = -EINVAL;
         goto out;

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 2e1f0e6eca7bd7b5a768cde0f67bff52ea388758..a93f93d3604e8755631911c95d3b22ba2547d415 100644 (file)
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2217,7 +2217,7 @@ static int coroutine_fn sd_co_create_opts(const char *filename, QemuOpts *opts,
     }
 
     /* Get the QAPI object */
-    crumpled = qdict_crumple(qdict, errp);
+    crumpled = qdict_crumple_for_keyval_qiv(qdict, errp);
     if (crumpled == NULL) {
         ret = -EINVAL;
         goto fail;

diff --git a/block/vhdx.c b/block/vhdx.c
index 2e32e245146eb39a54b27859891789145ec58ea3..78b29d9fc723b6fd56fb652ed98837da1798af19 100644 (file)
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -2005,7 +2005,7 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename,
     qdict_put_str(qdict, "driver", "vhdx");
     qdict_put_str(qdict, "file", bs->node_name);
 
-    qobj = qdict_crumple(qdict, errp);
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
     qobject_unref(qdict);
     qdict = qobject_to(QDict, qobj);
     if (qdict == NULL) {

diff --git a/block/vpc.c b/block/vpc.c
index 41c8c980f19d15a5d2ff3e2c682720f839ec31f5..16178e5a9076806ca815d81078ae6751dbecc926 100644 (file)
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1119,7 +1119,7 @@ static int coroutine_fn vpc_co_create_opts(const char *filename,
     qdict_put_str(qdict, "driver", "vpc");
     qdict_put_str(qdict, "file", bs->node_name);
 
-    qobj = qdict_crumple(qdict, errp);
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
     qobject_unref(qdict);
     qdict = qobject_to(QDict, qobj);
     if (qdict == NULL) {

diff --git a/include/block/qdict.h b/include/block/qdict.h
index 71c037afba4170b9bac423e30de5371d3b0c257d..47d9638c3771ca82893d3196098d4786ef9db3a2 100644 (file)
--- a/include/block/qdict.h
+++ b/include/block/qdict.h
@@ -21,6 +21,7 @@ void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
 void qdict_array_split(QDict *src, QList **dst);
 int qdict_array_entries(QDict *src, const char *subqdict);
 QObject *qdict_crumple(const QDict *src, Error **errp);
+QObject *qdict_crumple_for_keyval_qiv(QDict *qdict, Error **errp);
 void qdict_flatten(QDict *qdict);
 
 typedef struct QDictRenames {

diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
index fb92010dc50d9a43c04a7e9f0fa37a644b5c3727..aba372c2eb3d051801bb2f75dbd2e33f7822d773 100644 (file)
--- a/qobject/block-qdict.c
+++ b/qobject/block-qdict.c
@@ -9,7 +9,10 @@
 
 #include "qemu/osdep.h"
 #include "block/qdict.h"
+#include "qapi/qmp/qbool.h"
 #include "qapi/qmp/qlist.h"
+#include "qapi/qmp/qnum.h"
+#include "qapi/qmp/qstring.h"
 #include "qemu/cutils.h"
 #include "qapi/error.h"
 
@@ -513,6 +516,60 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
     return NULL;
 }
 
+/**
+ * qdict_crumple_for_keyval_qiv:
+ * @src: the flat dictionary (only scalar values) to crumple
+ * @errp: location to store error
+ *
+ * Like qdict_crumple(), but additionally transforms scalar values so
+ * the result can be passed to qobject_input_visitor_new_keyval().
+ *
+ * The block subsystem uses this function to prepare its flat QDict
+ * with possibly confused scalar types for a visit.  It should not be
+ * used for anything else, and it should go away once the block
+ * subsystem has been cleaned up.
+ */
+QObject *qdict_crumple_for_keyval_qiv(QDict *src, Error **errp)
+{
+    QDict *tmp = NULL;
+    char *buf;
+    const char *s;
+    const QDictEntry *ent;
+    QObject *dst;
+
+    for (ent = qdict_first(src); ent; ent = qdict_next(src, ent)) {
+        buf = NULL;
+        switch (qobject_type(ent->value)) {
+        case QTYPE_QNULL:
+        case QTYPE_QSTRING:
+            continue;
+        case QTYPE_QNUM:
+            s = buf = qnum_to_string(qobject_to(QNum, ent->value));
+            break;
+        case QTYPE_QDICT:
+        case QTYPE_QLIST:
+            /* @src isn't flat; qdict_crumple() will fail */
+            continue;
+        case QTYPE_QBOOL:
+            s = qbool_get_bool(qobject_to(QBool, ent->value))
+                ? "on" : "off";
+            break;
+        default:
+            abort();
+        }
+
+        if (!tmp) {
+            tmp = qdict_clone_shallow(src);
+        }
+        qdict_put(tmp, ent->key, qstring_from_str(s));
+        g_free(buf);
+    }
+
+    dst = qdict_crumple(tmp ?: src, errp);
+    qobject_unref(tmp);
+    return dst;
+}
+
 /**
  * qdict_array_entries(): Returns the number of direct array entries if the
  * sub-QDict of src specified by the prefix in subqdict (or src itself for