26ba25
From 3a92d2770a0909c1ab425b4a5d24014e7fea2297 Mon Sep 17 00:00:00 2001
26ba25
From: Markus Armbruster <armbru@redhat.com>
26ba25
Date: Mon, 18 Jun 2018 08:43:17 +0200
26ba25
Subject: [PATCH 019/268] block: Fix -blockdev for certain non-string scalars
26ba25
26ba25
RH-Author: Markus Armbruster <armbru@redhat.com>
26ba25
Message-id: <20180618084330.30009-11-armbru@redhat.com>
26ba25
Patchwork-id: 80743
26ba25
O-Subject: [RHEL-7.6 qemu-kvm-rhev PATCH 10/23] block: Fix -blockdev for certain non-string scalars
26ba25
Bugzilla: 1557995
26ba25
RH-Acked-by: Max Reitz <mreitz@redhat.com>
26ba25
RH-Acked-by: Jeffrey Cody <jcody@redhat.com>
26ba25
RH-Acked-by: Kevin Wolf <kwolf@redhat.com>
26ba25
26ba25
Configuration flows through the block subsystem in a rather peculiar
26ba25
way.  Configuration made with -drive enters it as QemuOpts.
26ba25
Configuration made with -blockdev / blockdev-add enters it as QAPI
26ba25
type BlockdevOptions.  The block subsystem uses QDict, QemuOpts and
26ba25
QAPI types internally.  The precise flow is next to impossible to
26ba25
explain (I tried for this commit message, but gave up after wasting
26ba25
several hours).  What I can explain is a flaw in the BlockDriver
26ba25
interface that leads to this bug:
26ba25
26ba25
    $ qemu-system-x86_64 -blockdev node-name=n1,driver=nfs,server.type=inet,server.host=localhost,path=/foo/bar,user=1234
26ba25
    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
26ba25
26ba25
QMP blockdev-add is broken the same way.
26ba25
26ba25
Here's what happens.  The block layer passes configuration represented
26ba25
as flat QDict (with dotted keys) to BlockDriver methods
26ba25
.bdrv_file_open().  The QDict's members are typed according to the
26ba25
QAPI schema.
26ba25
26ba25
nfs_file_open() converts it to QAPI type BlockdevOptionsNfs, with
26ba25
qdict_crumple() and a qobject input visitor.
26ba25
26ba25
This visitor comes in two flavors.  The plain flavor requires scalars
26ba25
to be typed according to the QAPI schema.  That's the case here.  The
26ba25
keyval flavor requires string scalars.  That's not the case here.
26ba25
nfs_file_open() uses the latter, and promptly falls apart for members
26ba25
@user, @group, @tcp-syn-count, @readahead-size, @page-cache-size,
26ba25
@debug.
26ba25
26ba25
Switching to the plain flavor would fix -blockdev, but break -drive,
26ba25
because there the scalars arrive in nfs_file_open() as strings.
26ba25
26ba25
The proper fix would be to replace the QDict by QAPI type
26ba25
BlockdevOptions in the BlockDriver interface.  Sadly, that's beyond my
26ba25
reach right now.
26ba25
26ba25
Next best would be to fix the block layer to always pass correctly
26ba25
typed QDicts to the BlockDriver methods.  Also beyond my reach.
26ba25
26ba25
What I can do is throw another hack onto the pile: have
26ba25
nfs_file_open() convert all members to string, so use of the keyval
26ba25
flavor actually works, by replacing qdict_crumple() by new function
26ba25
qdict_crumple_for_keyval_qiv().
26ba25
26ba25
The pattern "pass result of qdict_crumple() to
26ba25
qobject_input_visitor_new_keyval()" occurs several times more:
26ba25
26ba25
* qemu_rbd_open()
26ba25
26ba25
  Same issue as nfs_file_open(), but since BlockdevOptionsRbd has only
26ba25
  string members, its only a latent bug.  Fix it anyway.
26ba25
26ba25
* parallels_co_create_opts(), qcow_co_create_opts(),
26ba25
  qcow2_co_create_opts(), bdrv_qed_co_create_opts(),
