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