[virt-tools-list] [virt-viewer] virt-viewer: Don't connect to localhost displays with qemu+tcp://

[Jonathon Jongsma]

Hi

On Tue, 2014-06-24 at 09:49 +0200, Christophe Fergeau wrote:
> Ping ?
> 
> On Thu, Jun 19, 2014 at 06:32:40PM +0200, Christophe Fergeau wrote:
> > When connecting to a remote libvirt instance, a VM may only be listening
> > on localhost for SPICE/VNC connections. In such a situation, virt-viewer
> > then tries to connect to localhost, which is not correct as this
> > 'localhost' referred to the remote libvirt host it connected to.
> > This commit adds a couple of tests on the libvirt URI used and the
> > <graphics> listen address to error out in this situation.
> > 
> > Resolves: rhbz#1108523
> > ---
> >  src/virt-viewer.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> > 
> > diff --git a/src/virt-viewer.c b/src/virt-viewer.c
> > index 8a1d8c5..f8f175c 100644
> > --- a/src/virt-viewer.c
> > +++ b/src/virt-viewer.c
> > @@ -292,6 +292,55 @@ virt_viewer_replace_host(const gchar *host)
> >      return ret;
> >  }
> >  
> > +
> > +static gboolean
> > +virt_viewer_is_loopback(const char *host)
> > +{
> > +    GInetAddress *addr = NULL;
> > +    gboolean is_loopback = FALSE;
> > +
> > +    g_return_val_if_fail(host != NULL, FALSE);
> > +
> > +    addr = g_inet_address_new_from_string(host);
> > +    if (!addr) /* Parsing error means it was probably a hostname */
> > +        return (strcmp(host, "localhost") == 0);
> > +
> > +    is_loopback = g_inet_address_get_is_loopback(addr);
> > +    g_object_unref(addr);
> > +
> > +    return is_loopback;
> > +}
> > +
> > +
> > +static gboolean
> > +virt_viewer_is_reachable(const gchar *host, const char *transport,
> > +                         const char *transport_host)
> > +{
> > +    gboolean host_is_loopback;
> > +    gboolean transport_is_loopback;
> > +
> > +    if (!host)
> > +        return FALSE;
> > +
> > +    if (!transport)
> > +        return TRUE;
> > +
> > +    if (strcmp(transport, "ssh") == 0)
> > +        return TRUE;
> > +
> > +    if (strcmp(transport, "unix") == 0)
> > +        return TRUE;
> > +
> > +    host_is_loopback = virt_viewer_is_loopback(host);
> > +    transport_is_loopback = virt_viewer_is_loopback(transport_host);
> > +
> > +    if (transport_is_loopback && host_is_loopback)
> > +        return TRUE;
> > +    else
> > +        return !host_is_loopback;
> > +}
> > +
> > +
> >  static gboolean
> >  virt_viewer_extract_connect_info(VirtViewer *self,
> >                                   virDomainPtr dom)
> > @@ -377,6 +426,14 @@ virt_viewer_extract_connect_info(VirtViewer *self,
> >          ghost = replacement_host;
> >      }
> >  
> > +    if (!virt_viewer_is_reachable(ghost, transport, host)) {
> > +        g_debug("graphics listen '%s' is not reachable from this machine",
> > +                ghost ? ghost : "");
> > +        virt_viewer_app_simple_message_dialog(app, _("Guest '%s' is not reachable"),
> > +                                              priv->domkey);
> > +        goto cleanup;
> > +    }
> > +
> >      virt_viewer_app_set_connect_info(app, host, ghost, gport, gtlsport,transport, unixsock, user, port, NULL);
> >  
> >      retval = TRUE;
> > -- 
> > 1.9.3
> > 


It seems that this would fix the vast majority of cases, but it also may
be a bit too simplistic? For example, if the 'listen' address was some
string that was not literally "localhost" but still mapped to the
loopback address, virt_viewer_is_loopback() would return FALSE. This
seems like an improbable scenario, and maybe we don't care about it,
though. Otherwise it looks fine to me.

Jonathon