Re: [Qemu-devel] [PATCH v3 for-2.9 4/9] block: Document -drive problematic code and bugs

[Max Reitz]

On 30.03.2017 20:56, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> On 03/30/2017 12:43 PM, Markus Armbruster wrote:
>>> -blockdev and blockdev_add convert their arguments via QObject to
>>> BlockdevOptions for qmp_blockdev_add(), which converts them back to
>>> QObject, then to a flattened QDict.  The QDict's members are typed
>>> according to the QAPI schema.
>>>
>>> -drive converts its argument via QemuOpts to a (flat) QDict.  This
>>> QDict's members are all QString.
>>>
>>> Thus, the QType of a flat QDict member depends on whether it comes
>>> from -drive or -blockdev/blockdev_add, except when the QAPI type maps
>>> to QString, which is the case for 'str' and enumeration types.
>>>
>>
>>> +++ b/block/file-posix.c
>>> @@ -2193,6 +2193,12 @@ static int hdev_open(BlockDriverState *bs, QDict 
>>> *options, int flags,
>>>      int ret;
>>>  
>>>  #if defined(__APPLE__) && defined(__MACH__)
>>> +    /*
>>> +     * Caution: while qdict_get_try_str() is fine, getting non-string
>>> +     * types would require more care.  When @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 *filename = qdict_get_str(options, "filename");
>>
>> Comment mentions (via copy-and-past) qdict_get_try_str(), but this
>> instance uses qdict_get_str().  Doesn't bother me enough to withhold
>> review if you leave it, though.
>>
>>
>>> +++ b/block/nfs.c
>>> @@ -474,6 +474,13 @@ static NFSServer *nfs_config(QDict *options, Error 
>>> **errp)
>>>          goto out;
>>>      }
>>>  
>>> +    /*
>>> +     * Caution: this works only because all scalar members of
>>> +     * NFSServer are QString in @crumpled_addr.  The visitor expects
>>> +     * @crumpled_addr to be typed according to the QAPI scherma.  It
>>
>> s/scherma/schema/
> 
> In case nothing else comes up, I hope both can be touched up on commit.
> Max?

Sure, will do.

Max

>>> +     * is when @options come from -blockdev or blockdev_add.  But when
>>> +     * they come from -drive, they're all QString.
>>> +     */
>>
>> With the typo fix,
>> Reviewed-by: Eric Blake <address@hidden>
> 
> Thanks!
>

signature.asc
Description: OpenPGP digital signature