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@xxxxxxxxxx> 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:
pgp6QanxITU5H.pgp
Description: PGP signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
