[Spice-devel] [PATCH spice-server v2] Move channel registration to constructed vfunc

2019-03-14 Thread Jonathon Jongsma
For the Display Channel and the Cursor channel, move the call to
reds_register_channel() to the _constructed() vfunc rather than calling
it explicitly from RedWorker. This matches what other channels do.
---
Changes in v2:
 - remove cursor channel registration in stream device

Note that I didn't implement a couple other suggestions from Frediano's first 
review. Reference
earlier email reply for justification.

 server/cursor-channel.c| 12 
 server/display-channel.c   |  2 ++
 server/red-stream-device.c |  1 -
 server/red-worker.c|  2 --
 4 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/server/cursor-channel.c b/server/cursor-channel.c
index 9dd69fa25..4220084f5 100644
--- a/server/cursor-channel.c
+++ b/server/cursor-channel.c
@@ -366,12 +366,24 @@ cursor_channel_finalize(GObject *object)
 G_OBJECT_CLASS(cursor_channel_parent_class)->finalize(object);
 }
 
+static void
+cursor_channel_constructed(GObject *object)
+{
+RedChannel *red_channel = RED_CHANNEL(object);
+RedsState *reds = red_channel_get_server(red_channel);
+
+G_OBJECT_CLASS(cursor_channel_parent_class)->constructed(object);
+
+reds_register_channel(reds, red_channel);
+}
+
 static void
 cursor_channel_class_init(CursorChannelClass *klass)
 {
 GObjectClass *object_class = G_OBJECT_CLASS(klass);
 RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
 
+object_class->constructed = cursor_channel_constructed;
 object_class->finalize = cursor_channel_finalize;
 
 channel_class->parser = 
spice_get_client_channel_parser(SPICE_CHANNEL_CURSOR, NULL);
diff --git a/server/display-channel.c b/server/display-channel.c
index e68ed10f8..6684135eb 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -2304,6 +2304,8 @@ display_channel_constructed(GObject *object)
 red_channel_set_cap(channel, SPICE_DISPLAY_CAP_PREF_COMPRESSION);
 red_channel_set_cap(channel, SPICE_DISPLAY_CAP_PREF_VIDEO_CODEC_TYPE);
 red_channel_set_cap(channel, SPICE_DISPLAY_CAP_STREAM_REPORT);
+
+reds_register_channel(reds, channel);
 }
 
 void display_channel_process_surface_cmd(DisplayChannel *display,
diff --git a/server/red-stream-device.c b/server/red-stream-device.c
index b9e0827af..78a8c9083 100644
--- a/server/red-stream-device.c
+++ b/server/red-stream-device.c
@@ -701,7 +701,6 @@ stream_device_create_channel(StreamDevice *dev)
 client_cbs.connect = (channel_client_connect_proc) cursor_channel_connect;
 client_cbs.migrate = cursor_channel_client_migrate;
 red_channel_register_client_cbs(RED_CHANNEL(cursor_channel), &client_cbs);
-reds_register_channel(reds, RED_CHANNEL(cursor_channel));
 
 dev->stream_channel = stream_channel;
 dev->cursor_channel = cursor_channel;
diff --git a/server/red-worker.c b/server/red-worker.c
index c74ae8886..3cb12b9c2 100644
--- a/server/red-worker.c
+++ b/server/red-worker.c
@@ -1322,7 +1322,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
 red_channel_init_stat_node(channel, &worker->stat, "cursor_channel");
 red_channel_register_client_cbs(channel, client_cursor_cbs);
 g_object_set_data(G_OBJECT(channel), "dispatcher", dispatcher);
-reds_register_channel(reds, channel);
 
 // TODO: handle seamless migration. Temp, setting migrate to FALSE
 worker->display_channel = display_channel_new(reds, qxl, &worker->core, 
FALSE,
@@ -1333,7 +1332,6 @@ RedWorker* red_worker_new(QXLInstance *qxl,
 red_channel_init_stat_node(channel, &worker->stat, "display_channel");
 red_channel_register_client_cbs(channel, client_display_cbs);
 g_object_set_data(G_OBJECT(channel), "dispatcher", dispatcher);
-reds_register_channel(reds, channel);
 
 return worker;
 }
-- 
2.17.2

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-gtk 1/3] clipboard: accept grab only from the side with keyboard focus

2019-03-14 Thread Marc-André Lureau
Hi

On Thu, Feb 28, 2019 at 8:12 PM Jakub Janků  wrote:
>
> If two grab messages in opposite directions "meet" on their way
> to their destinations, we end up in a state when both spice-gtk
> and spice-vdagent think that the other component can provide
> clipboard data. As a consequence of this, neither of them can.
>
> This is a bug in the spice-protocol. To mitigate the issue,
> accept grab only from the side that currently has keyboard focus,
> this means:
> (1) spice-gtk has focus ==>
> * accept grabs from vdagent,
> * ignore grabs from other apps running in the host
> (2) spice-gtk doesn't have focus ==>
> * accept grabs from other apps running in the host
> * ignore grabs from vdagent
>
> The check in clipboard_owner_change() is X11-specific,
> because on Wayland, spice-gtk is first notified about the
> owner-change once it gains focus.
>
> The check in clipboard_grab() can be generic.
> Note that on Wayland, calling gtk_clipboard_set_with_owner()
> while not having focus doesn't have any effect anyway,
> only focused clients can set clipboard.
>
> With this patch, the race can still occur around the time
> when focus changes (rare, but possible), on X11 as well as Wayland.
>
> Related: https://gitlab.freedesktop.org/spice/spice-gtk/issues/82
> Related: https://bugzilla.redhat.com/show_bug.cgi?id=1594876
>
> Signed-off-by: Jakub Janků 
> ---
>
> Victor, half of this patch is basically yours,
> so feel free to add signed-off or something,
> in the case this gets upstream :)
>
> ---
>  src/spice-gtk-session.c | 13 +
>  1 file changed, 13 insertions(+)
>
> diff --git a/src/spice-gtk-session.c b/src/spice-gtk-session.c
> index b48f92a..7b7370c 100644
> --- a/src/spice-gtk-session.c
> +++ b/src/spice-gtk-session.c
> @@ -680,6 +680,13 @@ static void clipboard_owner_change(GtkClipboard
> *clipboard,
>  s->clip_hasdata[selection] = FALSE;
>  return;
>  }
> +
> +if (GDK_IS_X11_DISPLAY(gdk_display_get_default()) &&
> +spice_gtk_session_get_keyboard_has_focus(self)) {
> +SPICE_DEBUG("%s: spice-gtk has keyboard focus, "
> +"ignoring grab from other app", __FUNCTION__);
> +return;
> +}
>  #endif

