| Message ID | 1459967330-4573-9-git-send-email-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <mreitz@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 '||' > { > 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(). > - 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? 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. Needs a rebase, but looks promising.
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 <mreitz@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
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)) { 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))); + } + } + + 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; + } - 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); + qmp_input_visitor_cleanup(iv); + if (local_err) { + error_propagate(errp, local_err); + goto fail; } + /* TODO: Detect superfluous (unused) options under in addr */ + s->client.is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX; *export = g_strdup(qdict_get_try_str(options, "export")); @@ -249,7 +273,12 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, char **export, qdict_del(options, "export"); } + QDECREF(addr); return saddr; + +fail: + QDECREF(addr); + return NULL; } NbdClientSession *nbd_get_client_session(BlockDriverState *bs) @@ -447,9 +476,6 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options) const char *tlscreds = qdict_get_try_str(options, "tls-creds"); 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")) { @@ -493,9 +519,6 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options) } 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)) { diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out index 5b3e991..2f5c91b 100644 --- a/tests/qemu-iotests/051.out +++ b/tests/qemu-iotests/051.out @@ -222,7 +222,7 @@ Testing: -drive driver=file QEMU_PROG: -drive driver=file: The 'file' block driver requires a file name Testing: -drive driver=nbd -QEMU_PROG: -drive driver=nbd: one of path and host must be specified +QEMU_PROG: -drive driver=nbd: NBD server address missing Testing: -drive driver=raw QEMU_PROG: -drive driver=raw: Can't use 'raw' as a block driver for the protocol level @@ -231,7 +231,7 @@ Testing: -drive file.driver=file QEMU_PROG: -drive file.driver=file: The 'file' block driver requires a file name Testing: -drive file.driver=nbd -QEMU_PROG: -drive file.driver=nbd: one of path and host must be specified +QEMU_PROG: -drive file.driver=nbd: NBD server address missing Testing: -drive file.driver=raw QEMU_PROG: -drive file.driver=raw: Can't use 'raw' as a block driver for the protocol level diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out index 6395a30..603bb76 100644 --- a/tests/qemu-iotests/051.pc.out +++ b/tests/qemu-iotests/051.pc.out @@ -316,7 +316,7 @@ Testing: -drive driver=file QEMU_PROG: -drive driver=file: The 'file' block driver requires a file name Testing: -drive driver=nbd -QEMU_PROG: -drive driver=nbd: one of path and host must be specified +QEMU_PROG: -drive driver=nbd: NBD server address missing Testing: -drive driver=raw QEMU_PROG: -drive driver=raw: Can't use 'raw' as a block driver for the protocol level @@ -325,7 +325,7 @@ Testing: -drive file.driver=file QEMU_PROG: -drive file.driver=file: The 'file' block driver requires a file name Testing: -drive file.driver=nbd -QEMU_PROG: -drive file.driver=nbd: one of path and host must be specified +QEMU_PROG: -drive file.driver=nbd: NBD server address missing Testing: -drive file.driver=raw QEMU_PROG: -drive file.driver=raw: Can't use 'raw' as a block driver for the protocol level
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. Signed-off-by: Max Reitz <mreitz@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(-)