[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH 06/19] block: Fix -blockdev for certain non-
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [RFC PATCH 06/19] block: Fix -blockdev for certain non-string scalars |
Date: |
Tue, 12 Jun 2018 15:38:34 +0200 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
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 <address@hidden>
> +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
- [Qemu-devel] [RFC PATCH 00/19] block: Configuration fixes and rbd authentication, Markus Armbruster, 2018/06/07
- [Qemu-devel] [RFC PATCH 11/19] block-qdict: Simplify qdict_flatten_qdict(), Markus Armbruster, 2018/06/07
- [Qemu-devel] [RFC PATCH 07/19] block: Fix -drive for certain non-string scalars, Markus Armbruster, 2018/06/07
- [Qemu-devel] [RFC PATCH 14/19] block-qdict: Simplify qdict_is_list() some, Markus Armbruster, 2018/06/07
- [Qemu-devel] [RFC PATCH 02/19] iscsi: Drop deprecated -drive parameter "filename", Markus Armbruster, 2018/06/07
- [Qemu-devel] [RFC PATCH 17/19] block: Fix -blockdev / blockdev-add for empty objects and arrays, Markus Armbruster, 2018/06/07
- [Qemu-devel] [RFC PATCH 06/19] block: Fix -blockdev for certain non-string scalars, Markus Armbruster, 2018/06/07
- [Qemu-devel] [RFC PATCH 16/19] check-block-qdict: Cover flattening of empty lists and dictionaries, Markus Armbruster, 2018/06/07
- [Qemu-devel] [RFC PATCH 12/19] block-qdict: Tweak qdict_flatten_qdict(), qdict_flatten_qlist(), Markus Armbruster, 2018/06/07
- [Qemu-devel] [RFC PATCH 13/19] block-qdict: Clean up qdict_crumple() a bit, Markus Armbruster, 2018/06/07
- [Qemu-devel] [RFC PATCH 04/19] fixup block: Add block-specific QDict header, Markus Armbruster, 2018/06/07
- [Qemu-devel] [RFC PATCH 15/19] check-block-qdict: Rename qdict_flatten()'s variables for clarity, Markus Armbruster, 2018/06/07
- [Qemu-devel] [RFC PATCH 18/19] rbd: New parameter auth-client-required, Markus Armbruster, 2018/06/07
- [Qemu-devel] [RFC PATCH 19/19] rbd: New parameter key-secret, Markus Armbruster, 2018/06/07
- [Qemu-devel] [RFC PATCH 10/19] block: Make remaining uses of qobject input visitor more robust, Markus Armbruster, 2018/06/07