Re: [virt-tools-list] [PATCH virt-viewer v2] remote-viewer: Pass guri to remote_viewer_session_connected
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)
---
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
>>>
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@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
>>
>>
>> --
>> Eduardo de Barros Lima (E
Re: [virt-tools-list] [PATCH virt-viewer v2] remote-viewer: Pass guri to remote_viewer_session_connected
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)
> >> ---
> >> 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
> >
> >> 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@redhat.com
> >> https://www.redhat.com/mailman/listinfo/virt-tools-list
>
>
> --
> Eduardo de Barros Lima (Etrunko)
> Software Engineer - RedHat
> etru...@redhat.com
signature.asc
Description: PGP signature
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH virt-viewer v2] remote-viewer: Pass guri to remote_viewer_session_connected
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)
>> ---
>> 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.
> Either way,
> Reviewed-by: Victor Toso
>
>> 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@redhat.com
>> https://www.redhat.com/mailman/listinfo/virt-tools-list
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH virt-viewer v2] remote-viewer: Pass guri to remote_viewer_session_connected
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)
> ---
> 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...
Either way,
Reviewed-by: Victor Toso
> 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@redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list
signature.asc
Description: PGP signature
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
