[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [for-2.9 4/8] block: Document -drive problematic code a
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [for-2.9 4/8] block: Document -drive problematic code and bugs |
Date: |
Thu, 30 Mar 2017 16:42:22 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) |
Eric Blake <address@hidden> 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.
[Qemu-devel] [for-2.9 6/8] qapi-schema: SocketAddressFlat variants 'vsock' and 'fd', Markus Armbruster, 2017/03/29
[Qemu-devel] [for-2.9 5/8] gluster: Prepare for SocketAddressFlat extension, Markus Armbruster, 2017/03/29