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

[Max Reitz]

On 2017-07-05 09:14, 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).
>>
>> 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.

I actually did consider wrapping around column 70 and decided against it
because this comment is already indented by 12 characters, so none of
the lines exceed 65 characters (80 - (12 + strlen(" * "))).

About winging...  For some reason I don't quite like it here.  But it
probably is better because the comment is not immediately related to the
qobject_is_equal() call below, so I'll do it.

> 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".

Aw.  I've wanted this to be an example I could point people to to
educate them about the difference.  Now, alas, none shall learn. :-(

Max

signature.asc
Description: OpenPGP digital signature