This will break clipboard managers interactions, which may take the
clipboard content, save it and/or modify it.

>
>  s->clip_hasdata[selection] = TRUE;
> @@ -823,6 +830,12 @@ static gboolean clipboard_grab(SpiceMainChannel *main, 
> guint selection,
>  cb = get_clipboard_from_selection(s, selection);
>  g_return_val_if_fail(cb != NULL, FALSE);
>
> +if (!spice_gtk_session_get_keyboard_has_focus(self)) {
> +SPICE_DEBUG("%s: spice-gtk doesn't have keyboard focus, "
> +"ignoring grab from guest agent", __FUNCTION__);
> +return FALSE;
> +}


Beside automation, the cursor alone may easily create a new clipboard
content which won't be available to the client side (the auto-grab may
fail to follow cursor etc).

It's a bit unclear why it's not X11 specific but for client side
change it is, this could deserve a code comment.

All in all, this feels weak and breaks some legitimate cases.

I am not very strongly against this, as I understand it may help with
some races we discussed, but it feels like the problem is elsewhere
and we need a better solution to prevent the race from happening.

I haven't read the bug reports: this kind of workaround needs a
description of a broken use case (not a theoretical description of a
race that "never" happen in practice).


> +
>  for (n = 0; n < ntypes; ++n) {
>  found = FALSE;
>  for (m = 0; m < SPICE_N_ELEMENTS(atom2agent); m++) {
> --
> 2.20.1
>
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH] usbredirserver: Make compile under MacOS

2019-03-14 Thread Christophe de Dinechin


> On 29 Jan 2019, at 10:23, Christophe Fergeau  wrote:
> 
> On Mon, Jan 28, 2019 at 02:28:36PM -0500, Frediano Ziglio wrote:
>>> On Wed, Jan 23, 2019 at 10:09:25AM +, Frediano Ziglio wrote:
 This fixes Gitlab issue #9
 (cfr https://gitlab.freedesktop.org/spice/usbredir/issues/9).
 
 Signed-off-by: Frediano Ziglio 
 ---
 usbredirserver/usbredirserver.c | 7 +++
 1 file changed, 7 insertions(+)
 
 diff --git a/usbredirserver/usbredirserver.c
 b/usbredirserver/usbredirserver.c
 index 6aa2740..429985a 100644
 --- a/usbredirserver/usbredirserver.c
 +++ b/usbredirserver/usbredirserver.c
 @@ -43,6 +43,13 @@
 
 #define SERVER_VERSION "usbredirserver " PACKAGE_VERSION
 
 +#if !defined(SOL_TCP) && defined(IPPROTO_TCP)
 +#define SOL_TCP IPPROTO_TCP
 +#endif
 +#if !defined(TCP_KEEPIDLE) && defined(TCP_KEEPALIVE)
 +#define TCP_KEEPIDLE TCP_KEEPALIVE
 +#endif
>>> 
>>> Might be better to restrict this to OSX in case a system decides to
>>> use TCP_KEEPALIVE, but with a different meaning?
>>> 
>> 
>> So
>> 
>> #if !defined(TCP_KEEPIDLE) && defined(TCP_KEEPALIVE) && defined(__APPLE__)
> 
> Yep, or even just #if defined(__APPLE__)

I prefer Frediano’s version, Apple may change it later.

> 
> Christophe
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-gtk 08/13] usb-redir: change signal prototype of win-usb-dev

2019-03-14 Thread Christophe Fergeau
On Wed, Mar 13, 2019 at 08:14:14PM +0200, Yuri Benditovich wrote:
> On Tue, Mar 12, 2019 at 4:02 PM Christophe Fergeau  
> wrote:
> >
> > On Sun, Mar 10, 2019 at 04:46:07PM +0200, Yuri Benditovich wrote:
> > > Changing signal definition from (boxed-boxed) to (pointer,int).
> > > There is no need for additional referencing of GUdevDevice
> > > object before signal callback.
> >
> > I still feel it would be nicer to guarantee the GUdevDevice will stay
> > alive for the whole duration of the signal emission. For example, 2
> > callbacks may be attached to this signal, we don't want the first one to
> > be able to drop the last ref to the GUdevDevice and risking the second
> > one trying to use freed memory.
> >
> Additional referencing here is not required. The signal callback does not
> dereference the GUdevDevice object, so this does not make any difference
> how many callbacks attached to the signal.

Yes, right now it does not make any difference. But maybe that will make
a difference for someone hacking on that code in a few years. And
gobject signals usually own a reference on signals parameters while the
callbacks are running. Hence the suggestion ;)

> 
> 
> 
> > > Second parameter (action) is 0 for device removal and 1 for device
> > > addition.
> >
> > Imo this should either be a gboolean or an enum.
> 
> I do not see any reason to invent additional marshalling procedure where
> existing one can be used without any problem.

Code readability. More or less the same reasons why we have
gboolean/bool while int does the job. Someone looking at the callback
won't wonder why only 0 and 1 are handled, and won't be worried whether
they can get other values in the callback.

Christophe


signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel