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

[Eduardo Lima (Etrunko)]

On 14/11/17 14:38, Victor Toso wrote:
> 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.
> 

Now I got it, you were talking about the other function, but commented
on this block of code in remote_viewer_session_connected(), thus my
confusion. I will change the check as suggested.

Thanks, Eduardo.

> 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


-- 
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etrunko at redhat.com