[virt-tools-list] [PATCH virt-viewer v2] remote-viewer: Pass guri to remote_viewer_session_connected

[Victor Toso]

Hi,

On Tue, Nov 14, 2017 at 02:14:25PM -0200, Eduardo Lima (Etrunko) wrote:
> On 14/11/17 13:47, Victor Toso wrote:
> > On Thu, Oct 26, 2017 at 03:39:31PM +0200, Eduardo Lima (Etrunko) wrote:
> >> When connecting to a VM via oVirt instance, the original uri can not be
> >> retrieved using virt_viewer_session_get_uri(). Consequently, it was
> >> never saved, even though the connection succeeds and the actual callback
> >> for "session-connected" signal, which saves the URI, is invoked.
> >>
> >> To solve this problem, we always pass a copy of the guri as user-data
> >> parameter for the callback, and if the call to
> >> virt_viewer_session_get_uri() returns NULL, the parameter is used
> >> instead.
> >>
> >> Resolves: https://bugzilla.redhat.com/1459792
> >>
> >> Signed-off-by: Eduardo Lima (Etrunko) <etrunko at redhat.com>
> >> ---
> >>  src/remote-viewer.c | 18 +++++++++---------
> >>  1 file changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> >> index fb5376c..58dc04f 100644
> >> --- a/src/remote-viewer.c
> >> +++ b/src/remote-viewer.c
> >> @@ -85,10 +85,6 @@ static void spice_foreign_menu_updated(RemoteViewer *self);
> >>  static void foreign_menu_title_changed(SpiceCtrlForeignMenu *menu, GParamSpec *pspec, RemoteViewer *self);
> >>  #endif
> >>  
> >> -static gboolean
> >> -remote_viewer_initial_connect(RemoteViewer *self, const gchar *type,
> >> -                              VirtViewerFile *vvfile, GError **error);
> >> -
> >>  static void
> >>  remote_viewer_dispose (GObject *object)
> >>  {
> >> @@ -1075,17 +1071,21 @@ remote_viewer_recent_add(gchar *uri, const gchar *mime_type)
> >>  
> >>  static void
> >>  remote_viewer_session_connected(VirtViewerSession *session,
> >> -                                VirtViewerApp *self G_GNUC_UNUSED)
> >> +                                gchar *guri)
> >>  {
> >>      gchar *uri = virt_viewer_session_get_uri(session);
> >>      const gchar *mime = virt_viewer_session_mime_type(session);
> >>  
> >> +    if (uri == NULL)
> >> +        uri = g_strdup(guri);
> >> +
> >>      remote_viewer_recent_add(uri, mime);
> > 
> > Could you also change the if (uri == NULL) in above function to a
> > g_return_if_fail(uri != NULL) as it should not happen anymore...
> > 
> 
> I don't really get what you mean here, as I explained on the commit
> message, uri *can* be NULL. In this case, we will use the guri, which is
> now passed as user_data argument to the callback.

You are right, you just did not get what I mean here.

If virt_viewer_session_get_uri() returns NULL we use the guri which
*cannot be null* - That means that we can change the check in
remote_viewer_recent_add() from if (uri == NULL) return; to
g_return_if_fail() instead.

This is more like a suggestion. Another possibility is just to remove
the check (in remote_viewer_recent_add()) entirely) as we should have
critical messages elsewhere if guri is null.

Cheers,

> 
> > Either way,
> > Reviewed-by: Victor Toso <victortoso at redhat.com>
> > 
> >>      g_free(uri);
> >> +    g_free(guri);
> >>  }
> >>  
> >>  static gboolean
> >> -remote_viewer_initial_connect(RemoteViewer *self, const gchar *type,
> >> +remote_viewer_initial_connect(RemoteViewer *self, const gchar *type, const gchar *guri,
> >>                                VirtViewerFile *vvfile, GError **error)
> >>  {
> >>      VirtViewerApp *app = VIRT_VIEWER_APP(self);
> >> @@ -1093,8 +1093,9 @@ remote_viewer_initial_connect(RemoteViewer *self, const gchar *type,
> >>      if (!virt_viewer_app_create_session(app, type, error))
> >>          return FALSE;
> >>  
> >> +
> >>      g_signal_connect(virt_viewer_app_get_session(app), "session-connected",
> >> -                     G_CALLBACK(remote_viewer_session_connected), app);
> >> +                     G_CALLBACK(remote_viewer_session_connected), g_strdup(guri));
> >>  
> >>      virt_viewer_session_set_file(virt_viewer_app_get_session(app), vvfile);
> >>  #ifdef HAVE_OVIRT
> >> @@ -1200,12 +1201,11 @@ retry_dialog:
> >>          } else
> >>  #endif
> >>          {
> >> -            if (!remote_viewer_initial_connect(self, type, vvfile, &error))
> >> +            if (!remote_viewer_initial_connect(self, type, guri, vvfile, &error))
> >>                  goto cleanup;
> >>          }
> >>      }
> >>  
> >> -
> >>      ret = VIRT_VIEWER_APP_CLASS(remote_viewer_parent_class)->start(app, &error);
> >>  
> >>  cleanup:
> >> -- 
> >> 2.13.6
> >>
> >> _______________________________________________
> >> virt-tools-list mailing list
> >> virt-tools-list at redhat.com
> >> https://www.redhat.com/mailman/listinfo/virt-tools-list
> 
> 
> -- 
> Eduardo de Barros Lima (Etrunko)
> Software Engineer - RedHat
> etrunko at redhat.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20171114/0940531e/attachment.sig>