On 30.03.2017 16:42, Markus Armbruster wrote:
> Eric Blake <ebl...@redhat.com> writes:
>
>> On 03/30/2017 01:52 AM, Markus Armbruster wrote:
>>
>>>>> +++ b/block.c
>>>>> @@ -1157,6 +1157,12 @@ static int bdrv_open_common(BlockDriverState *bs,
>>>>> BlockBackend *file,
>>>>> if (file != NULL) {
>>>>> filename = blk_bs(file)->filename;
>>>>> } else {
>>>>> + /*
>>>>> + * Caution: direct use of non-string @options members is
>>>>> + * problematic. When they come from -blockdev or blockdev_add,
>>>>> + * members are typed according to the QAPI schema, but when
>>>>> + * they come from -drive, they're all QString.
>>>>> + */
>>>>> filename = qdict_get_try_str(options, "filename");
>>>>
>>>> For instance this one: Well, yes, for -drive, this will always be a
>>>> QString. Which is OK, because that's what we're trying to get.
>>>>
>>>> The comment makes this confusing, IMO. If you really want a comment here
>>>> it should at least contain a mention that it's totally fine in practice
>>>> here. Calling the code "problematic" sounds like this could blow up when
>>>> it reality it can't; and I would think it actually is the most sane
>>>> solution given the current state of the whole infrastructure (i.e. how
>>>> -drive and -blockdev work).
>>>
>>> Well, if it could blow up, I'd call it wrong, and start the comment with
>>> FIXME :)
>>>
>>> Even though qdict_get_try_str() is indeed fine, I propose to have a
>>> comment, because someone with less detailed understanding of how the
>>> configuration machine works (me, until yesterday, and probably again
>>> after next month) could conclude that qdict_get_try_bool() would be just
>>> as fine.
>>>
>>> What about:
>>>
>>> /*
>>> * 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.
>>> */
>>
>> Yes, that's better - it makes it obvious that our current usage works,
>> but that the code must not be carelessly edited if we add another field
>> in the future.
>
> If Max is also happy with it, I'll put it in v3.Yes, more than happy, thanks! (Same for the other comment.) Max
signature.asc
Description: OpenPGP digital signature
