From nobody Sat Apr 12 13:23:18 2025 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) client-ip=208.118.235.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=fail(p=none dis=none) header.from=redhat.com Return-Path: Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) by mx.zohomail.com with SMTPS id 1529073214947544.538457806164; Fri, 15 Jun 2018 07:33:34 -0700 (PDT) Received: from localhost ([::1]:47193 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fTpnC-0008Kz-3y for importer@patchew.org; Fri, 15 Jun 2018 10:33:34 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59227) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fTpbb-00069P-8a for qemu-devel@nongnu.org; Fri, 15 Jun 2018 10:21:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fTpbU-0003fH-9i for qemu-devel@nongnu.org; Fri, 15 Jun 2018 10:21:35 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:50214 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fTpbM-0003VY-Eg; Fri, 15 Jun 2018 10:21:20 -0400 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1788881A3245; Fri, 15 Jun 2018 14:21:20 +0000 (UTC) Received: from localhost.localdomain.com (ovpn-117-70.ams2.redhat.com [10.36.117.70]) by smtp.corp.redhat.com (Postfix) with ESMTP id 660A163A51; Fri, 15 Jun 2018 14:21:19 +0000 (UTC) From: Kevin Wolf To: qemu-block@nongnu.org Date: Fri, 15 Jun 2018 16:20:51 +0200 Message-Id: <20180615142108.27814-10-kwolf@redhat.com> In-Reply-To: <20180615142108.27814-1-kwolf@redhat.com> References: <20180615142108.27814-1-kwolf@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Fri, 15 Jun 2018 14:21:20 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Fri, 15 Jun 2018 14:21:20 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'kwolf@redhat.com' RCPT:'' X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 66.187.233.73 Subject: [Qemu-devel] [PULL 09/26] block: Fix -blockdev for certain non-string scalars X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kwolf@redhat.com, qemu-devel@nongnu.org Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: "Qemu-devel" X-ZohoMail: RSF_0 Z_629925259 SPT_0 Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" From: Markus Armbruster 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=3Dn1,driver=3Dnfs,server.type= =3Dinet,server.host=3Dlocalhost,path=3D/foo/bar,user=3D1234 qemu-system-x86_64: -blockdev node-name=3Dn1,driver=3Dnfs,server.type= =3Dinet,server.host=3Dlocalhost,path=3D/foo/bar,user=3D1234: 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 Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- include/block/qdict.h | 1 + block/nfs.c | 2 +- block/parallels.c | 2 +- block/qcow.c | 2 +- block/qcow2.c | 2 +- block/qed.c | 2 +- block/rbd.c | 2 +- block/sheepdog.c | 2 +- block/vhdx.c | 2 +- block/vpc.c | 2 +- qobject/block-qdict.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++= ++++ 11 files changed, 67 insertions(+), 9 deletions(-) diff --git a/include/block/qdict.h b/include/block/qdict.h index 71c037afba..47d9638c37 100644 --- a/include/block/qdict.h +++ b/include/block/qdict.h @@ -21,6 +21,7 @@ void qdict_extract_subqdict(QDict *src, QDict **dst, cons= t 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); =20 typedef struct QDictRenames { diff --git a/block/nfs.c b/block/nfs.c index 3170b059b3..6935b611cc 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -561,7 +561,7 @@ static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QD= ict *options, const QDictEntry *e; Error *local_err =3D NULL; =20 - crumpled =3D qdict_crumple(options, errp); + crumpled =3D qdict_crumple_for_keyval_qiv(options, errp); if (crumpled =3D=3D NULL) { return NULL; } diff --git a/block/parallels.c b/block/parallels.c index c1d9643498..695899fc4b 100644 --- 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); =20 - qobj =3D qdict_crumple(qdict, errp); + qobj =3D qdict_crumple_for_keyval_qiv(qdict, errp); qobject_unref(qdict); qdict =3D qobject_to(QDict, qobj); if (qdict =3D=3D NULL) { diff --git a/block/qcow.c b/block/qcow.c index 8c08908fd8..860b058240 100644 --- 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); =20 - qobj =3D qdict_crumple(qdict, errp); + qobj =3D qdict_crumple_for_keyval_qiv(qdict, errp); qobject_unref(qdict); qdict =3D qobject_to(QDict, qobj); if (qdict =3D=3D NULL) { diff --git a/block/qcow2.c b/block/qcow2.c index d2d955f984..0a27afa2ef 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3152,7 +3152,7 @@ static int coroutine_fn qcow2_co_create_opts(const ch= ar *filename, QemuOpts *opt qdict_put_str(qdict, "file", bs->node_name); =20 /* Now get the QAPI type BlockdevCreateOptions */ - qobj =3D qdict_crumple(qdict, errp); + qobj =3D qdict_crumple_for_keyval_qiv(qdict, errp); qobject_unref(qdict); qdict =3D qobject_to(QDict, qobj); if (qdict =3D=3D NULL) { diff --git a/block/qed.c b/block/qed.c index 324a953cbc..88fa36d409 100644 --- a/block/qed.c +++ b/block/qed.c @@ -763,7 +763,7 @@ static int coroutine_fn bdrv_qed_co_create_opts(const c= har *filename, qdict_put_str(qdict, "driver", "qed"); qdict_put_str(qdict, "file", bs->node_name); =20 - qobj =3D qdict_crumple(qdict, errp); + qobj =3D qdict_crumple_for_keyval_qiv(qdict, errp); qobject_unref(qdict); qdict =3D qobject_to(QDict, qobj); if (qdict =3D=3D NULL) { diff --git a/block/rbd.c b/block/rbd.c index 9659c7361f..09720e97c0 100644 --- a/block/rbd.c +++ b/block/rbd.c @@ -647,7 +647,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *o= ptions, int flags, } =20 /* Convert the remaining options into a QAPI object */ - crumpled =3D qdict_crumple(options, errp); + crumpled =3D qdict_crumple_for_keyval_qiv(options, errp); if (crumpled =3D=3D NULL) { r =3D -EINVAL; goto out; diff --git a/block/sheepdog.c b/block/sheepdog.c index 2e1f0e6eca..a93f93d360 100644 --- 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, } =20 /* Get the QAPI object */ - crumpled =3D qdict_crumple(qdict, errp); + crumpled =3D qdict_crumple_for_keyval_qiv(qdict, errp); if (crumpled =3D=3D NULL) { ret =3D -EINVAL; goto fail; diff --git a/block/vhdx.c b/block/vhdx.c index 2e32e24514..78b29d9fc7 100644 --- a/block/vhdx.c +++ b/block/vhdx.c @@ -2005,7 +2005,7 @@ static int coroutine_fn vhdx_co_create_opts(const cha= r *filename, qdict_put_str(qdict, "driver", "vhdx"); qdict_put_str(qdict, "file", bs->node_name); =20 - qobj =3D qdict_crumple(qdict, errp); + qobj =3D qdict_crumple_for_keyval_qiv(qdict, errp); qobject_unref(qdict); qdict =3D qobject_to(QDict, qobj); if (qdict =3D=3D NULL) { diff --git a/block/vpc.c b/block/vpc.c index 41c8c980f1..16178e5a90 100644 --- 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); =20 - qobj =3D qdict_crumple(qdict, errp); + qobj =3D qdict_crumple_for_keyval_qiv(qdict, errp); qobject_unref(qdict); qdict =3D qobject_to(QDict, qobj); if (qdict =3D=3D NULL) { diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c index fb92010dc5..aba372c2eb 100644 --- a/qobject/block-qdict.c +++ b/qobject/block-qdict.c @@ -9,7 +9,10 @@ =20 #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" =20 @@ -514,6 +517,60 @@ QObject *qdict_crumple(const QDict *src, Error **errp) } =20 /** + * 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 =3D NULL; + char *buf; + const char *s; + const QDictEntry *ent; + QObject *dst; + + for (ent =3D qdict_first(src); ent; ent =3D qdict_next(src, ent)) { + buf =3D NULL; + switch (qobject_type(ent->value)) { + case QTYPE_QNULL: + case QTYPE_QSTRING: + continue; + case QTYPE_QNUM: + s =3D buf =3D 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 =3D qbool_get_bool(qobject_to(QBool, ent->value)) + ? "on" : "off"; + break; + default: + abort(); + } + + if (!tmp) { + tmp =3D qdict_clone_shallow(src); + } + qdict_put(tmp, ent->key, qstring_from_str(s)); + g_free(buf); + } + + dst =3D 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 * prefix =3D=3D "") is valid as an array, i.e. the length of the created = list if --=20 2.13.6