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

Re: [virt-tools-list] [PATCH virt-viewer 3/4] Allow app_initial_connect() to raise an error



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


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