Re: [Qemu-devel] [PATCH for-2.9 3/5] rbd: Rewrite the code to extract list-valued options

[Eric Blake]

On 03/24/2017 03:25 AM, Markus Armbruster wrote:

>>>> -            value = host;
>>>> -            if (port) {

>>>> +        host = qstring_get_str(qobject_to_qstring(val));
>>>> +        sprintf(keybuf, "server.%d.port", i);
>>>> +        port = qdict_get_str(options, keybuf);
>>>
>>> This segfaults if the port option isn't given.
>>
>> @port is mandatory in BlockdevOptionsRbd.  If it's not there here, the
>> options must have bypassed QAPI.  That would be bad news.  Can you
>> explain how it can happen?
> 
> Answering myself, please correct mistakes.
> 
> There are two ways to create @options:
> 
> 1. -blockdev and blockdev-add
> 
>    These create @options with a QAPI visitor from the command line
>    option argument or QMP arguments, respectively.  This checks them
>    against the QAPI schema.  Missing @port is rejected.
> 
> 2. -drive and drive_add
> 
>    These appear to create @options manually, without checking against
>    the QAPI schema.
> 
>    Crash reproducer: -drive driver=rbd,server.0.host=s0

Yep - and that's why the old code was checking 'if (port)'.

> 
> In other words, we have *two* specifications for @options: the QAPI
> schema, and the union of all the QemuOptsList that apply.  In case 1, we
> check against both (I think).  In case 2, we only check against the
> latter.
> 
> I understand how we got into this state, but it's not a good state to be
> in.  We need to have our options defined in one way and one way only.
> 
> For 2.9, we cope with missing @port.

Maybe for 2.10 we would make port optional rather than mandatory - but
that's a LOT of code to audit to add in appropriate 'has_port=true' so
it's too late to do for 2.9.  Or even better, maybe for 2.10 we finally
implement parameter defaults so that we don't need a has_port field (if
port is omitted, it gets assigned the default value).  Except that I
don't know if all the existing users of InetSocketAddress will want the
same default.  Maybe that argues that we want some way to declare a QAPI
subtype that changes the default of a field otherwise inherited from the
base class (so port is mandatory in the base class, but each instance
that wants a default port creates a new subclass with a new default value).

> 
> Post 2.9, we should either finish the QAPIfication of block
> configuration we started with blockdev-add, or back it out, i.e. make
> the QAPI schema accept anything, and rely on the QemuOpts-based
> checking.
> 
> I want us to finish QAPIfication.

I'd like to finish the QAPIfication as well.  I'm fine if port is
mandatory for QMP and -blockdev-add for 2.9, where only -drive gets away
with the default (but it DOES mean that we have to make sure that
QemuOpts created by -drive is augmented with the default before we pass
it through QAPI validation), so that back-compat of -drive omitting port
doesn't break.


>>> Of course, this also changes the behaviour so that additional options in
>>> server.* and auth-supported.* aren't silently ignored any more, but we
>>> complain that they are unknown. I consider this a bonus bug fix, but it
>>> should probably be spelt out in the commit message.
>>
>> Good point.
> 
> Note to self: this applies to -drive / drive_add, but not to -blockdev /
> blockdev_add, because the QAPI schema kicks in there.  Example:
> -drive driver=rbd,server.0.host=s0,server.0.port=p0,server.0.foo=bar

Yep - another instance where -drive parsing is slightly different than
-blockdev-add parsing.  However, while tightening the parse to require
port is not back-compat friendly to -drive, I think that tightening the
parse to reject garbage given to -drive is perfectly acceptable.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature