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

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



Hi

it looks good me,

On Thu, Jan 24, 2013 at 4:57 PM, Christophe Fergeau <cfergeau redhat com> wrote:
> This commit adds support for ovirt:// URIs. It does so by using
> libgovirt to get the spice/vnc connection information through
> oVirt xmlrpc API.
> ---
>  configure.ac        |  20 ++++-
>  src/Makefile.am     |  13 +++
>  src/remote-viewer.c | 233 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 261 insertions(+), 5 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 251b134..d832769 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -19,7 +19,7 @@ GTK2_REQUIRED="2.18.0"
>  GTK3_REQUIRED="3.0"
>  GTK_VNC1_REQUIRED="0.3.8"
>  GTK_VNC2_REQUIRED="0.4.0"
> -SPICE_GTK_REQUIRED="0.12.101"
> +SPICE_GTK_REQUIRED="0.15"
>  SPICE_PROTOCOL_REQUIRED="0.10.1"
>
>  AC_MSG_CHECKING([for native Win32])
> @@ -165,6 +165,22 @@ AS_IF([test "x$have_spice_gtk" = "xyes"],
>  ])
>  AM_CONDITIONAL([HAVE_SPICE_GTK], [test "x$have_spice_gtk" = "xyes"])
>
> +AC_ARG_WITH([ovirt],
> +    AS_HELP_STRING([--without-ovirt], [Ignore presence of librest and disable oVirt support]))
> +
> +AS_IF([test "x$with_ovirt" != "xno"],
> +      [PKG_CHECK_MODULES([OVIRT], [govirt-1.0],
> +                         [have_ovirt=yes], [have_ovirt=no])],
> +      [have_ovirt=no])
> +
> +AS_IF([test "x$have_ovirt" = "xyes"],
> +      [AC_DEFINE([HAVE_OVIRT], 1, [Have libgovirt?])],
> +      [AS_IF([test "x$with_ovirt" = "xyes"],
> +             [AC_MSG_ERROR([oVirt support requested but libgovirt not found])
> +      ])
> +])
> +AM_CONDITIONAL([HAVE_OVIRT], [test "x$have_ovirt" = "xyes"])
> +
>  dnl Decide if this platform can support the SSH tunnel feature.
>  AC_CHECK_HEADERS([sys/socket.h sys/un.h windows.h])
>  AC_CHECK_FUNCS([fork socketpair])
> @@ -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

> +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?

>  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.

> +#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.

> +{
> +    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.

> +    *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?

> +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

> +    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.

> +    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()

> +    g_object_get(G_OBJECT(vm), "state", &state, NULL);
> +    if (state != OVIRT_VM_STATE_UP) {
> +        g_debug("oVirt VM %s is not running", vm_name);
> +        goto error;
> +    }
> +
> +    if (!ovirt_vm_get_ticket(vm, proxy, &error)) {
> +        g_debug("failed to get ticket for %s: %s", vm_name, error->message);
> +        goto error;
> +    }
> +
> +    g_object_get(G_OBJECT(vm), "display", &display, NULL);
> +    if (display == NULL) {
> +        goto error;
> +    }
> +
> +    g_object_get(G_OBJECT(display),
> +                 "type", &type,
> +                 "address", &ghost,
> +                 "port", &port,
> +                 "secure-port", &secure_port,
> +                 "ticket", &ticket,
> +                 NULL);
> +    gport = g_strdup_printf("%d", port);
> +    gtlsport = g_strdup_printf("%d", secure_port);
> +
> +    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()

> +    virt_viewer_app_set_connect_info(app, NULL, ghost, gport, gtlsport,
> +                                     session_type, NULL, NULL, 0, NULL);
> +
> +    if (virt_viewer_app_create_session(app, session_type) < 0)
> +        goto error;
> +
> +#if HAVE_SPICE_GTK
> +    if (type == OVIRT_VM_DISPLAY_SPICE) {
> +        SpiceSession *session;
> +        GByteArray *ca_cert;
> +
> +        g_object_get(G_OBJECT(proxy), "ca-cert", &ca_cert, NULL);
> +        session = remote_viewer_get_spice_session(REMOTE_VIEWER(app));
> +        g_object_set(G_OBJECT(session),
> +                     "ca", ca_cert,
> +                     "password", ticket,
> +                     NULL);
> +        g_byte_array_unref(ca_cert);
> +    }
> +#endif
> +
> +    success = TRUE;
> +
> +error:
> +    g_free(rest_uri);
> +    g_free(vm_name);
> +    g_free(ticket);
> +    g_free(gport);
> +    g_free(gtlsport);
> +    g_free(ghost);
> +
> +    if (error != NULL)
> +        g_error_free(error);
> +    if (display != NULL)
> +        g_object_unref(display);
> +    if (vm != NULL)
> +        g_object_unref(vm);
> +    if (proxy != NULL)
> +        g_object_unref(proxy);
> +
> +    return success;
> +}
> +
> +#endif
>




-- 
Marc-André Lureau


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