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
