Re: [Qemu-devel] [PATCH 5/8] ui: extract code to connect/listen from vnc_display_open

[Eric Blake]

On 01/05/2017 10:06 AM, Daniel P. Berrange wrote:
> The code which takes a SocketAddress and connects/listens on the
> network is going to get more complicated to deal with multiple
> listeners. Pull it out into a separate method to avoid making the
> vnc_display_open method even more complex.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  ui/vnc.c | 122 
> +++++++++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 80 insertions(+), 42 deletions(-)
> 

> +static int vnc_display_listen(VncDisplay *vd,
> +                              SocketAddress *saddr,
> +                              SocketAddress *wsaddr,
> +                              Error **errp)
> +{
> +    vd->is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
> +
> +    if (vnc_display_listen_addr(vd, saddr,
> +                                "vnc-listen",
> +                                &vd->lsock,
> +                                &vd->lsock_tag,
> +                                &vd->nlsock,
> +                                errp) < 0) {
> +        return -1;

If this succeeds,

> +    }
> +    if (wsaddr &&
> +        vnc_display_listen_addr(vd, wsaddr,
> +                                "vnc-ws-listen",
> +                                &vd->lwebsock,
> +                                &vd->lwebsock_tag,
> +                                &vd->nlwebsock,
> +                                errp) < 0) {
> +        return -1;

but this fails, you are relying on the caller to clean up the successful
port allocation.  Then again, in the old code:

> @@ -3909,53 +3987,13 @@ void vnc_display_open(const char *id, Error **errp)

>      } else {
> -        vd->nlsock = 1;
> -        vd->lsock = g_new0(QIOChannelSocket *, 1);
> -        vd->lsock_tag = g_new0(guint, 1);
> -
> -        vd->lsock[0] = qio_channel_socket_new();
> -        qio_channel_set_name(QIO_CHANNEL(vd->lsock[0]), "vnc-listen");
> -        if (qio_channel_socket_listen_sync(vd->lsock[0], saddr, errp) < 0) {
> +        if (vnc_display_listen(vd, saddr, wsaddr, errp) < 0) {
>              goto fail;
>          }
> -        vd->is_unix = saddr->type == SOCKET_ADDRESS_KIND_UNIX;
> -
> -        if (wsaddr) {
> -            vd->nlwebsock = 1;
> -            vd->lwebsock = g_new0(QIOChannelSocket *, 1);
> -            vd->lwebsock_tag = g_new0(guint, 1);
> -
> -            vd->lwebsock[0] = qio_channel_socket_new();
> -            qio_channel_set_name(QIO_CHANNEL(vd->lwebsock[0]), 
> "vnc-ws-listen");
> -            if (qio_channel_socket_listen_sync(vd->lwebsock[0],
> -                                               wsaddr, errp) < 0) {
> -                goto fail;
> -            }

this is all the more cleanup you do locally:

fail:
    qapi_free_SocketAddress(saddr);
    qapi_free_SocketAddress(wsaddr);
    ws_enabled = false;

so you already have cleanup at a distance when you have partial
allocation before failure (that is, the caller cleans up the
partially-constructed 'vd' object when errp is set).  So you aren't
making it worse.  Thus

Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature