From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55591) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bCxXu-00069F-2G for qemu-devel@nongnu.org; Tue, 14 Jun 2016 19:15:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bCxXr-0004y7-II for qemu-devel@nongnu.org; Tue, 14 Jun 2016 19:14:56 -0400 References: <1459967330-4573-1-git-send-email-mreitz@redhat.com> <1459967330-4573-9-git-send-email-mreitz@redhat.com> From: Eric Blake Message-ID: <57608FE7.1070000@redhat.com> Date: Tue, 14 Jun 2016 17:14:47 -0600 MIME-Version: 1.0 In-Reply-To: <1459967330-4573-9-git-send-email-mreitz@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="79ojcJTReAQlfX6L0b2O5fcLogNoVxaOe" Subject: Re: [Qemu-devel] [PATCH v3 08/14] block/nbd: Accept SocketAddress List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-block@nongnu.org Cc: qemu-devel@nongnu.org, Kevin Wolf , Paolo Bonzini , "Daniel P . Berrange" , Markus Armbruster , Luiz Capitulino This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --79ojcJTReAQlfX6L0b2O5fcLogNoVxaOe Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/06/2016 12:28 PM, Max Reitz wrote: > Add a new option "address" to the NBD block driver which accepts a > SocketAddress. >=20 > "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. >=20 > Signed-off-by: Max Reitz > --- > block/nbd.c | 97 ++++++++++++++++++++++++++---------= -------- > tests/qemu-iotests/051.out | 4 +- > tests/qemu-iotests/051.pc.out | 4 +- > 3 files changed, 64 insertions(+), 41 deletions(-) >=20 > 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 n= ame", > e->key); > @@ -202,46 +206,66 @@ out: > g_free(file); > } > =20 > +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 ti= me"); > + 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 =3D NULL; > + QDict *addr =3D NULL; > + QObject *crumpled_addr; > + QmpInputVisitor *iv; > + Error *local_err =3D NULL; > =20 > - if (qdict_haskey(options, "path") =3D=3D qdict_haskey(options, "ho= st")) { > - if (qdict_haskey(options, "path")) { > - error_setg(errp, "path and host may not be used at the sam= e 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; > } > =20 > - saddr =3D g_new0(SocketAddress, 1); > + crumpled_addr =3D qdict_crumple(addr, true, errp); > + if (!crumpled_addr) { > + goto fail; > + } > =20 Ah, so you ARE depending on Dan's qdict_crumple(). > - if (qdict_haskey(options, "path")) { > - UnixSocketAddress *q_unix; > - saddr->type =3D SOCKET_ADDRESS_KIND_UNIX; > - q_unix =3D saddr->u.q_unix.data =3D g_new0(UnixSocketAddress, = 1); > - q_unix->path =3D g_strdup(qdict_get_str(options, "path")); > - qdict_del(options, "path"); > - } else { > - InetSocketAddress *inet; > - saddr->type =3D SOCKET_ADDRESS_KIND_INET; > - inet =3D saddr->u.inet.data =3D g_new0(InetSocketAddress, 1); > - inet->host =3D g_strdup(qdict_get_str(options, "host")); > - if (!qdict_get_try_str(options, "port")) { > - inet->port =3D g_strdup_printf("%d", NBD_DEFAULT_PORT); > - } else { > - inet->port =3D g_strdup(qdict_get_str(options, "port")); > - } > - qdict_del(options, "host"); > - qdict_del(options, "port"); > + iv =3D 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; > } > =20 > + /* 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=3Dtrue 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --79ojcJTReAQlfX6L0b2O5fcLogNoVxaOe Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJXYI/nAAoJEKeha0olJ0Nq1PAH/1iUMLpIazUrm7ZfcDWF6syn wP7gu5Elo3TjLBurJZdCSxj1I/kUoNp4xL0irC/4ISryH+GQLBmwJrNCMpXZx+s5 f5hmE/Y6/0YyTaF+CvFL/WOubTfssogVrfbgKTIxrqHgSWm8xNEoR74H6jHC1yY1 rCU1azfReZ7kjFwytVgb3zpL8fC9uX9l/8EcJAssXe5qf0T5/6J6ZR7IdhWUYKS9 HqlTq7zYxVMnAkTTUaQ3OeYLA5OKHOTRVkP3GTGIV1xqc540c7jPor4TLb500pIs 0Yo+josfx/T1SYQ38VSLELkk10fzRYLXTCH/Gp/LosUQP7OzX1jpXpW9KzjS8Nw= =g0fy -----END PGP SIGNATURE----- --79ojcJTReAQlfX6L0b2O5fcLogNoVxaOe--