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 <[email protected]> > --- > 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
