[virt-tools-list] [PATCH virt-viewer 3/4] Allow app_initial_connect() to raise an error
Hans de Goede
hdegoede at redhat.com
Fri Mar 8 08:22:18 UTC 2013
Hi,
Some small nitpicks inline...
On 03/07/2013 08:55 PM, Marc-André Lureau wrote:
> ---
> src/remote-viewer.c | 31 ++++++++++++++++++-----------
> src/virt-viewer-app.c | 44 ++++++++++++++++++++---------------------
> src/virt-viewer-app.h | 8 ++++----
> src/virt-viewer-file.c | 2 ++
> src/virt-viewer-session-spice.c | 4 ++--
> src/virt-viewer-session-vnc.c | 5 +++--
> src/virt-viewer-session.c | 4 ++--
> src/virt-viewer-session.h | 4 ++--
> src/virt-viewer.c | 43 +++++++++++++++++++++++-----------------
> 9 files changed, 82 insertions(+), 63 deletions(-)
>
> diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> index fa17edd..216d0d8 100644
> --- a/src/remote-viewer.c
> +++ b/src/remote-viewer.c
> @@ -66,7 +66,7 @@ enum {
>
> static gboolean remote_viewer_start(VirtViewerApp *self);
> #if HAVE_SPICE_GTK
> -static int remote_viewer_activate(VirtViewerApp *self);
> +static gboolean remote_viewer_activate(VirtViewerApp *self, GError **error);
> static void remote_viewer_window_added(VirtViewerApp *self, VirtViewerWindow *win);
> static void spice_foreign_menu_updated(RemoteViewer *self);
> #endif
> @@ -236,8 +236,13 @@ static void
> spice_ctrl_do_connect(SpiceCtrlController *ctrl G_GNUC_UNUSED,
> VirtViewerApp *self)
> {
> - if (virt_viewer_app_initial_connect(self) < 0) {
> - virt_viewer_app_simple_message_dialog(self, _("Failed to initiate connection"));
> + GError *error = NULL;
> +
> + if (!virt_viewer_app_initial_connect(self, &error)) {
> + const gchar *msg = error ? error->message :
> + _("Failed to initiate connection");
> + virt_viewer_app_simple_message_dialog(self, msg);
> + g_clear_error(&error);
> }
> }
>
> @@ -558,19 +563,19 @@ spice_ctrl_listen_async_cb(GObject *object,
> }
>
>
> -static int
> -remote_viewer_activate(VirtViewerApp *app)
> +static gboolean
> +remote_viewer_activate(VirtViewerApp *app, GError **error)
> {
> - g_return_val_if_fail(REMOTE_VIEWER_IS(app), -1);
> + g_return_val_if_fail(REMOTE_VIEWER_IS(app), FALSE);
> RemoteViewer *self = REMOTE_VIEWER(app);
> - int ret = -1;
> + gboolean ret = FALSE;
>
> if (self->priv->controller) {
> SpiceSession *session = remote_viewer_get_spice_session(self);
> ret = spice_session_connect(session);
> g_object_unref(session);
> } else {
> - ret = VIRT_VIEWER_APP_CLASS(remote_viewer_parent_class)->activate(app);
> + ret = VIRT_VIEWER_APP_CLASS(remote_viewer_parent_class)->activate(app, error);
> }
>
> return ret;
> @@ -599,6 +604,7 @@ remote_viewer_start(VirtViewerApp *app)
> gboolean ret = FALSE;
> gchar *guri = NULL;
> gchar *type = NULL;
> + GError *error = NULL;
>
> #if HAVE_SPICE_GTK
> g_signal_connect(app, "notify", G_CALLBACK(app_notified), self);
> @@ -631,7 +637,6 @@ remote_viewer_start(VirtViewerApp *app)
>
> file = g_file_new_for_commandline_arg(guri);
> if (g_file_query_exists(file, NULL)) {
> - GError *error = NULL;
> gchar *path = g_file_get_path(file);
> vvfile = virt_viewer_file_new(path, &error);
> g_free(path);
> @@ -654,8 +659,12 @@ remote_viewer_start(VirtViewerApp *app)
>
> virt_viewer_session_set_file(virt_viewer_app_get_session(app), vvfile);
>
> - if (virt_viewer_app_initial_connect(app) < 0) {
> - virt_viewer_app_simple_message_dialog(app, _("Failed to initiate connection"));
> + if (!virt_viewer_app_initial_connect(app, &error)) {
> + const gchar *msg = error ? error->message :
> + _("Failed to initiate connection");
> +
> + virt_viewer_app_simple_message_dialog(app, msg);
> + g_clear_error(&error);
> goto cleanup;
> }
> #if HAVE_SPICE_GTK
> diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
> index 25df5c8..2e6c76d 100644
> --- a/src/virt-viewer-app.c
> +++ b/src/virt-viewer-app.c
> @@ -860,14 +860,14 @@ virt_viewer_app_channel_open(VirtViewerSession *session G_GNUC_UNUSED,
> }
> #endif
>
> -static int
> -virt_viewer_app_default_activate(VirtViewerApp *self)
> +static gboolean
> +virt_viewer_app_default_activate(VirtViewerApp *self, GError **error)
> {
> VirtViewerAppPrivate *priv = self->priv;
> int fd = -1;
>
> if (!virt_viewer_app_open_connection(self, &fd))
> - return -1;
> + return FALSE;
>
> DEBUG_LOG("After open connection callback fd=%d", fd);
>
> @@ -897,12 +897,12 @@ virt_viewer_app_default_activate(VirtViewerApp *self)
> if ((fd = virt_viewer_app_open_tunnel_ssh(priv->host, priv->port,
> priv->user, priv->ghost,
> priv->gport, priv->unixsock)) < 0)
> - return -1;
> + return FALSE;
> } else if (priv->unixsock && fd == -1) {
> virt_viewer_app_trace(self, "Opening direct UNIX connection to display at %s",
> priv->unixsock);
> if ((fd = virt_viewer_app_open_unix_sock(priv->unixsock)) < 0)
> - return -1;
> + return FALSE;
> }
> #endif
>
> @@ -910,7 +910,7 @@ virt_viewer_app_default_activate(VirtViewerApp *self)
> return virt_viewer_session_open_fd(VIRT_VIEWER_SESSION(priv->session), fd);
> } else if (priv->guri) {
> virt_viewer_app_trace(self, "Opening connection to display at %s", priv->guri);
> - return virt_viewer_session_open_uri(VIRT_VIEWER_SESSION(priv->session), priv->guri);
> + return virt_viewer_session_open_uri(VIRT_VIEWER_SESSION(priv->session), priv->guri, error);
> } else {
> virt_viewer_app_trace(self, "Opening direct TCP connection to display at %s:%s:%s",
> priv->ghost, priv->gport, priv->gtlsport ? priv->gtlsport : "-1");
> @@ -918,24 +918,24 @@ virt_viewer_app_default_activate(VirtViewerApp *self)
> priv->ghost, priv->gport, priv->gtlsport);
> }
>
> - return -1;
> + return FALSE;
> }
>
> -int
> -virt_viewer_app_activate(VirtViewerApp *self)
> +gboolean
> +virt_viewer_app_activate(VirtViewerApp *self, GError **error)
> {
> VirtViewerAppPrivate *priv;
> int ret;
ret should be changed into a gboolean here.
>
> - g_return_val_if_fail(VIRT_VIEWER_IS_APP(self), -1);
> + g_return_val_if_fail(VIRT_VIEWER_IS_APP(self), FALSE);
>
> priv = self->priv;
> if (priv->active)
> - return -1;
> + return FALSE;
>
> - ret = VIRT_VIEWER_APP_GET_CLASS(self)->activate(self);
> + ret = VIRT_VIEWER_APP_GET_CLASS(self)->activate(self, error);
>
> - if (ret == -1) {
> + if (ret == FALSE) {
> priv->connected = FALSE;
> } else {
> virt_viewer_app_show_status(self, _("Connecting to graphic server"));
> @@ -1004,21 +1004,21 @@ static void virt_viewer_app_bell(VirtViewerSession *session G_GNUC_UNUSED,
> }
>
>
> -static int
> -virt_viewer_app_default_initial_connect(VirtViewerApp *self)
> +static gboolean
> +virt_viewer_app_default_initial_connect(VirtViewerApp *self, GError **error)
> {
> - return virt_viewer_app_activate(self);
> + return virt_viewer_app_activate(self, error);
> }
>
> -int
> -virt_viewer_app_initial_connect(VirtViewerApp *self)
> +gboolean
> +virt_viewer_app_initial_connect(VirtViewerApp *self, GError **error)
> {
> VirtViewerAppClass *klass;
>
> - g_return_val_if_fail(VIRT_VIEWER_IS_APP(self), -1);
> + g_return_val_if_fail(VIRT_VIEWER_IS_APP(self), FALSE);
> klass = VIRT_VIEWER_APP_GET_CLASS(self);
>
> - return klass->initial_connect(self);
> + return klass->initial_connect(self, error);
> }
>
> static gboolean
> @@ -1026,7 +1026,7 @@ virt_viewer_app_retryauth(gpointer opaque)
> {
> VirtViewerApp *self = opaque;
>
> - virt_viewer_app_initial_connect(self);
> + virt_viewer_app_initial_connect(self, NULL);
>
> return FALSE;
> }
> @@ -1040,7 +1040,7 @@ virt_viewer_app_connect_timer(void *opaque)
> DEBUG_LOG("Connect timer fired");
>
> if (!priv->active &&
> - virt_viewer_app_initial_connect(self) < 0)
> + virt_viewer_app_initial_connect(self, NULL) < 0)
> gtk_main_quit();
>
> if (priv->active) {
> diff --git a/src/virt-viewer-app.h b/src/virt-viewer-app.h
> index 663f7a1..6fc523b 100644
> --- a/src/virt-viewer-app.h
> +++ b/src/virt-viewer-app.h
> @@ -52,8 +52,8 @@ typedef struct {
>
> /*< private >*/
> gboolean (*start) (VirtViewerApp *self);
> - int (*initial_connect) (VirtViewerApp *self);
> - int (*activate) (VirtViewerApp *self);
> + gboolean (*initial_connect) (VirtViewerApp *self, GError **error);
> + gboolean (*activate) (VirtViewerApp *self, GError **error);
> void (*deactivated) (VirtViewerApp *self);
> gboolean (*open_connection)(VirtViewerApp *self, int *fd);
> } VirtViewerAppClass;
> @@ -71,8 +71,8 @@ void virt_viewer_app_simple_message_dialog(VirtViewerApp *self, const char *fmt,
> gboolean virt_viewer_app_is_active(VirtViewerApp *app);
> void virt_viewer_app_free_connect_info(VirtViewerApp *self);
> int virt_viewer_app_create_session(VirtViewerApp *self, const gchar *type);
> -int virt_viewer_app_activate(VirtViewerApp *self);
> -int virt_viewer_app_initial_connect(VirtViewerApp *self);
> +gboolean virt_viewer_app_activate(VirtViewerApp *self, GError **error);
> +gboolean virt_viewer_app_initial_connect(VirtViewerApp *self, GError **error);
> void virt_viewer_app_start_reconnect_poll(VirtViewerApp *self);
> void virt_viewer_app_set_zoom_level(VirtViewerApp *self, gint zoom_level);
> void virt_viewer_app_set_direct(VirtViewerApp *self, gboolean direct);
> diff --git a/src/virt-viewer-file.c b/src/virt-viewer-file.c
> index 7271156..33b9666 100644
> --- a/src/virt-viewer-file.c
> +++ b/src/virt-viewer-file.c
> @@ -20,6 +20,8 @@
> */
>
> #include <config.h>
> +#include <glib/gi18n.h>
> +
Hmm, unrelated / unneeded change ?
> #include "virt-viewer-util.h"
> #include "virt-viewer-file.h"
>
> diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer-session-spice.c
> index 80a3f34..cfe31ac 100644
> --- a/src/virt-viewer-session-spice.c
> +++ b/src/virt-viewer-session-spice.c
> @@ -64,7 +64,7 @@ enum {
> static void virt_viewer_session_spice_close(VirtViewerSession *session);
> static gboolean virt_viewer_session_spice_open_fd(VirtViewerSession *session, int fd);
> static gboolean virt_viewer_session_spice_open_host(VirtViewerSession *session, const gchar *host, const gchar *port, const gchar *tlsport);
> -static gboolean virt_viewer_session_spice_open_uri(VirtViewerSession *session, const gchar *uri);
> +static gboolean virt_viewer_session_spice_open_uri(VirtViewerSession *session, const gchar *uri, GError **error);
> static gboolean virt_viewer_session_spice_channel_open_fd(VirtViewerSession *session, VirtViewerSessionChannel *channel, int fd);
> static gboolean virt_viewer_session_spice_has_usb(VirtViewerSession *session);
> static void virt_viewer_session_spice_usb_device_selection(VirtViewerSession *session, GtkWindow *parent);
> @@ -342,7 +342,7 @@ fill_session(VirtViewerFile *file, SpiceSession *session)
>
> static gboolean
> virt_viewer_session_spice_open_uri(VirtViewerSession *session,
> - const gchar *uri)
> + const gchar *uri, GError **error)
> {
> VirtViewerSessionSpice *self = VIRT_VIEWER_SESSION_SPICE(session);
> VirtViewerFile *file = virt_viewer_session_get_file(session);
> diff --git a/src/virt-viewer-session-vnc.c b/src/virt-viewer-session-vnc.c
> index 3abd4e4..bbebc7e 100644
> --- a/src/virt-viewer-session-vnc.c
> +++ b/src/virt-viewer-session-vnc.c
> @@ -44,7 +44,7 @@ struct _VirtViewerSessionVncPrivate {
> static void virt_viewer_session_vnc_close(VirtViewerSession* session);
> static gboolean virt_viewer_session_vnc_open_fd(VirtViewerSession* session, int fd);
> static gboolean virt_viewer_session_vnc_open_host(VirtViewerSession* session, const gchar *host, const gchar *port, const gchar *tlsport);
> -static gboolean virt_viewer_session_vnc_open_uri(VirtViewerSession* session, const gchar *uri);
> +static gboolean virt_viewer_session_vnc_open_uri(VirtViewerSession* session, const gchar *uri, GError **error);
> static gboolean virt_viewer_session_vnc_channel_open_fd(VirtViewerSession* session,
> VirtViewerSessionChannel* channel, int fd);
>
> @@ -198,7 +198,8 @@ virt_viewer_session_vnc_open_host(VirtViewerSession* session,
>
> static gboolean
> virt_viewer_session_vnc_open_uri(VirtViewerSession* session,
> - const gchar *uristr)
> + const gchar *uristr,
> + GError **error)
> {
> VirtViewerSessionVnc *self = VIRT_VIEWER_SESSION_VNC(session);
> VirtViewerFile *file = virt_viewer_session_get_file(session);
> diff --git a/src/virt-viewer-session.c b/src/virt-viewer-session.c
> index 16c878a..07e7226 100644
> --- a/src/virt-viewer-session.c
> +++ b/src/virt-viewer-session.c
> @@ -374,7 +374,7 @@ gboolean virt_viewer_session_open_host(VirtViewerSession *session, const gchar *
> return klass->open_host(session, host, port, tlsport);
> }
>
> -gboolean virt_viewer_session_open_uri(VirtViewerSession *session, const gchar *uri)
> +gboolean virt_viewer_session_open_uri(VirtViewerSession *session, const gchar *uri, GError **error)
> {
> VirtViewerSessionClass *klass;
>
> @@ -385,7 +385,7 @@ gboolean virt_viewer_session_open_uri(VirtViewerSession *session, const gchar *u
>
> session->priv->uri = g_strdup(uri);
>
> - return klass->open_uri(session, uri);
> + return klass->open_uri(session, uri, error);
> }
>
> const gchar* virt_viewer_session_mime_type(VirtViewerSession *self)
> diff --git a/src/virt-viewer-session.h b/src/virt-viewer-session.h
> index f337937..5aa748c 100644
> --- a/src/virt-viewer-session.h
> +++ b/src/virt-viewer-session.h
> @@ -68,7 +68,7 @@ struct _VirtViewerSessionClass {
> void (* close) (VirtViewerSession* session);
> gboolean (* open_fd) (VirtViewerSession* session, int fd);
> gboolean (* open_host) (VirtViewerSession* session, const gchar *host, const gchar *port, const gchar *tlsport);
> - gboolean (* open_uri) (VirtViewerSession* session, const gchar *uri);
> + gboolean (* open_uri) (VirtViewerSession* session, const gchar *uri, GError **error);
> gboolean (* channel_open_fd) (VirtViewerSession* session, VirtViewerSessionChannel *channel, int fd);
> gboolean (* has_usb) (VirtViewerSession* session);
> void (* usb_device_selection) (VirtViewerSession* session, GtkWindow *parent);
> @@ -114,7 +114,7 @@ gboolean virt_viewer_session_open_host(VirtViewerSession* session, const gchar *
> GObject* virt_viewer_session_get(VirtViewerSession* session);
> gboolean virt_viewer_session_channel_open_fd(VirtViewerSession* session,
> VirtViewerSessionChannel* channel, int fd);
> -gboolean virt_viewer_session_open_uri(VirtViewerSession *session, const gchar *uri);
> +gboolean virt_viewer_session_open_uri(VirtViewerSession *session, const gchar *uri, GError **error);
>
> void virt_viewer_session_set_auto_usbredir(VirtViewerSession* session, gboolean auto_usbredir);
> gboolean virt_viewer_session_get_auto_usbredir(VirtViewerSession* session);
> diff --git a/src/virt-viewer.c b/src/virt-viewer.c
> index aedbf73..8d4ec62 100644
> --- a/src/virt-viewer.c
> +++ b/src/virt-viewer.c
> @@ -63,7 +63,7 @@ G_DEFINE_TYPE (VirtViewer, virt_viewer, VIRT_VIEWER_TYPE_APP)
> #define GET_PRIVATE(o) \
> (G_TYPE_INSTANCE_GET_PRIVATE ((o), VIRT_VIEWER_TYPE, VirtViewerPrivate))
>
> -static int virt_viewer_initial_connect(VirtViewerApp *self);
> +static gboolean virt_viewer_initial_connect(VirtViewerApp *self, GError **error);
> static gboolean virt_viewer_open_connection(VirtViewerApp *self, int *fd);
> static void virt_viewer_deactivated(VirtViewerApp *self);
> static gboolean virt_viewer_start(VirtViewerApp *self);
> @@ -406,7 +406,7 @@ virt_viewer_extract_connect_info(VirtViewer *self,
> return retval;
> }
>
> -static int
> +static gboolean
> virt_viewer_update_display(VirtViewer *self, virDomainPtr dom)
> {
> VirtViewerPrivate *priv = self->priv;
> @@ -424,10 +424,10 @@ virt_viewer_update_display(VirtViewer *self, virDomainPtr dom)
>
> if (!virt_viewer_app_has_session(app)) {
> if (!virt_viewer_extract_connect_info(self, dom))
> - return -1;
> + return FALSE;
> }
>
> - return 0;
> + return TRUE;
> }
>
> static gboolean
> @@ -469,6 +469,7 @@ virt_viewer_domain_event(virConnectPtr conn G_GNUC_UNUSED,
> {
> VirtViewer *self = opaque;
> VirtViewerApp *app = VIRT_VIEWER_APP(self);
> + GError *error = NULL;
>
> DEBUG_LOG("Got domain event %d %d", event, detail);
>
> @@ -482,7 +483,13 @@ virt_viewer_domain_event(virConnectPtr conn G_GNUC_UNUSED,
>
> case VIR_DOMAIN_EVENT_STARTED:
> virt_viewer_update_display(self, dom);
> - virt_viewer_app_activate(app);
> + virt_viewer_app_activate(app, &error);
> + if (error) {
> + /* we may want to consolidate error reporting in
> + app_activate() instead */
> + g_warning("%s", error->message);
> + g_clear_error(&error);
> + }
> break;
> }
>
> @@ -508,16 +515,15 @@ virt_viewer_conn_event(virConnectPtr conn G_GNUC_UNUSED,
>
> static int virt_viewer_connect(VirtViewerApp *app);
>
> -static int
> -virt_viewer_initial_connect(VirtViewerApp *app)
> +static gboolean
> +virt_viewer_initial_connect(VirtViewerApp *app, GError **error)
> {
> virDomainPtr dom = NULL;
> virDomainInfo info;
> - int ret = -1;
> + gboolean ret = FALSE;
> VirtViewer *self = VIRT_VIEWER(app);
> VirtViewerPrivate *priv = self->priv;
>
> -
> DEBUG_LOG("initial connect");
>
> if (!priv->conn &&
> @@ -552,9 +558,9 @@ virt_viewer_initial_connect(VirtViewerApp *app)
> virt_viewer_app_show_status(app, _("Waiting for guest domain to start"));
> } else {
> ret = virt_viewer_update_display(self, dom);
> - if (ret >= 0)
> - ret = VIRT_VIEWER_APP_CLASS(virt_viewer_parent_class)->initial_connect(app);
> - if (ret < 0) {
> + if (ret)
> + ret = VIRT_VIEWER_APP_CLASS(virt_viewer_parent_class)->initial_connect(app, error);
> + if (!ret) {
> if (priv->waitvm) {
> virt_viewer_app_show_status(app, _("Waiting for guest domain to start server"));
> virt_viewer_app_trace(app, "Guest %s has not activated its display yet, waiting for it to start",
> @@ -563,15 +569,11 @@ virt_viewer_initial_connect(VirtViewerApp *app)
> DEBUG_LOG("Failed to activate viewer");
> goto cleanup;
> }
> - } else if (ret == 0) {
> - DEBUG_LOG("Failed to activate viewer");
> - ret = -1;
> - goto cleanup;
> }
> }
>
> done:
> - ret = 0;
> + ret = TRUE;
> cleanup:
> if (dom)
> virDomainFree(dom);
> @@ -660,6 +662,7 @@ virt_viewer_connect(VirtViewerApp *app)
> .cbdata = app,
> };
> int oflags = 0;
> + GError *error = NULL;
>
> if (!virt_viewer_app_get_attach(app))
> oflags |= VIR_CONNECT_RO;
> @@ -678,8 +681,12 @@ virt_viewer_connect(VirtViewerApp *app)
> return -1;
> }
>
> - if (virt_viewer_app_initial_connect(app) < 0)
> + if (!virt_viewer_app_initial_connect(app, &error)) {
> + if (error)
> + g_warning(error->message);
> + g_clear_error(&error);
> return -1;
> + }
>
> if (virConnectDomainEventRegister(priv->conn,
> virt_viewer_domain_event,
>
Regards,
Hans
More information about the virt-tools-list
mailing list