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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2349 bytes --]

On 07/03/2017 07:25 AM, Max Reitz wrote:
> 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

s/invokable/invocable/

> 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 <mreitz@redhat.com>
> ---
>  block.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 

> +            /* 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

Seeing "can not" usually looks wrong for "cannot"; but here, it is
grammatically correct.  But better would be "the user can simply not
specify" or "the user can simply avoid specifying options".

> +             * anyway, so they will stay unchanged. */

The grammar tweak is minor, so:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]