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

[Max Reitz]

On 2017-06-21 18:06, Markus Armbruster wrote:
> 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?

The only thing that I can think of that may be applicable is what I
wrote in the commit message; this fails now:

$ qemu-io -c 'reopen -o size=65536' \
    "json:{'driver':'null-co','size':65536}"
Cannot change the option 'size'

As I wrote in the commit message, though, I don't think this is too bad.
First, before, it just crashed, so this definitely is better behavior.

Secondly, I think this is just good for convenience; it allows the user
to specify an option (to "change" it) even if the block driver doesn't
support changing it. If it actually has not change, this is supposed to
be handled gracefully. But sometimes we cannot easily handle it, so...
We just give an error.

I suspect that at some point we want to fix this by having everything
correctly typed internally...? Until then, in my opinion, we can just
not provide this feature.

However, I should have probably not just deleted the comment and hoped
everyone can find the necessary information through git-blame. I should
leave a comment about this here.


Or we do make an effort to provide this test-unchanged functionality for
every case, in which case we would have to convert either string options
to the correct type (according to the schema) here (but if that were so
simple, we could do that centrally in bdrv_open() already, right?) or
convert typed options to strings and compare them -- but since there are
probably multiple different strings that can mean the same value for
whatever type the option has, this won't work in every case either.

So I'm for leaving a comment and leaving this not quite as it should be
until we have fixed all of the block layer (O:-)). Maybe you have a
better idea though, which would be great.

Max

>> -            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;

signature.asc
Description: OpenPGP digital signature