[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v3 3/5] block: qobject_is_equal() i
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v3 3/5] block: qobject_is_equal() in bdrv_reopen_prepare() |
Date: |
Wed, 05 Jul 2017 09:14:22 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) |
Max Reitz <address@hidden> writes:
> Currently, bdrv_reopen_prepare() assumes that all BDS options are
> strings. However, this is not the case if the BDS has been created
> through the json: pseudo-protocol or blockdev-add.
>
> Note that the user-invokable reopen command is an HMP command, so you
> can only specify strings there. Therefore, specifying a non-string
> option with the "same" value as it was when originally created will now
> return an error because the values are supposedly similar (and there is
> no way for the user to circumvent this but to just not specify the
> option again -- however, this is still strictly better than just
> crashing).
>
> Signed-off-by: Max Reitz <address@hidden>
> ---
> block.c | 31 ++++++++++++++++++-------------
> 1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/block.c b/block.c
> index 913bb43..45eb248 100644
> --- a/block.c
> +++ b/block.c
> @@ -2947,19 +2947,24 @@ int bdrv_reopen_prepare(BDRVReopenState
> *reopen_state, BlockReopenQueue *queue,
> const QDictEntry *entry = qdict_first(reopen_state->options);
>
> do {
> - QString *new_obj = qobject_to_qstring(entry->value);
> - const char *new = qstring_get_str(new_obj);
> - /*
> - * Caution: while qdict_get_try_str() is fine, getting
> - * non-string types would require more care. When
> - * bs->options come from -blockdev or blockdev_add, its
> - * members are typed according to the QAPI schema, but
> - * when they come from -drive, they're all QString.
> - */
> - const char *old = qdict_get_try_str(reopen_state->bs->options,
> - entry->key);
> -
> - if (!old || strcmp(new, old)) {
> + QObject *new = entry->value;
> + QObject *old = qdict_get(reopen_state->bs->options, entry->key);
> +
> + /* TODO: When using -drive to specify blockdev options, all
> values
> + * will be strings; however, when using -blockdev, blockdev-add
> or
> + * filenames using the json:{} pseudo-protocol, they will be
> + * correctly typed.
> + * In contrast, reopening options are (currently) always strings
> + * (because you can only specify them through qemu-io; all other
> + * callers do not specify any options).
> + * Therefore, when using anything other than -drive to create a
> BDS,
> + * this cannot detect non-string options as unchanged, because
> + * qobject_is_equal() always returns false for objects of
> different
> + * type. In the future, this should be remedied by correctly
> typing
> + * all options. For now, this is not too big of an issue because
> + * the user simply can not specify options which cannot be
> changed
> + * anyway, so they will stay unchanged. */
I'm not the maintainer, and this is not a demand: consider winging this
comment and wrapping lines around column 70.
As much as I fancy word play (see your reply to Eric), I have to admit
that I had to read "the user simply can not specify options" about three
times to make sense of it. Please consider a wording that's easier to
grasp, perhaps "the user can simply refrain from specifying options that
cannot be changed".
> + if (!qobject_is_equal(new, old)) {
> error_setg(errp, "Cannot change the option '%s'",
> entry->key);
> ret = -EINVAL;
> goto error;
[Qemu-block] [PATCH v3 3/5] block: qobject_is_equal() in bdrv_reopen_prepare(), Max Reitz, 2017/07/03
Re: [Qemu-block] [Qemu-devel] [PATCH v3 3/5] block: qobject_is_equal() in bdrv_reopen_prepare(),
Markus Armbruster <=
[Qemu-block] [PATCH v3 4/5] iotests: Add test for non-string option reopening, Max Reitz, 2017/07/03
[Qemu-block] [PATCH v3 5/5] tests: Add check-qobject for equality tests, Max Reitz, 2017/07/03