26ba25
  sd_co_create_opts(), vhdx_co_create_opts(), vpc_co_create_opts()
26ba25
26ba25
  These work, because they create the QDict with
26ba25
  qemu_opts_to_qdict_filtered(), which creates only string scalars.
26ba25
  The function sports a TODO comment asking for better typing; that's
26ba25
  going to be fun.  Use qdict_crumple_for_keyval_qiv() to be safe.
26ba25
26ba25
Signed-off-by: Markus Armbruster <armbru@redhat.com>
26ba25
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
26ba25
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
26ba25
(cherry picked from commit e5af0da1dcbfb1a4694150f9954554fb6df88819)
26ba25
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
26ba25
---
26ba25
 block/nfs.c           |  2 +-
26ba25
 block/parallels.c     |  2 +-
26ba25
 block/qcow.c          |  2 +-
26ba25
 block/qcow2.c         |  2 +-
26ba25
 block/qed.c           |  2 +-
26ba25
 block/rbd.c           |  2 +-
26ba25
 block/sheepdog.c      |  2 +-
26ba25
 block/vhdx.c          |  2 +-
26ba25
 block/vpc.c           |  2 +-
26ba25
 include/block/qdict.h |  1 +
26ba25
 qobject/block-qdict.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++
26ba25
 11 files changed, 67 insertions(+), 9 deletions(-)
26ba25
26ba25
diff --git a/block/nfs.c b/block/nfs.c
26ba25
index 5159ef0..4090d28 100644
26ba25
--- a/block/nfs.c
26ba25
+++ b/block/nfs.c
26ba25
@@ -560,7 +560,7 @@ static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QDict *options,
26ba25
     Visitor *v;
26ba25
     Error *local_err = NULL;
26ba25
 
26ba25
-    crumpled = qdict_crumple(options, errp);
26ba25
+    crumpled = qdict_crumple_for_keyval_qiv(options, errp);
26ba25
     if (crumpled == NULL) {
26ba25
         return NULL;
26ba25
     }
