[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v3 08/14] block/nbd: Accept SocketAddress
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH v3 08/14] block/nbd: Accept SocketAddress |
Date: |
Tue, 14 Jun 2016 17:14:47 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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 <address@hidden>
> ---
> 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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-block] [PATCH v3 08/14] block/nbd: Accept SocketAddress,
Eric Blake <=