From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49205) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bvSVu-0002Qx-8C for qemu-devel@nongnu.org; Sat, 15 Oct 2016 13:12:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bvSVr-0005ur-VR for qemu-devel@nongnu.org; Sat, 15 Oct 2016 13:12:49 -0400 References: <20160928205602.17275-1-mreitz@redhat.com> <20160928205602.17275-7-mreitz@redhat.com> <20161013114244.GG5803@noname.redhat.com> From: Max Reitz Message-ID: <4b932f12-e8ca-9816-970d-e9ea7b30f538@redhat.com> Date: Sat, 15 Oct 2016 19:12:35 +0200 MIME-Version: 1.0 In-Reply-To: <20161013114244.GG5803@noname.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="8LW7xhAraldc6XcxslCQ0E4DoTpt4FBt9" 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: qemu-block@nongnu.org, qemu-devel@nongnu.org, Eric Blake , Paolo Bonzini , Markus Armbruster , ashijeetacharya@gmail.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --8LW7xhAraldc6XcxslCQ0E4DoTpt4FBt9 From: Max Reitz To: Kevin Wolf Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, Eric Blake , Paolo Bonzini , Markus Armbruster , ashijeetacharya@gmail.com Message-ID: <4b932f12-e8ca-9816-970d-e9ea7b30f538@redhat.com> Subject: Re: [PATCH v4 06/12] block/nbd: Accept SocketAddress References: <20160928205602.17275-1-mreitz@redhat.com> <20160928205602.17275-7-mreitz@redhat.com> <20161013114244.GG5803@noname.redhat.com> In-Reply-To: <20161013114244.GG5803@noname.redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 13.10.2016 13:42, 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 ar= e >> mapped to their corresponding SocketAddress representation. >> >> Signed-off-by: Max Reitz >=20 > 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. >=20 > 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. OK, I'll use "server" then. I don't mind either way, so even a weak opinion is enough to persuade me. ;-) >> 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; >> =20 >> /* For nbd_refresh_filename() */ >> - char *path, *host, *port, *export, *tlscredsid; >> + SocketAddress *saddr; >> + char *export, *tlscredsid; >> } BDRVNBDState; >> =20 >> static int nbd_parse_uri(const char *filename, QDict *options) >> @@ -131,7 +135,9 @@ static bool nbd_has_filename_options_conflict(QDic= t *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)) >=20 > strstart()? Yep, will use. >> { >> error_setg(errp, "Option '%s' cannot be used with a file = name", >> e->key); >> @@ -205,50 +211,67 @@ out: >> g_free(file); >> } >> =20 >> -static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, Err= or **errp) >> +static bool nbd_process_legacy_socket_options(QDict *output_options, >> + QemuOpts *legacy_opts, >> + Error **errp) >> { >> - SocketAddress *saddr; >> - >> - s->path =3D g_strdup(qemu_opt_get(opts, "path")); >> - s->host =3D g_strdup(qemu_opt_get(opts, "host")); >> - s->port =3D g_strdup(qemu_opt_get(opts, "port")); >> - >> - if (!s->path =3D=3D !s->host) { >> - if (s->path) { >> - error_setg(errp, "path and host may not be used at the sa= me time"); >> - } else { >> - error_setg(errp, "one of path and host must be specified"= ); >> + const char *path =3D qemu_opt_get(legacy_opts, "path"); >> + const char *host =3D qemu_opt_get(legacy_opts, "host"); >> + const char *port =3D qemu_opt_get(legacy_opts, "port"); >> + >> + if (path && host) { >> + error_setg(errp, "path and host may not be used at the same t= ime"); >> + return false; >> + } else if (path) { >> + if (port) { >> + error_setg(errp, "port may not be used without host"); >> + return false; >> } >> - return NULL; >> + >> + qdict_put(output_options, "address.type", qstring_from_str("u= nix")); >> + qdict_put(output_options, "address.data.path", qstring_from_s= tr(path)); >> + } else if (host) { >> + qdict_put(output_options, "address.type", qstring_from_str("i= net")); >> + qdict_put(output_options, "address.data.host", qstring_from_s= tr(host)); >> + qdict_put(output_options, "address.data.port", >> + qstring_from_str(port ?: stringify(NBD_DEFAULT_PORT= ))); >> } >> - if (s->port && !s->host) { >> - error_setg(errp, "port may not be used without host"); >> - return NULL; >> + >> + return true; >> +} >=20 > If both the legacy option and the new one are given, the legacy one > takes precedence. Intentional? No. I think it'll be easiest to just return an error when a user is trying to use both. Thanks, Max --8LW7xhAraldc6XcxslCQ0E4DoTpt4FBt9 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEvBAEBCAAZBQJYAmODEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRD0B9sAYdXPQAkH B/4uQZw4cjKazABymfrI6xnx8mJbnLFths89/cYBo4VDJMGCoM3g2I/D3WTJFDy+ d16FQ16jE44xaUkwcl5scheQ/n5ztQLjmIZuJsO0+w1BsaVHQHI2I9cR8kadXTTS NHzghIccC1Kj15K48BVRFkxaYZbFqt/BNErtVoBjxOTeUOWPBnXjKOKPMns7l2WL ikXSQsCN4U0jQJrUMftMjtl/Qfyr8/tG9yhJRc1cYYSiwiIC1X3r1XC3mFc3rCAf rBBKRBbmvsHVMxt9r8/0YHUxsIicKtr9I6xNTTbg5zmiRMnUF4yNy8rGjLc6gaU2 iq5r9aGo6VTn36vITLdpzk1O =a2VU -----END PGP SIGNATURE----- --8LW7xhAraldc6XcxslCQ0E4DoTpt4FBt9--