Re: [Qemu-block] [RFC PATCH 06/19] block: Fix -blockdev for certain non-string scalars
Am 07.06.2018 um 08:25 hat Markus Armbruster geschrieben:
> 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 -blockdev
> node-name=n1,driver=nfs,server.type=inet,server.host=localhost,path=/foo/bar,user=1234:
> Internal error: parameter user invalid
> qemu-system-x86: -blockdev
> node-name=n1,driver=nfs,server.type=inet,server.host=localhost,path=/foo/bar,user=1234:
> Internal error: parameter user invalid
I don't think the error message was intended to be part of your command
line, and I also don't have a binary called qemu-system-x86. :-)
> 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
> +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";
This fits in a single line.
> +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;
> +}
Kevin
Re: [Qemu-block] [RFC PATCH 06/19] block: Fix -blockdev for certain non-string scalars
On 2018-06-07 08:25, Markus Armbruster wrote: > 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 -blockdev > node-name=n1,driver=nfs,server.type=inet,server.host=localhost,path=/foo/bar,user=1234: > Internal error: parameter user invalid > qemu-system-x86: -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. (I'll just chime in because why not.) > 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. Agreed. The way there probably is to (as always) unify how the block drivers convert their QDict to their own BlockdevOptions, and then pull that out into block.c, but let's see how far off that still is. > Next best would be to fix the block layer to always pass correctly > typed QDicts to the BlockDriver methods. Also beyond my reach. I tried and failed. (http://lists.nongnu.org/archive/html/qemu-block/2018-05/msg00061.html) But that will probably be an intermediate step before we get to BlockdevOptions. > 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(). I'm calling qdict_stringify_for_keyval() only from a single function, and it's immediately followed by a qdict_crumple(), so I suppose replacing those invocations in my series would be trivial. OTOH, just using qdict_stringify_for_keyval() in your qdict_crumple_for_keyval_qiv() function would be trivial as well. :-) But then again, it probably makes more sense to combine the two functions because I too assumed the QDict to be flattened before the function call for simplification. This assumption makes more sense if the function proceeds to call qdict_crumple(). > 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. Sounds good! > Signed-off-by: Markus Armbruster > --- > 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 +- > include/block/qdict.h | 1 + > qobject/block-qdict.c | 57 +++ > 11 files changed, 67 insertions(+), 9 deletions(-) [...] > @@ -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
[Qemu-block] [RFC PATCH 06/19] 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 -blockdev
node-name=n1,driver=nfs,server.type=inet,server.host=localhost,path=/foo/bar,user=1234:
Internal error: parameter user invalid
qemu-system-x86: -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
---
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 +-
include/block/qdict.h | 1 +
qobject/block-qdict.c | 57 +++
11 files changed, 67 insertions(+), 9 deletions(-)
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(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 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);
-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 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);
-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 3c5bd46663..a81ad4aef0 100644
--- 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
