[Spice-devel] [PATCH spice-server v2] Move channel registration to constructed vfunc
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
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
> 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
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