Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/4] block: qobject_is_equal() in bdrv_reopen_prepare()

[Markus Armbruster]

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).
>
> Reviewed-by: Kevin Wolf <address@hidden>
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/block.c b/block.c
> index fa1d06d..23424d5 100644
> --- a/block.c
> +++ b/block.c
> @@ -2950,19 +2950,10 @@ 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.
> -             */

Your commit message makes me suspect this comment still applies in some
form.  Does it?

> -            const char *old = qdict_get_try_str(reopen_state->bs->options,
> -                                                entry->key);
> +            QObject *new = entry->value;
> +            QObject *old = qdict_get(reopen_state->bs->options, entry->key);
>  
> -            if (!old || strcmp(new, old)) {
> +            if (!qobject_is_equal(new, old)) {
>                  error_setg(errp, "Cannot change the option '%s'", 
> entry->key);
>                  ret = -EINVAL;
>                  goto error;