From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56761) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buytD-0002Mk-9D for qemu-devel@nongnu.org; Fri, 14 Oct 2016 05:34:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1buytB-0008ET-1j for qemu-devel@nongnu.org; Fri, 14 Oct 2016 05:34:54 -0400 MIME-Version: 1.0 In-Reply-To: <20161013114244.GG5803@noname.redhat.com> References: <20160928205602.17275-1-mreitz@redhat.com> <20160928205602.17275-7-mreitz@redhat.com> <20161013114244.GG5803@noname.redhat.com> From: Ashijeet Acharya Date: Fri, 14 Oct 2016 15:04:47 +0530 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v4 06/12] block/nbd: Accept SocketAddress List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: Max Reitz , qemu-block@nongnu.org, QEMU Developers , Eric Blake , Paolo Bonzini , Markus Armbruster On Thu, Oct 13, 2016 at 5:12 PM, Kevin Wolf wrote: > Am 28.09.2016 um 22:55 hat Max Reitz geschrieben: >> 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 > > Not opposed in principle to your change, but we should try to keep the > naming consistent between NBD and the other block drivers, notably the > SSH work that is currently going on. > > This patch uses 'address' for the SockAddress, the proposed SSH patch > uses 'server'. I don't mind too much which one we choose, though I like > 'server' a bit better. Anyway, we should choose one and stick to it in > all drivers. > >> block/nbd.c | 166 ++++++++++++++++++++++++++---------------- >> tests/qemu-iotests/051.out | 4 +- >> tests/qemu-iotests/051.pc.out | 4 +- >> 3 files changed, 106 insertions(+), 68 deletions(-) >> >> diff --git a/block/nbd.c b/block/nbd.c >> index cdab20f..449f94e 100644 >> --- a/block/nbd.c >> +++ b/block/nbd.c >> @@ -32,6 +32,9 @@ >> #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-output-visitor.h" >> #include "qapi/qmp/qdict.h" >> #include "qapi/qmp/qjson.h" >> #include "qapi/qmp/qint.h" >> @@ -44,7 +47,8 @@ typedef struct BDRVNBDState { >> NbdClientSession client; >> >> /* For nbd_refresh_filename() */ >> - char *path, *host, *port, *export, *tlscredsid; >> + SocketAddress *saddr; >> + char *export, *tlscredsid; >> } BDRVNBDState; >> >> static int nbd_parse_uri(const char *filename, QDict *options) >> @@ -131,7 +135,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)) > > strstart()? > >> { >> error_setg(errp, "Option '%s' cannot be used with a file name", >> e->key); >> @@ -205,50 +211,67 @@ out: >> g_free(file); >> } >> >> -static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, Error **errp) >> +static bool nbd_process_legacy_socket_options(QDict *output_options, >> + QemuOpts *legacy_opts, >> + Error **errp) >> { >> - SocketAddress *saddr; >> - >> - s->path = g_strdup(qemu_opt_get(opts, "path")); >> - s->host = g_strdup(qemu_opt_get(opts, "host")); >> - s->port = g_strdup(qemu_opt_get(opts, "port")); >> - >> - if (!s->path == !s->host) { >> - if (s->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"); >> + const char *path = qemu_opt_get(legacy_opts, "path"); >> + const char *host = qemu_opt_get(legacy_opts, "host"); >> + const char *port = qemu_opt_get(legacy_opts, "port"); I am working on a similar task for the SSH block driver, and in my 'ssh_process_legacy_socket_options' I get the @host and @port fields still pointing to NULL even after qemu_opt_get(). To clarify things a bit more I tried to debug the same on your patch using gdb and faced the same issue. So in your patch, ultimately the control flows directly to the last statement i.e. 'return true;' and none of the 'if' conditions get satisfied. Reverting the patches and using the same command-line seems to overcome the issue. Command line I used: $ ./bin/qemu-system-x86_64 -cdrom nbd://localhost/precise-5.7.1.iso I can be wrong so I advise debugging yourself once and correcting me if I am wrong. Link to my patch: https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg02139.html Thanks Ashijeet > Kevin