SocketAddress is a simple union, and simple unions are awkward: they
have their variant members wrapped in a "data" object on the wire, and
require additional indirections in C. I intend to limit its use to
existing external interfaces, and convert all internal interfaces to
SocketAddressFlat.
BlockdevOptionsNbd is an external interface using SocketAddress, but
it's new in 2.9. Replace it by SocketAddressFlat while we can. This
simplifies
{ "execute": "blockdev-add",
"arguments": { "node-name": "foo", "driver": "nbd",
"server": { "type": "inet",
"data": { "host": "localhost",
"port": "12345" } } } }
to
{ "execute": "blockdev-add",
"arguments": { "node-name": "foo", "driver": "nbd",
"server": { "type": "inet",
"host": "localhost", "port": "12345" } } }
Since the internal interfaces still take SocketAddress, this requires
conversion function socket_address_crumple(). It'll go away when I
update the interfaces.
Unfortunately, SocketAddress is also visible in -drive since 2.8. Add
still more gunk to nbd_process_legacy_socket_options(). You can now
shorten
-drive if=none,driver=nbd,server.type=inet,server.data.host=127.0.0.1,server.data.port=12345
to
-drive if=none,driver=nbd,server.type=inet,server.host=127.0.0.1,server.port=12345
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
block/nbd.c | 131 ++++++++++++++++++++++++++++++++++++++++-----------
qapi/block-core.json | 2 +-
2 files changed, 104 insertions(+), 29 deletions(-)
diff --git a/block/nbd.c b/block/nbd.c
index 11e3ba7..199cb06 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -33,6 +33,7 @@
#include "block/block_int.h"
#include "qemu/module.h"
#include "qapi-visit.h"
+#include "qapi/clone-visitor.h"
#include "qapi/qobject-input-visitor.h"
#include "qapi/qobject-output-visitor.h"
#include "qapi/qmp/qdict.h"
@@ -47,7 +48,7 @@ typedef struct BDRVNBDState {
NBDClientSession client;
/* For nbd_refresh_filename() */
- SocketAddress *saddr;
+ SocketAddressFlat *saddr;
char *export, *tlscredsid;
} BDRVNBDState;
@@ -95,7 +96,7 @@ static int nbd_parse_uri(const char *filename, QDict *options)
goto out;
}
qdict_put(options, "server.type", qstring_from_str("unix"));
- qdict_put(options, "server.data.path",
+ qdict_put(options, "server.path",
qstring_from_str(qp->p[0].value));
} else {
QString *host;
@@ -116,10 +117,10 @@ static int nbd_parse_uri(const char *filename, QDict *options)
}
qdict_put(options, "server.type", qstring_from_str("inet"));
- qdict_put(options, "server.data.host", host);
+ qdict_put(options, "server.host", host);
port_str = g_strdup_printf("%d", uri->port ?: NBD_DEFAULT_PORT);
- qdict_put(options, "server.data.port", qstring_from_str(port_str));
+ qdict_put(options, "server.port", qstring_from_str(port_str));
g_free(port_str);
}
@@ -197,7 +198,7 @@ static void nbd_parse_filename(const char *filename, QDict *options,
/* are we a UNIX or TCP socket? */
if (strstart(host_spec, "unix:", &unixpath)) {
qdict_put(options, "server.type", qstring_from_str("unix"));
- qdict_put(options, "server.data.path", qstring_from_str(unixpath));
+ qdict_put(options, "server.path", qstring_from_str(unixpath));
} else {
InetSocketAddress *addr = NULL;
@@ -207,8 +208,8 @@ static void nbd_parse_filename(const char *filename, QDict *options,
}
qdict_put(options, "server.type", qstring_from_str("inet"));
- qdict_put(options, "server.data.host", qstring_from_str(addr->host));
- qdict_put(options, "server.data.port", qstring_from_str(addr->port));
+ qdict_put(options, "server.host", qstring_from_str(addr->host));
+ qdict_put(options, "server.port", qstring_from_str(addr->port));
qapi_free_InetSocketAddress(addr);
}
@@ -223,17 +224,45 @@ static bool nbd_process_legacy_socket_options(QDict *output_options,
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");
+ const char *sd_path = qemu_opt_get(legacy_opts, "server.data.path");
+ const char *sd_host = qemu_opt_get(legacy_opts, "server.data.host");
+ const char *sd_port = qemu_opt_get(legacy_opts, "server.data.port");
+ bool no_server = path || host || port;
+ bool server_data = sd_path || sd_host || sd_port;
const QDictEntry *e;
- if (!path && !host && !port) {
+ if (!no_server && !server_data) {
+ return true;
+ }
+
+ if (no_server && server_data) {
+ error_setg(errp, "Cannot use %s and %s at the same time",
+ "path/host/port", "server.data.*");
+ return false;
+ }
+
+ if (server_data) {
+ if (sd_host) {
+ qdict_put(output_options, "server.host",
+ qstring_from_str(sd_host));
+ }
+ if (sd_port) {
+ qdict_put(output_options, "server.port",
+ qstring_from_str(sd_port));
+ }
+ if (sd_path) {
+ qdict_put(output_options, "server.path",
+ qstring_from_str(sd_path));
+ }
return true;
}
for (e = qdict_first(output_options); e; e = qdict_next(output_options, e))
{
if (strstart(e->key, "server.", NULL)) {
- error_setg(errp, "Cannot use 'server' and path/host/port at the "
- "same time");
+ error_setg(errp, "Cannot use %s and %s at the same time",
+ e->key,
+ no_server ? "path/host/port" : "server.data.*");
return false;
}
}
@@ -248,20 +277,21 @@ static bool nbd_process_legacy_socket_options(QDict *output_options,
}
qdict_put(output_options, "server.type", qstring_from_str("unix"));
- qdict_put(output_options, "server.data.path", qstring_from_str(path));
+ qdict_put(output_options, "server.path", qstring_from_str(path));
} else if (host) {
qdict_put(output_options, "server.type", qstring_from_str("inet"));
- qdict_put(output_options, "server.data.host", qstring_from_str(host));
- qdict_put(output_options, "server.data.port",
+ qdict_put(output_options, "server.host", qstring_from_str(host));
+ qdict_put(output_options, "server.port",
qstring_from_str(port ?: stringify(NBD_DEFAULT_PORT)));
}
return true;
}
-static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, Error **errp)
+static SocketAddressFlat *nbd_config(BDRVNBDState *s, QDict *options,
+ Error **errp)
{
- SocketAddress *saddr = NULL;
+ SocketAddressFlat *saddr = NULL;
QDict *addr = NULL;
QObject *crumpled_addr = NULL;
Visitor *iv = NULL;
@@ -287,7 +317,7 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict *options, Error **errp)
* visitor expects the former.
*/
iv = qobject_input_visitor_new(crumpled_addr);
- visit_type_SocketAddress(iv, NULL, &saddr, &local_err);
+ visit_type_SocketAddressFlat(iv, NULL, &saddr, &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto done;
@@ -306,9 +336,38 @@ NBDClientSession *nbd_get_client_session(BlockDriverState *bs)
return &s->client;
}
-static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr,
+static SocketAddress *socket_address_crumple(SocketAddressFlat *saddr_flat)
+{
+ SocketAddress *saddr = g_new(SocketAddress, 1);
+
+ saddr->type = saddr_flat->type;
+ switch (saddr->type) {
+ case SOCKET_ADDRESS_FLAT_TYPE_INET:
+ saddr->u.inet.data = QAPI_CLONE(InetSocketAddress,
+ &saddr_flat->u.inet);
+ break;
+ case SOCKET_ADDRESS_FLAT_TYPE_UNIX:
+ saddr->u.q_unix.data = QAPI_CLONE(UnixSocketAddress, &
+ saddr_flat->u.q_unix);
+ break;
+ case SOCKET_ADDRESS_FLAT_TYPE_VSOCK:
+ saddr->u.vsock.data = QAPI_CLONE(VsockSocketAddress,
+ &saddr_flat->u.vsock);
+ break;
+ case SOCKET_ADDRESS_FLAT_TYPE_FD:
+ saddr->u.fd.data = QAPI_CLONE(String, &saddr_flat->u.fd);
+ break;
+ default:
+ abort();
+ }
+
+ return saddr;
+}
+
+static QIOChannelSocket *nbd_establish_connection(SocketAddressFlat *saddr_flat,
Error **errp)
{
+ SocketAddress *saddr = socket_address_crumple(saddr_flat);
QIOChannelSocket *sioc;
Error *local_err = NULL;
@@ -318,6 +377,7 @@ static QIOChannelSocket *nbd_establish_connection(SocketAddress *saddr,
qio_channel_socket_connect_sync(sioc,
saddr,
&local_err);
+ qapi_free_SocketAddress(saddr);
if (local_err) {
error_propagate(errp, local_err);
return NULL;
@@ -379,6 +439,21 @@ static QemuOptsList nbd_runtime_opts = {
.help = "Unix socket path to connect to",
},
{
+ .name = "server.data.host",
+ .type = QEMU_OPT_STRING,
+ .help = "TCP host to connect to",
+ },
+ {
+ .name = "server.data.port",
+ .type = QEMU_OPT_STRING,
+ .help = "TCP port to connect to",
+ },
+ {
+ .name = "server.data.path",
+ .type = QEMU_OPT_STRING,
+ .help = "Unix socket path to connect to",
+ },
+ {
.name = "export",
.type = QEMU_OPT_STRING,
.help = "Name of the NBD export to open",
@@ -409,7 +484,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
goto error;
}
- /* Translate @host, @port, and @path to a SocketAddress */
+ /* Translate @host, @port, and @path to a SocketAddressFlat */
if (!nbd_process_legacy_socket_options(options, opts, errp)) {
goto error;
}
@@ -430,11 +505,11 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
}
/* TODO SOCKET_ADDRESS_KIND_FD where fd has AF_INET or AF_INET6 */
- if (s->saddr->type != SOCKET_ADDRESS_KIND_INET) {
+ if (s->saddr->type != SOCKET_ADDRESS_FLAT_TYPE_INET) {
error_setg(errp, "TLS only supported over IP sockets");
goto error;
}
- hostname = s->saddr->u.inet.data->host;
+ hostname = s->saddr->u.inet.host;
}
/* establish TCP connection, return error if it fails
@@ -457,7 +532,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
object_unref(OBJECT(tlscreds));
}
if (ret < 0) {
- qapi_free_SocketAddress(s->saddr);
+ qapi_free_SocketAddressFlat(s->saddr);
g_free(s->export);
g_free(s->tlscredsid);
}
@@ -483,7 +558,7 @@ static void nbd_close(BlockDriverState *bs)
nbd_client_close(bs);
- qapi_free_SocketAddress(s->saddr);
+ qapi_free_SocketAddressFlat(s->saddr);
g_free(s->export);
g_free(s->tlscredsid);
}
@@ -514,15 +589,15 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
Visitor *ov;
const char *host = NULL, *port = NULL, *path = NULL;
- if (s->saddr->type == SOCKET_ADDRESS_KIND_INET) {
- const InetSocketAddress *inet = s->saddr->u.inet.data;
+ if (s->saddr->type == SOCKET_ADDRESS_FLAT_TYPE_INET) {
+ const InetSocketAddress *inet = &s->saddr->u.inet;
if (!inet->has_ipv4 && !inet->has_ipv6 && !inet->has_to) {
host = inet->host;
port = inet->port;
}
- } else if (s->saddr->type == SOCKET_ADDRESS_KIND_UNIX) {
- path = s->saddr->u.q_unix.data->path;
- }
+ } else if (s->saddr->type == SOCKET_ADDRESS_FLAT_TYPE_UNIX) {
+ path = s->saddr->u.q_unix.path;
+ } /* else can't represent as pseudo-filename */
qdict_put(opts, "driver", qstring_from_str("nbd"));
@@ -541,7 +616,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
}
ov = qobject_output_visitor_new(&saddr_qdict);
- visit_type_SocketAddress(ov, NULL, &s->saddr, &error_abort);
+ visit_type_SocketAddressFlat(ov, NULL, &s->saddr, &error_abort);
visit_complete(ov, &saddr_qdict);
visit_free(ov);
qdict_put_obj(opts, "server", saddr_qdict);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4e8e4e3..8d87962 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2762,7 +2762,7 @@
# Since: 2.9
##
{ 'struct': 'BlockdevOptionsNbd',
- 'data': { 'server': 'SocketAddress',
+ 'data': { 'server': 'SocketAddressFlat',
'*export': 'str',
'*tls-creds': 'str' } }
--
2.7.4
On 29/03/2017 18:45, Markus Armbruster wrote: > Unfortunately, SocketAddress is also visible in -drive since 2.8. Add > still more gunk to nbd_process_legacy_socket_options(). You can now > shorten > > -drive if=none,driver=nbd,server.type=inet,server.data.host=127.0.0.1,server.data.port=12345 > > to > > -drive if=none,driver=nbd,server.type=inet,server.host=127.0.0.1,server.port=12345 I think it's reasonable to assume nobody has used this and break it. Paolo
On 29.03.2017 18:45, Markus Armbruster wrote:
> SocketAddress is a simple union, and simple unions are awkward: they
> have their variant members wrapped in a "data" object on the wire, and
> require additional indirections in C. I intend to limit its use to
> existing external interfaces, and convert all internal interfaces to
> SocketAddressFlat.
>
> BlockdevOptionsNbd is an external interface using SocketAddress, but
> it's new in 2.9. Replace it by SocketAddressFlat while we can. This
> simplifies
>
> { "execute": "blockdev-add",
> "arguments": { "node-name": "foo", "driver": "nbd",
> "server": { "type": "inet",
> "data": { "host": "localhost",
> "port": "12345" } } } }
>
> to
>
> { "execute": "blockdev-add",
> "arguments": { "node-name": "foo", "driver": "nbd",
> "server": { "type": "inet",
> "host": "localhost", "port": "12345" } } }
>
> Since the internal interfaces still take SocketAddress, this requires
> conversion function socket_address_crumple(). It'll go away when I
> update the interfaces.
>
> Unfortunately, SocketAddress is also visible in -drive since 2.8. Add
> still more gunk to nbd_process_legacy_socket_options(). You can now
> shorten
>
> -drive if=none,driver=nbd,server.type=inet,server.data.host=127.0.0.1,server.data.port=12345
>
> to
>
> -drive if=none,driver=nbd,server.type=inet,server.host=127.0.0.1,server.port=12345
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/nbd.c | 131 ++++++++++++++++++++++++++++++++++++++++-----------
> qapi/block-core.json | 2 +-
> 2 files changed, 104 insertions(+), 29 deletions(-)
To be completely frank: If there is any drawback to using
SocketAddressFlat instead of SocketAdress, I would be against using it.
Less indirections in C is an argument, but less {} on the wire is not,
in my opinion. QMP is a machine interface and machines don't care about
that additional level.
Therefore, if you think this somehow affects compatibility, I would not
want to do this change.
I personally agree with Paolo that we can just break what we have, and
in addition I think that we *should* break what we have or we should not
do this change at all.
Max
On 03/29/2017 11:45 AM, Markus Armbruster wrote:
> SocketAddress is a simple union, and simple unions are awkward: they
> have their variant members wrapped in a "data" object on the wire, and
> require additional indirections in C. I intend to limit its use to
> existing external interfaces, and convert all internal interfaces to
> SocketAddressFlat.
>
> BlockdevOptionsNbd is an external interface using SocketAddress, but
> it's new in 2.9. Replace it by SocketAddressFlat while we can. This
> simplifies
>
> { "execute": "blockdev-add",
> "arguments": { "node-name": "foo", "driver": "nbd",
> "server": { "type": "inet",
> "data": { "host": "localhost",
> "port": "12345" } } } }
>
> to
>
> { "execute": "blockdev-add",
> "arguments": { "node-name": "foo", "driver": "nbd",
> "server": { "type": "inet",
> "host": "localhost", "port": "12345" } } }
This part makes sense, and means this has to go in 2.9.
>
> Since the internal interfaces still take SocketAddress, this requires
> conversion function socket_address_crumple(). It'll go away when I
> update the interfaces.
This is probably the fastest way forward for 2.9, even if we rip it out
later in 2.10.
>
> Unfortunately, SocketAddress is also visible in -drive since 2.8. Add
> still more gunk to nbd_process_legacy_socket_options(). You can now
> shorten
>
> -drive if=none,driver=nbd,server.type=inet,server.data.host=127.0.0.1,server.data.port=12345
>
> to
>
> -drive if=none,driver=nbd,server.type=inet,server.host=127.0.0.1,server.port=12345
Here, I'm 50/50 on whether the compatibility gunk is worth it, or
whether to make a clean break of old configurations, as I'm not sure who
is using the old configurations. But given the lateness of the change
(and my recent case of being burned on qom-type during freeze when I was
not conservative), I agree with your conservative approach of remaining
compatible, even if it means the patch is larger than desired, and even
if we rip it out again in 2.10.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> block/nbd.c | 131 ++++++++++++++++++++++++++++++++++++++++-----------
> qapi/block-core.json | 2 +-
> 2 files changed, 104 insertions(+), 29 deletions(-)
>
> @@ -223,17 +224,45 @@ static bool nbd_process_legacy_socket_options(QDict *output_options,
> 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");
> + const char *sd_path = qemu_opt_get(legacy_opts, "server.data.path");
> + const char *sd_host = qemu_opt_get(legacy_opts, "server.data.host");
> + const char *sd_port = qemu_opt_get(legacy_opts, "server.data.port");
> + bool no_server = path || host || port;
> + bool server_data = sd_path || sd_host || sd_port;
> const QDictEntry *e;
>
> - if (!path && !host && !port) {
> + if (!no_server && !server_data) {
Feels like a double negative, although it's not really serving that
role. Maybe a better name would be s/no_server/bare/, so that 'bare' is
now your counterpart to 'server_data'.
> + return true;
> + }
> +
> + if (no_server && server_data) {
> + error_setg(errp, "Cannot use %s and %s at the same time",
> + "path/host/port", "server.data.*");
> + return false;
Again, 'bare' makes this conditional read a bit better.
> + }
> +
> + if (server_data) {
> + if (sd_host) {
> + qdict_put(output_options, "server.host",
> + qstring_from_str(sd_host));
> + }
> + if (sd_port) {
> + qdict_put(output_options, "server.port",
> + qstring_from_str(sd_port));
> + }
> + if (sd_path) {
> + qdict_put(output_options, "server.path",
> + qstring_from_str(sd_path));
> + }
Wow, I need to rebase my qdict_put_str() stuff again - that ought to be
something we include early in 2.10, as it touches a lot of stuff.
Doesn't affect your patch for now, though.
Renaming the local variable is trivial, so whether or not you
incorporate my naming suggestion,
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
© 2016 - 2026 Red Hat, Inc.