26ba25
diff --git a/block/parallels.c b/block/parallels.c
26ba25
index 0ee1f6a..aa58955 100644
26ba25
--- a/block/parallels.c
26ba25
+++ b/block/parallels.c
26ba25
@@ -651,7 +651,7 @@ static int coroutine_fn parallels_co_create_opts(const char *filename,
26ba25
     qdict_put_str(qdict, "driver", "parallels");
26ba25
     qdict_put_str(qdict, "file", bs->node_name);
26ba25
 
26ba25
-    qobj = qdict_crumple(qdict, errp);
26ba25
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
26ba25
     qobject_unref(qdict);
26ba25
     qdict = qobject_to(QDict, qobj);
26ba25
     if (qdict == NULL) {
26ba25
diff --git a/block/qcow.c b/block/qcow.c
26ba25
index fb821ad..14b7296 100644
26ba25
--- a/block/qcow.c
26ba25
+++ b/block/qcow.c
26ba25
@@ -995,7 +995,7 @@ static int coroutine_fn qcow_co_create_opts(const char *filename,
26ba25
     qdict_put_str(qdict, "driver", "qcow");
26ba25
     qdict_put_str(qdict, "file", bs->node_name);
26ba25
 
26ba25
-    qobj = qdict_crumple(qdict, errp);
26ba25
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
26ba25
     qobject_unref(qdict);
26ba25
     qdict = qobject_to(QDict, qobj);
26ba25
     if (qdict == NULL) {
26ba25
diff --git a/block/qcow2.c b/block/qcow2.c
26ba25
index fa9f557..fa06b41 100644
26ba25
--- a/block/qcow2.c
26ba25
+++ b/block/qcow2.c
26ba25
@@ -3139,7 +3139,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt
26ba25
     qdict_put_str(qdict, "file", bs->node_name);
26ba25
 
26ba25
     /* Now get the QAPI type BlockdevCreateOptions */
26ba25
-    qobj = qdict_crumple(qdict, errp);
26ba25
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
26ba25
     qobject_unref(qdict);
26ba25
     qdict = qobject_to(QDict, qobj);
26ba25
     if (qdict == NULL) {
26ba25
diff --git a/block/qed.c b/block/qed.c
26ba25
index 9a8997a..d8810f5 100644
26ba25
--- a/block/qed.c
26ba25
+++ b/block/qed.c
26ba25
@@ -763,7 +763,7 @@ static int coroutine_fn bdrv_qed_co_create_opts(const char *filename,
26ba25
     qdict_put_str(qdict, "driver", "qed");
26ba25
     qdict_put_str(qdict, "file", bs->node_name);
26ba25
 
26ba25
-    qobj = qdict_crumple(qdict, errp);
26ba25
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
26ba25
     qobject_unref(qdict);
26ba25
     qdict = qobject_to(QDict, qobj);
26ba25
     if (qdict == NULL) {
26ba25
diff --git a/block/rbd.c b/block/rbd.c
26ba25
index e695cf2..0b5455f 100644
26ba25
--- a/block/rbd.c
26ba25
+++ b/block/rbd.c
26ba25
@@ -640,7 +640,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
26ba25
     }
26ba25
 
26ba25
     /* Convert the remaining options into a QAPI object */
26ba25
-    crumpled = qdict_crumple(options, errp);
26ba25
+    crumpled = qdict_crumple_for_keyval_qiv(options, errp);
26ba25
     if (crumpled == NULL) {
26ba25
         r = -EINVAL;
26ba25
         goto out;
26ba25
diff --git a/block/sheepdog.c b/block/sheepdog.c
26ba25
index fd3876f..821a3c4 100644
26ba25
--- a/block/sheepdog.c
26ba25
+++ b/block/sheepdog.c
26ba25
@@ -2218,7 +2218,7 @@ static int coroutine_fn sd_co_create_opts(const char *filename, QemuOpts *opts,
26ba25
     }
26ba25
 
26ba25
     /* Get the QAPI object */
26ba25
-    crumpled = qdict_crumple(qdict, errp);
26ba25
+    crumpled = qdict_crumple_for_keyval_qiv(qdict, errp);
26ba25
     if (crumpled == NULL) {
26ba25
         ret = -EINVAL;
26ba25
         goto fail;
26ba25
diff --git a/block/vhdx.c b/block/vhdx.c
26ba25
index 26c05aa..32939c4 100644
26ba25
--- a/block/vhdx.c
26ba25
+++ b/block/vhdx.c
26ba25
@@ -2003,7 +2003,7 @@ static int coroutine_fn vhdx_co_create_opts(const char *filename,
26ba25
     qdict_put_str(qdict, "driver", "vhdx");
26ba25
     qdict_put_str(qdict, "file", bs->node_name);
26ba25
 
26ba25
-    qobj = qdict_crumple(qdict, errp);
26ba25
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
26ba25
     qobject_unref(qdict);
26ba25
     qdict = qobject_to(QDict, qobj);
26ba25
     if (qdict == NULL) {
26ba25
diff --git a/block/vpc.c b/block/vpc.c
26ba25
index 41c8c98..16178e5 100644
26ba25
--- a/block/vpc.c
26ba25
+++ b/block/vpc.c
26ba25
@@ -1119,7 +1119,7 @@ static int coroutine_fn vpc_co_create_opts(const char *filename,
26ba25
     qdict_put_str(qdict, "driver", "vpc");
26ba25
     qdict_put_str(qdict, "file", bs->node_name);
26ba25
 
26ba25
-    qobj = qdict_crumple(qdict, errp);
26ba25
+    qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
26ba25
     qobject_unref(qdict);
26ba25
     qdict = qobject_to(QDict, qobj);
26ba25
     if (qdict == NULL) {
26ba25
diff --git a/include/block/qdict.h b/include/block/qdict.h
26ba25
index 71c037a..47d9638 100644
26ba25
--- a/include/block/qdict.h
26ba25
+++ b/include/block/qdict.h
26ba25
@@ -21,6 +21,7 @@ void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
26ba25
 void qdict_array_split(QDict *src, QList **dst);
26ba25
 int qdict_array_entries(QDict *src, const char *subqdict);
26ba25
 QObject *qdict_crumple(const QDict *src, Error **errp);
26ba25
+QObject *qdict_crumple_for_keyval_qiv(QDict *qdict, Error **errp);
26ba25
 void qdict_flatten(QDict *qdict);
26ba25
 
26ba25
 typedef struct QDictRenames {
26ba25
diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
26ba25
index fb92010..aba372c 100644
26ba25
--- a/qobject/block-qdict.c
26ba25
+++ b/qobject/block-qdict.c
26ba25
@@ -9,7 +9,10 @@
26ba25
 
26ba25
 #include "qemu/osdep.h"
26ba25
 #include "block/qdict.h"
26ba25
+#include "qapi/qmp/qbool.h"
26ba25
 #include "qapi/qmp/qlist.h"
26ba25
+#include "qapi/qmp/qnum.h"
26ba25
+#include "qapi/qmp/qstring.h"
26ba25
 #include "qemu/cutils.h"
26ba25
 #include "qapi/error.h"
26ba25
 
26ba25
@@ -514,6 +517,60 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
26ba25
 }
26ba25
 
26ba25
 /**
26ba25
+ * qdict_crumple_for_keyval_qiv:
26ba25
+ * @src: the flat dictionary (only scalar values) to crumple
26ba25
+ * @errp: location to store error
26ba25
+ *
26ba25
+ * Like qdict_crumple(), but additionally transforms scalar values so
26ba25
+ * the result can be passed to qobject_input_visitor_new_keyval().
26ba25
+ *
26ba25
+ * The block subsystem uses this function to prepare its flat QDict
26ba25
+ * with possibly confused scalar types for a visit.  It should not be
26ba25
+ * used for anything else, and it should go away once the block
26ba25
+ * subsystem has been cleaned up.
26ba25
+ */
26ba25
+QObject *qdict_crumple_for_keyval_qiv(QDict *src, Error **errp)
26ba25
+{
26ba25
+    QDict *tmp = NULL;
26ba25
+    char *buf;
26ba25
+    const char *s;
26ba25
+    const QDictEntry *ent;
26ba25
+    QObject *dst;
26ba25
+
26ba25
+    for (ent = qdict_first(src); ent; ent = qdict_next(src, ent)) {
26ba25
+        buf = NULL;
26ba25
+        switch (qobject_type(ent->value)) {
26ba25
+        case QTYPE_QNULL:
26ba25
+        case QTYPE_QSTRING:
26ba25
+            continue;
26ba25
+        case QTYPE_QNUM:
26ba25
+            s = buf = qnum_to_string(qobject_to(QNum, ent->value));
26ba25
+            break;
26ba25
+        case QTYPE_QDICT:
26ba25
+        case QTYPE_QLIST:
26ba25
+            /* @src isn't flat; qdict_crumple() will fail */
26ba25
+            continue;
26ba25
+        case QTYPE_QBOOL:
26ba25
+            s = qbool_get_bool(qobject_to(QBool, ent->value))
26ba25
+                ? "on" : "off";
26ba25
+            break;
26ba25
+        default:
26ba25
+            abort();
26ba25
+        }
26ba25
+
26ba25
+        if (!tmp) {
26ba25
+            tmp = qdict_clone_shallow(src);
26ba25
+        }
26ba25
+        qdict_put(tmp, ent->key, qstring_from_str(s));
26ba25
+        g_free(buf);
26ba25
+    }
26ba25
+
26ba25
+    dst = qdict_crumple(tmp ?: src, errp);
26ba25
+    qobject_unref(tmp);
26ba25
+    return dst;
26ba25
+}
26ba25
+
26ba25
+/**
26ba25
  * qdict_array_entries(): Returns the number of direct array entries if the
26ba25
  * sub-QDict of src specified by the prefix in subqdict (or src itself for
26ba25
  * prefix == "") is valid as an array, i.e. the length of the created list if
26ba25
-- 
26ba25
1.8.3.1
26ba25