On 04/06/2016 12:28 PM, Max Reitz wrote: > As of a future patch, the NBD block driver will accept a SocketAddress > structure for a new "address" option. In order to support this, > nbd_refresh_filename() needs some changes. > > The two TODOs introduced by this patch will be removed in the very next > one. They exist to explain that it is currently impossible for > nbd_refresh_filename() to emit an "address.*" option (which the NBD > block driver does not handle yet). The next patch will arm these code > paths, but it will also enable handling of these options. > > Signed-off-by: Max Reitz <mre...@redhat.com> > --- > block/nbd.c | 84 > ++++++++++++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 61 insertions(+), 23 deletions(-) > > diff --git a/block/nbd.c b/block/nbd.c > index 1736f68..3adf302 100644 > --- a/block/nbd.c > +++ b/block/nbd.c > @@ -441,37 +441,75 @@ static void nbd_attach_aio_context(BlockDriverState *bs, > static void nbd_refresh_filename(BlockDriverState *bs, QDict *options) > { > QDict *opts = qdict_new(); > - const char *path = qdict_get_try_str(options, "path"); > - const char *host = qdict_get_try_str(options, "host"); > - const char *port = qdict_get_try_str(options, "port"); > + bool can_generate_filename = true; > + const char *path = NULL, *host = NULL, *port = NULL; > const char *export = qdict_get_try_str(options, "export"); > const char *tlscreds = qdict_get_try_str(options, "tls-creds"); > > - if (host && !port) { > - port = stringify(NBD_DEFAULT_PORT); > + if (qdict_get_try_str(options, "address.type")) { > + /* This path will only be possible as of a future patch; > + * TODO: Remove this note once it is */ > + > + const char *type = qdict_get_str(options, "address.type"); > +
Oh, I'm sooooo tempted to teach the QAPI generator how to make a discriminated union have a default 'type' value (thus making the discriminator optional), so that we don't need a layer of nesting behind 'address.'. > + if (!strcmp(type, "unix")) { > + path = qdict_get_str(options, "address.data.path"); > + } else if (!strcmp(type, "inet")) { > + host = qdict_get_str(options, "address.data.host"); > + port = qdict_get_str(options, "address.data.port"); It's especially annoying that because SocketAddress is not flat, we have to expose the 'data.' layer of nesting, even if we could avoid the 'address.' layer. > + > + can_generate_filename = !qdict_haskey(options, "address.data.to") > + && !qdict_haskey(options, > "address.data.ipv4") > + && !qdict_haskey(options, > "address.data.ipv6"); > + } else { > + can_generate_filename = false; > + } > + } else { > + path = qdict_get_try_str(options, "path"); > + host = qdict_get_try_str(options, "host"); > + port = qdict_get_try_str(options, "port"); > + > + if (host && !port) { > + port = stringify(NBD_DEFAULT_PORT); > + } > } Looks clean given the constraints of what you are able to use from QAPI. > + > + if (qdict_get_try_str(options, "address.type")) { > + /* This path will only be possible as of a future patch; > + * TODO: Remove this note once it is */ > + > + const QDictEntry *e; > + for (e = qdict_first(options); e; e = qdict_next(options, e)) { > + if (!strncmp(e->key, "address.", 8)) { > + qobject_incref(e->value); > + qdict_put_obj(opts, e->key, e->value); > + } > + } This part makes me wonder if we want Dan's qdict_crumple() working first. > } else { > - qdict_put(opts, "host", qstring_from_str(host)); > - qdict_put(opts, "port", qstring_from_str(port)); > + if (path) { > + qdict_put(opts, "path", qstring_from_str(path)); > + } else { > + qdict_put(opts, "host", qstring_from_str(host)); > + qdict_put(opts, "port", qstring_from_str(port)); > + } > } > if (export) { > qdict_put(opts, "export", qstring_from_str(export)); > At this point, I'll reserve giving R-b until I've seen the whole series (it may need rebasing anyways...) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature