On 15.06.2016 01:14, Eric Blake wrote: > On 04/06/2016 12:28 PM, Max Reitz wrote: >> Add a new option "address" to the NBD block driver which accepts a >> SocketAddress. >> >> "path", "host" and "port" are still supported as legacy options and are >> mapped to their corresponding SocketAddress representation. > > The back-compat work is pretty slick. > >> >> Signed-off-by: Max Reitz <mre...@redhat.com> >> --- >> block/nbd.c | 97 >> ++++++++++++++++++++++++++----------------- >> tests/qemu-iotests/051.out | 4 +- >> tests/qemu-iotests/051.pc.out | 4 +- >> 3 files changed, 64 insertions(+), 41 deletions(-) >> >> diff --git a/block/nbd.c b/block/nbd.c >> index 3adf302..9ae66ba 100644 >> --- a/block/nbd.c >> +++ b/block/nbd.c >> @@ -32,6 +32,8 @@ >> #include "qemu/uri.h" >> #include "block/block_int.h" >> #include "qemu/module.h" >> +#include "qapi-visit.h" >> +#include "qapi/qmp-input-visitor.h" >> #include "qapi/qmp/qdict.h" >> #include "qapi/qmp/qjson.h" >> #include "qapi/qmp/qint.h" >> @@ -128,7 +130,9 @@ static bool nbd_has_filename_options_conflict(QDict >> *options, Error **errp) >> if (!strcmp(e->key, "host") >> || !strcmp(e->key, "port") >> || !strcmp(e->key, "path") >> - || !strcmp(e->key, "export")) >> + || !strcmp(e->key, "export") >> + || !strcmp(e->key, "address") >> + || !strncmp(e->key, "address.", 8)) > > May need a tweak if you break before '||'
Will do. (Same for patch 6.) > >> { >> error_setg(errp, "Option '%s' cannot be used with a file name", >> e->key); >> @@ -202,46 +206,66 @@ out: >> g_free(file); >> } >> >> +static bool nbd_process_legacy_socket_options(QDict *options, Error **errp) >> +{ >> + if (qdict_haskey(options, "path") && qdict_haskey(options, "host")) { >> + error_setg(errp, "path and host may not be used at the same time"); >> + return false; >> + } else if (qdict_haskey(options, "path")) { >> + if (qdict_haskey(options, "port")) { >> + error_setg(errp, "port may not be used without host"); >> + return false; >> + } >> + >> + qdict_put(options, "address.type", qstring_from_str("unix")); >> + qdict_change_key(options, "path", "address.data.path"); >> + } else if (qdict_haskey(options, "host")) { >> + qdict_put(options, "address.type", qstring_from_str("inet")); >> + qdict_change_key(options, "host", "address.data.host"); >> + if (!qdict_change_key(options, "port", "address.data.port")) { >> + qdict_put(options, "address.data.port", >> + qstring_from_str(stringify(NBD_DEFAULT_PORT))); >> + } > > The rewrite from old to modern is rather nice. I wonder if we could > then use a generated qapi_visit_SocketAddress instead of manually > handling all the complication of SocketAddress proper. > >> + } >> + >> + return true; >> +} >> + >> static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char >> **export, >> Error **errp) >> { >> - SocketAddress *saddr; >> + SocketAddress *saddr = NULL; >> + QDict *addr = NULL; >> + QObject *crumpled_addr; >> + QmpInputVisitor *iv; >> + Error *local_err = NULL; >> >> - if (qdict_haskey(options, "path") == qdict_haskey(options, "host")) { >> - if (qdict_haskey(options, "path")) { >> - error_setg(errp, "path and host may not be used at the same >> time"); >> - } else { >> - error_setg(errp, "one of path and host must be specified"); >> - } >> - return NULL; >> + if (!nbd_process_legacy_socket_options(options, errp)) { >> + goto fail; >> } >> - if (qdict_haskey(options, "port") && !qdict_haskey(options, "host")) { >> - error_setg(errp, "port may not be used without host"); >> - return NULL; >> + >> + qdict_extract_subqdict(options, &addr, "address."); >> + if (!qdict_size(addr)) { >> + error_setg(errp, "NBD server address missing"); >> + goto fail; >> } >> >> - saddr = g_new0(SocketAddress, 1); >> + crumpled_addr = qdict_crumple(addr, true, errp); >> + if (!crumpled_addr) { >> + goto fail; >> + } >> > > Ah, so you ARE depending on Dan's qdict_crumple(). In v2, I was still using my own version (qdict_unflatten()), written specifically for this. Since both fulfill the same purpose and I got to review qdict_crumple() before anyone got to review qdict_unflatten(), I dropped the qdict_unflatten() patch from v3; also, "crumple" is more imaginative than "unflatten", so it has to be better. :-) >> - if (qdict_haskey(options, "path")) { >> - UnixSocketAddress *q_unix; >> - saddr->type = SOCKET_ADDRESS_KIND_UNIX; >> - q_unix = saddr->u.q_unix.data = g_new0(UnixSocketAddress, 1); >> - q_unix->path = g_strdup(qdict_get_str(options, "path")); >> - qdict_del(options, "path"); >> - } else { >> - InetSocketAddress *inet; >> - saddr->type = SOCKET_ADDRESS_KIND_INET; >> - inet = saddr->u.inet.data = g_new0(InetSocketAddress, 1); >> - inet->host = g_strdup(qdict_get_str(options, "host")); >> - if (!qdict_get_try_str(options, "port")) { >> - inet->port = g_strdup_printf("%d", NBD_DEFAULT_PORT); >> - } else { >> - inet->port = g_strdup(qdict_get_str(options, "port")); >> - } >> - qdict_del(options, "host"); >> - qdict_del(options, "port"); >> + iv = qmp_input_visitor_new(crumpled_addr); >> + visit_type_SocketAddress(qmp_input_get_visitor(iv), NULL, &saddr, >> + &local_err); > > and you DO use the generated QAPI code. Cool! > >> + qmp_input_visitor_cleanup(iv); >> + if (local_err) { >> + error_propagate(errp, local_err); >> + goto fail; >> } >> >> + /* TODO: Detect superfluous (unused) options under in addr */ > > Is this TODO addressed anywhere, or mentioned in the commit message? Not yet (also, it should be either of "under" and "in" in the comment, but not both). > In fact, I think it's quite easy to address: qmp_input_visitor_new() > recently changed signatures (commit fc471c18), so all you have to do is > pass strict=true to that function as part of rebasing your work, at > which point you will automatically reject any leftover keys in addr that > didn't get parsed by SocketAddress. Sounds good, will look into it! > Needs a rebase, but looks promising. Will do, thanks for reviewing! Max
signature.asc
Description: OpenPGP digital signature