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

[Markus Armbruster]

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?

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