On 04/26/2017 02:36 AM, Markus Armbruster wrote: I think it reads fine if you shorten the subject via s/except //
> SocketAddressLegacy 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. SocketAddress is the
> equivalent flat union. Convert all users of SocketAddressLegacy to
> SocketAddress, except for existing external interfaces.
>
> See also commit fce5d53..9445673 and 85a82e8..c5f1ae3.
>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> ---
> +++ b/blockdev-nbd.c
> @@ -99,9 +99,8 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id,
> Error **errp)
> }
>
>
> -void qmp_nbd_server_start(SocketAddressLegacy *addr,
> - bool has_tls_creds, const char *tls_creds,
> - Error **errp)
> +void nbd_server_start(SocketAddress *addr, const char *tls_creds,
> + Error **errp)
> {
...
> @@ -145,6 +144,16 @@ void qmp_nbd_server_start(SocketAddressLegacy *addr,
> nbd_server = NULL;
> }
>
> +void qmp_nbd_server_start(SocketAddressLegacy *addr,
> + bool has_tls_creds, const char *tls_creds,
> + Error **errp)
> +{
> + SocketAddress *addr_flat = socket_address_flatten(addr);
> +
> + nbd_server_start(addr_flat, tls_creds, errp);
We didn't always guarantee that tls_creds was initialized when
has_tls_creds was false, but it's been quite some time that we fixed
that. So what you have works, and I'm not sure if it's worth using the
longer:
has_tls_creds ? tls_creds : NULL
> +++ b/chardev/char-socket.c
> @@ -52,7 +52,7 @@ typedef struct {
> int *write_msgfds;
> size_t write_msgfds_num;
>
> - SocketAddressLegacy *addr;
> + SocketAddress *addr;
> bool is_listen;
> bool is_telnet;
>
> @@ -337,30 +337,30 @@ static void tcp_chr_free_connection(Chardev *chr)
> s->connected = 0;
> }
>
> -static char *SocketAddress_to_str(const char *prefix, SocketAddressLegacy
> *addr,
> +static char *SocketAddressto_str(const char *prefix, SocketAddress *addr,
> bool is_listen, bool is_telnet)
Is this rename(s/_to/to/) intentional? If it is, your indentation is now
off. If not, you've got a couple of fixes to make to restore the _.
> @@ -380,7 +380,7 @@ static void tcp_chr_disconnect(Chardev *chr)
> s->listen_tag = qio_channel_add_watch(
> QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL);
> }
> - chr->filename = SocketAddress_to_str("disconnected:", s->addr,
> + chr->filename = SocketAddressto_str("disconnected:", s->addr,
> s->is_listen, s->is_telnet);
Again, either restore the _ or fix the indentation.
> @@ -864,18 +864,18 @@ static void qmp_chardev_open_socket(Chardev *chr,
> }
> }
>
> - s->addr = QAPI_CLONE(SocketAddressLegacy, sock->addr);
> + s->addr = addr = socket_address_flatten(sock->addr);
>
> qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_RECONNECTABLE);
> /* TODO SOCKET_ADDRESS_FD where fd has AF_UNIX */
> - if (addr->type == SOCKET_ADDRESS_LEGACY_KIND_UNIX) {
> + if (addr->type == SOCKET_ADDRESS_TYPE_UNIX) {
> qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS);
> }
>
> /* be isn't opened until we get a connection */
> *be_opened = false;
>
> - chr->filename = SocketAddress_to_str("disconnected:",
> + chr->filename = SocketAddressto_str("disconnected:",
> addr, is_listen, is_telnet);
and again
> @@ -131,4 +131,15 @@ char *socket_address_to_string(struct
> SocketAddressLegacy *addr, Error **errp);
> */
> SocketAddressLegacy *socket_address_crumple(SocketAddress *addr);
>
> +/**
> + * socket_address_flatten:
> + * @addr: the socket address to flatten
> + *
> + * Convert SocketAddressLegacy to SocketAddress. Caller is responsible
> + * for freeing with qapi_free_SocketAddress().
> + *
> + * Returns: the argument converted to SocketAddressLegacy.
s/SocketAddressLegacy/SocketAddress/
> + */
> +SocketAddress *socket_address_flatten(SocketAddressLegacy *addr);
> +
> #endif /* QEMU_SOCKETS_H */
> +++ b/qapi-schema.json
> @@ -4126,7 +4126,12 @@
> #
> # Captures the address of a socket, which could also be a named file
> descriptor
> #
> -# Note: This type is deprecated in favor of SocketAddress.
> +# Note: This type is deprecated in favor of SocketAddress. The
> +# difference to SocketAddress is that SocketAddress is a flat union
> +# rather than a simple union. Flat is nicer because it avoids nesting
> +# on the wire, i.e. this form has fewer {}.
This comment doesn't read correctly. I think you want:
The difference between SocketAddressLegacy and SocketAddress is that the
latter is a flat union rather than a simple union. Flat is nicer because
it avoids nesting on the wire, i.e. that form has fewer {}.
> +++ b/util/qemu-sockets.c
> @@ -1371,3 +1366,34 @@ SocketAddressLegacy
> *socket_address_crumple(SocketAddress *addr_flat)
>
> return addr;
> }
> +
> +SocketAddress *socket_address_flatten(SocketAddressLegacy *addr_legacy)
> +{
> + SocketAddress *addr = g_new(SocketAddress, 1);
> +
> + switch (addr_legacy->type) {
> + case SOCKET_ADDRESS_LEGACY_KIND_INET:
> + addr->type = SOCKET_ADDRESS_TYPE_INET;
> + QAPI_CLONE_MEMBERS(InetSocketAddress, &addr->u.inet,
> + addr_legacy->u.inet.data);
> + break;
Works nicely.
I've pointed out some minor issues, but if you are comfortable changing
those as maintainer rather than posting a v2, feel free to add:
Reviewed-by: Eric Blake <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
