Message ID | 1459967330-4573-8-git-send-email-mreitz@redhat.com |
---|---|
State | New |
Headers | show |
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 <mreitz@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...)
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"); + + 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"); + + 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); + } } qdict_put(opts, "driver", qstring_from_str("nbd")); - if (path && export) { - snprintf(bs->exact_filename, sizeof(bs->exact_filename), - "nbd+unix:///%s?socket=%s", export, path); - } else if (path && !export) { - snprintf(bs->exact_filename, sizeof(bs->exact_filename), - "nbd+unix://?socket=%s", path); - } else if (!path && export) { - snprintf(bs->exact_filename, sizeof(bs->exact_filename), - "nbd://%s:%s/%s", host, port, export); - } else if (!path && !export) { - snprintf(bs->exact_filename, sizeof(bs->exact_filename), - "nbd://%s:%s", host, port); - } - - if (path) { - qdict_put(opts, "path", qstring_from_str(path)); + if (can_generate_filename) { + if (path && export) { + snprintf(bs->exact_filename, sizeof(bs->exact_filename), + "nbd+unix:///%s?socket=%s", export, path); + } else if (path && !export) { + snprintf(bs->exact_filename, sizeof(bs->exact_filename), + "nbd+unix://?socket=%s", path); + } else if (!path && export) { + snprintf(bs->exact_filename, sizeof(bs->exact_filename), + "nbd://%s:%s/%s", host, port, export); + } else if (!path && !export) { + snprintf(bs->exact_filename, sizeof(bs->exact_filename), + "nbd://%s:%s", host, 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 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); + } + } } 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));
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 <mreitz@redhat.com> --- block/nbd.c | 84 ++++++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 61 insertions(+), 23 deletions(-)