Re: [Qemu-devel] [PATCH 6/7] sockets: Limit SocketAddressLegacy except to external interfaces

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6078 bytes --]

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 <armbru@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 <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]