[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [for-2.9 4/8] block: Document -drive probl
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [Qemu-devel] [for-2.9 4/8] block: Document -drive problematic code and bugs |
Date: |
Thu, 30 Mar 2017 08:09:33 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 |
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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-block] [for-2.9 7/8] nbd: Tidy up blockdev-add interface, Markus Armbruster, 2017/03/29