[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [virt-tools-list] [virt-viewer] Add oVirt support



On Fri, Jan 25, 2013 at 04:13:35PM +0100, Marc-André Lureau wrote:
> Hi
> 
> it looks good me,
> 
> On Thu, Jan 24, 2013 at 4:57 PM, Christophe Fergeau <cfergeau redhat com> wrote:
> > @@ -240,3 +256,5 @@ AC_MSG_NOTICE([     LIBXML2: $LIBXML2_CFLAGS $LIBXML2_LIBS])
> >  AC_MSG_NOTICE([])
> >  AC_MSG_NOTICE([     LIBVIRT: $LIBVIRT_CFLAGS $LIBVIRT_LIBS])
> >  AC_MSG_NOTICE([])
> > +AC_MSG_NOTICE([     LIBREST: $OVIRT_CFLAGS $OVIRT_LIBS])
> 
> LIBREST -> OVIRT

Changed, thanks

> 
> > +AC_MSG_NOTICE([])
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 05e20b2..7bb03cb 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -59,6 +59,11 @@ COMMON_SOURCES +=                                            \
> >         $(NULL)
> >  endif
> >
> > +if HAVE_OVIRT
> > +COMMON_SOURCES +=                                              \
> > +       $(NULL)
> > +endif
> 
> leftover?

Yup, totally, removed.

> 
> >  if HAVE_SPICE_GTK
> >  COMMON_SOURCES +=                                              \
> >         virt-viewer-session-spice.h virt-viewer-session-spice.c \
> > @@ -96,6 +101,10 @@ if HAVE_GTK_VNC
> >  virt_viewer_LDFLAGS += $(GTK_VNC_LIBS)
> >  virt_viewer_CFLAGS += $(GTK_VNC_CFLAGS)
> >  endif
> > +if HAVE_OVIRT
> > +virt_viewer_LDFLAGS += $(OVIRT_LIBS)
> > +virt_viewer_CFLAGS += $(OVIRT_CFLAGS)
> > +endif
> >  if HAVE_SPICE_GTK
> >  virt_viewer_LDFLAGS += $(SPICE_GTK_LIBS)
> >  virt_viewer_CFLAGS += $(SPICE_GTK_CFLAGS)
> > @@ -128,6 +137,10 @@ if HAVE_GTK_VNC
> >  remote_viewer_LDFLAGS += $(GTK_VNC_LIBS)
> >  remote_viewer_CFLAGS += $(GTK_VNC_CFLAGS)
> >  endif
> > +if HAVE_OVIRT
> > +remote_viewer_LDFLAGS += $(OVIRT_LIBS)
> > +remote_viewer_CFLAGS += $(OVIRT_CFLAGS)
> > +endif
> 
> That makes me realize that we shouldn't need those conditions for the
> X_{LIBS,CFLAGS}. A future cleanup perhaps.

I'll send a followup patch

> 
> > +#ifdef HAVE_OVIRT
> > +static gboolean
> > +parse_uri(const gchar *uri, char **rest_uri, char **name)
> 
> nitpick: it would help to prefix ovirt specific functions
> consistantly, although ovirt_ is already taken by govirt. At least
> rename to parse_ovirt_uri() would be nice.

I've renamed it to parse_ovirt_uri()

> 
> > +{
> > +    char *pos;
> > +    char *vm_name;
> > +    char *tmp_uri;
> > +
> > +    g_return_val_if_fail(uri != NULL, FALSE);
> > +    g_return_val_if_fail(rest_uri != NULL, FALSE);
> > +    g_return_val_if_fail(name != NULL, FALSE);
> > +
> > +    /* extract VM name from URI */
> > +    pos = strrchr(uri, '/');
> > +    if (pos == NULL)
> > +        return FALSE;
> > +
> > +    pos++;
> > +    if (*pos == '\0')
> > +        return FALSE;
> > +    vm_name = pos;
> > +
> > +    /* remove vm_name and trailing / from uri */
> > +    pos = pos - 1;
> > +    g_assert(*pos == '/');
> > +
> > +    while ((pos != uri) && (*pos == '/'))
> > +        pos--;
> > +    if (pos == uri)
> > +        return FALSE;
> > +    if (pos - uri + 1 < strlen("ovirt://")) {
> > +        /* we trimmed too much */
> > +        return FALSE;
> > +    }
> > +    tmp_uri = g_strndup(uri, pos - uri + 1);
> > +    g_assert(g_str_has_prefix(tmp_uri, "ovirt://"));
> > +    /* FIXME: how to decide between http and https? */
> 
> Not a big deal, but since we use xmlParseURI() already, you could use that too.

I've switched to xmlParseURI, but this does not make the code much shorter.
Maybe a bit more readable.

> 
> > +    *rest_uri = g_strdup_printf("https://%s/api/";,
> > +                                tmp_uri + strlen("ovirt://"));
> > +    *name = g_strdup(vm_name);
> > +    g_free(tmp_uri);
> > +
> > +    g_message("oVirt base URI: %s", *rest_uri);
> > +    g_message("oVirt VM name: %s", *name);
> 
> perhaps debug is more appropriate?

Yeah, forgot to switch it back to debug.

> 
> > +static gboolean
> > +create_ovirt_session(VirtViewerApp *app, const char *uri)
> > +{
> > +    OvirtProxy *proxy = NULL;
> > +    OvirtVm *vm = NULL;
> > +    OvirtVmDisplay *display = NULL;
> > +    OvirtVmState state;
> > +    GError *error = NULL;
> > +    char *rest_uri = NULL;
> > +    char *vm_name = NULL;
> > +    gboolean success = FALSE;
> > +    guint port;
> > +    guint secure_port;
> > +    OvirtVmDisplayType type;
> > +    const char *session_type;
> > +
> > +    gchar *gport = NULL;
> > +    gchar *gtlsport = NULL;
> > +    gchar *ghost = NULL;
> > +    gchar *ticket = NULL;
> > +
> > +    g_return_val_if_fail(VIRT_VIEWER_IS_APP(app), FALSE);
> > +
> > +    if (!parse_uri(uri, &rest_uri, &vm_name))
> > +        goto error;
> > +    proxy = ovirt_proxy_new (rest_uri);
> 
> nitpick, extra space

Removed.

> 
> > +    if (proxy == NULL)
> > +        goto error;
> > +    g_signal_connect(G_OBJECT(proxy), "authenticate",
> > +                     G_CALLBACK(authenticate_cb), app);
> > +
> > +    ovirt_proxy_fetch_ca_certificate(proxy, &error);
> > +    if (error != NULL) {
> > +        g_debug("failed to get CA certificate: %s", error->message);
> > +        goto error;
> > +    }
> > +
> > +    ovirt_proxy_fetch_vms(proxy, &error);
> > +    if (error != NULL) {
> > +        g_debug("failed to lookup %s: %s", vm_name, error->message);
> > +        goto error;
> > +    }
> 
> That could use async calls, it's a UI after all.

Yeah, though making start() async is a bit more involved, and the existing
code is doing sync calls, and no UI is shown at this point iirc, so I went
the lazy way

> 
> > +    vm = ovirt_proxy_lookup_vm(proxy, vm_name);
> > +    g_return_val_if_fail(vm != NULL, FALSE);
> 
> So if the given VM name doesn't exist, it just goes to g_return_val_if_fail?
> 
> That could use a virt_viewer_app_simple_message_dialog()

The calling code is taking care of that already:
if (!create_ovirt_session(app, guri)) {
    virt_viewer_app_simple_message_dialog(app, _("Couldn't open oVirt session"));
    goto cleanup;
}

> > +    if (type == OVIRT_VM_DISPLAY_SPICE) {
> > +#if HAVE_SPICE_GTK
> > +        session_type = "spice";
> > +#else
> > +        g_debug("This binary was compiled without SPICE support");
> > +        goto error;
> > +#endif
> > +    } else if (type == OVIRT_VM_DISPLAY_VNC) {
> > +#if HAVE_GTK_VNC
> > +        session_type = "vnc";
> > +#else
> > +        g_debug("This binary was compiled without VNC support");
> > +        goto error;
> > +#endif
> > +    } else {
> > +        g_debug("Unknown display type: %d", type);
> > +        goto error;
> > +    }
> 
> Check is not needed, or it would be better to improve
> virt_viewer_app_create_session()

You're right, virt_viewer_app_create_session is already taking care of
that, I've removed the #ifdef checks.

Christophe

Attachment: pgpdQQQ07bHce.pgp
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]