date:20190328

Hi

On Thu, Mar 28, 2019 at 6:05 PM Frediano Ziglio  wrote:
>
> >
> > ..Hi
> >
> > On Thu, Mar 28, 2019 at 4:14 PM Frediano Ziglio  wrote:
> > > > The role of the grab message is to take ownership of the clipboard (to
> > > > advertize clipboard data available). It may come at any time from both
> > > > side, and override the current grab owner. It may come from the guest
> > > > (after C-c in guest app, for ex), so the client grabs the clipboard.
> > > > Or it may come from the client (C-c in client side app), and the agent
> > > > will grab the guest clipboard.
> > > >
> > >
> > > Yes, I realized this morning thinking about how clipboards works
> > > (very rusty in my mind).
> > > Instead of setting it you get the ownership that is you are willing
> > > to set a value on the clipboard but you defer setting it to avoid
> > > the expense of data copy.
> > > So, the old (original?) protocol was simply to sending entire data
> > > on every change, this avoided to loose the clipboard data entirely.
> >
> > I don't even remember that version. It looks like the original version
> > was already "on-demand"
> >
> >
> > commit 5fdd0ba6650919dcfd7740c041ad7d2b9c4560ab
> > Author: Arnon Gilboa 
> > Date:   Mon Oct 4 16:45:15 2010 +0200
> >
> > vd_agent: add protocol messages for clipboard/selection-owner model
> >
> > -VD_AGENT_CLIPBOARD_GRAB(type) - tell the other side that an
> > application in our side ("we") got ownership of the clipboard.
> > -VD_AGENT_CLIPBOARD_REQUEST(type) - after we know the other side
> > owns the clipboard (GRAB received), we notify the os we are the owner.
> > when we are asked by the os/app for the clipboard data, we send this
> > RE
> > QUEST msg to the other side.
> > -VD_AGENT_CLIPBOARD(type, data) - the existing message for sending
> > the clipboard, is now sent only in response to REQUEST.
> > -VD_AGENT_CLIPBOARD_RELEASE - tell the other side that we are no
> > longer the owner of the clipboard (e.g. the owner app was closed).
> >
> > this patch will be followed by agent & client patches handling the
> > above messages.
> >
> >
> >
> > So VD_AGENT_CAP_CLIPBOARD_BY_DEMAND simply means clipboard support to me.
> >
>
> I suppose the "now" in "the existing message for sending the clipboard, is
> now sent only in response to REQUEST" means that was changed.
>
> I personally think the code should be readable from a tarball/snapshot not
> pretending people to dig into 10 years of history. I'll try to find some time
> to put these lines into vd_agent.h.
>
> > > The current is: if we get new local clipboard data send the "grab"
> > > so remote knows that will have to read our data.
> >
> > yes
> >
> > > So either we have ownership/grab, meaning the data are remote
> > > waiting to get fetched or we don't have ownership and the remote
> > > should be grabbing as we have data to send (at least in a stable
> > > state).
> >
> >
> > That last sentence is confusing. There are 2 states the client can
> > "have the grab".
> >
> > 1. the client took the grab for the remote data: we are talking about
> > the client app in the client desktop environment
> > 2. the client took the grab, at the protocol level: it sent a grab
> > over the protocol to announce it can provide clipboard data to the
> > remote. In this case, the client app doesn't have the grab in the
> > client desktop environment, but another client application.
> >
> > In general, when talking about the protocol, "client has the grab"
> > means 2) to me, iow it can provide data to the remote.
> >
>
> I think I mean the opposite. The reason is that some application
> have the ownership on either side, when you send a VD_AGENT_CLIPBOARD_GRAB
> the current local application (so spice-gtk/vd_agent) does NOT have the
> ownership and it's asking the remote to take to the application (so will
> remove the ownership from another remote application).
> From how I read the message (VD_AGENT_CLIPBOARD_GRAB) is telling the
> other side to grab (take ownership) of the clipboard so will be the
> remote to have the "grab", not the local.


I find it hard to understand you. The sequence of events is as such.

A & B are client or remote exchangeably:
- an application on A takes the clipboard grab (== announce clipboard
data is available)
- A get notified by the desktop and sends a GRAB message to B side
- B receives GRAB, and takes the clipboard grab on B desktop side

With this sequence, "A" has the clipboard grab at the Spice protocol
level, so to say. It means there is clipboard data on A side.

> >
> > > Not sure what is the initial state, when you connect.
> >
> > Initial state is undefined, and currently whatever the guest or client
> > side state is. Iow, Spice client/agent doen't do anything afaik. They
> > will only react to further grab events.
> >
>
> I suppose the agent will keep its state while the client if was not
> connected get some initial state (boolean variables have to be true or
> false at the end).
>
> > We

Re: [Spice-devel] [PATCH linux/vd-agent 10/11] clipboard: only send release when no immediate grab

Hi

On Thu, Mar 28, 2019 at 5:30 PM Frediano Ziglio  wrote:
>
> >
> > From: Marc-André Lureau 
> >
> > Do not send a release event between two grabs, this helps with window
> > manager interaction issues on peer side.
> >
>
> I would explain which kind of issue this is supposed to fix.

They react to clipboard events in different ways. The point is to
minimize the amount of events to avoid extra unnecessary interactions.

In particular, on release event, some clipboard managers take the
grab. This creates a race with Spice which does a release between
grabs currently.

>
> > Advertise this behaviour via a capability introduced in spice-protocol
> > 0.12.16, so the client doesn't need to do some time-based filtering.
> >
> > (the capability shouldn't need to be negotiated, a client shouldn't
> > expect a release between two grabs)
> >
> > Signed-off-by: Marc-André Lureau 
>
> I suppose Jakub/Victor could do a much better review/test than me.
>
> > ---
> >  src/vdagent/clipboard.c | 12 ++--
> >  src/vdagent/x11.c   |  7 +++
> >  src/vdagentd/vdagentd.c |  1 +
> >  3 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/vdagent/clipboard.c b/src/vdagent/clipboard.c
> > index 9fb87e3..097c6ee 100644
> > --- a/src/vdagent/clipboard.c
> > +++ b/src/vdagent/clipboard.c
> > @@ -242,13 +242,13 @@ static void clipboard_owner_change_cb(GtkClipboard
> > *clipboard,
> >  return;
> >  }
> >
> > -if (sel->owner == OWNER_GUEST) {
> > -clipboard_new_owner(c, sel_id, OWNER_NONE);
> > -udscs_write(c->conn, VDAGENTD_CLIPBOARD_RELEASE, sel_id, 0, NULL,
> > 0);
> > -}
> > -
> > -if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER)
> > +if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
> > +if (sel->owner == OWNER_GUEST) {
> > +clipboard_new_owner(c, sel_id, OWNER_NONE);
> > +udscs_write(c->conn, VDAGENTD_CLIPBOARD_RELEASE, sel_id, 0,
> > NULL, 0);
> > +}
> >  return;
> > +}
> >
> >  /* if there's a pending request for clipboard targets, cancel it */
> >  if (sel->last_targets_req)
> > diff --git a/src/vdagent/x11.c b/src/vdagent/x11.c
> > index c2515a8..9409b53 100644
> > --- a/src/vdagent/x11.c
> > +++ b/src/vdagent/x11.c
> > @@ -530,11 +530,10 @@ static void vdagent_x11_handle_event(struct 
> > vdagent_x11
> > *x11, XEvent event)
> >  if (ev.xfev.owner == x11->selection_window)
> >  return;
> >
> > -/* If the clipboard owner is changed we no longer own it */
>
> Why not keeping this comment?

The comment is no longer valid with this change: if the clipboard
owner is changed, the agent may still own the grab (at the protocol
level).

As you can see with the following line being removed:
>
> > -vdagent_x11_set_clipboard_owner(x11, selection, owner_none);
> > -
> > -if (ev.xfev.owner == None)
> > +if (ev.xfev.owner == None) {
> > +vdagent_x11_set_clipboard_owner(x11, selection, owner_none);
> >  return;
> > +}
> >
> >  /* Request the supported targets from the new owner */
> >  XConvertSelection(x11->display, ev.xfev.selection,
> >  x11->targets_atom,
> > diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> > index 72a3e13..683e5d3 100644
> > --- a/src/vdagentd/vdagentd.c
> > +++ b/src/vdagentd/vdagentd.c
> > @@ -133,6 +133,7 @@ static void send_capabilities(struct vdagent_virtio_port
> > *vport,
> >  VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MAX_CLIPBOARD);
> >  VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_AUDIO_VOLUME_SYNC);
> >  VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_GRAPHICS_DEVICE_INFO);
> > +VD_AGENT_SET_CAPABILITY(caps->caps,
> > VD_AGENT_CAP_CLIPBOARD_NO_RELEASE_ON_REGRAB);
> >  virtio_msg_uint32_to_le((uint8_t *)caps, size, 0);
> >
> >  vdagent_virtio_port_write(vport, VDP_CLIENT_PORT,
>
> Frediano
> ___
> 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 spice-server v2 12/12] display-channel: Inline red_migrate_display function

Acked-by: Jonathon Jongsma 


On Tue, 2019-03-26 at 19:10 +, Frediano Ziglio wrote:
> The only caller only called that function.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/display-channel.c | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 7b833e4c..9f3555e5 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -2633,8 +2633,10 @@ static void
> display_channel_disconnect(RedChannelClient *rcc)
>  red_channel_client_disconnect(rcc);
>  }
>  
> -static void red_migrate_display(DisplayChannel *display,
> RedChannelClient *rcc)
> +static void display_channel_migrate(RedChannelClient *rcc)
>  {
> +DisplayChannel *display =
> DISPLAY_CHANNEL(red_channel_client_get_channel(rcc));
> +
>  /* We need to stop the streams, and to send upgrade_items to the
> client.
>   * Otherwise, (1) the client might display lossy regions that we
> don't track
>   * (streams are not part of the migration data) (2)
> streams_timeout may occur
> @@ -2651,12 +2653,6 @@ static void red_migrate_display(DisplayChannel
> *display, RedChannelClient *rcc)
>  }
>  }
>  
> -static void display_channel_migrate(RedChannelClient *rcc)
> -{
> -DisplayChannel *display =
> DISPLAY_CHANNEL(red_channel_client_get_channel(rcc));
> -red_migrate_display(display, rcc);
> -}
> -
>  void display_channel_set_image_compression(DisplayChannel *display,
> SpiceImageCompression
> image_compression)
>  {

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

Re: [Spice-devel] [PATCH spice-server v2 11/12] Move image_compression field from RedWorker to DisplayChannel

Acked-by: Jonathon Jongsma 


On Tue, 2019-03-26 at 19:10 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/display-channel-private.h |  1 +
>  server/display-channel.c | 10 --
>  server/display-channel.h |  2 ++
>  server/red-worker.c  |  7 +++
>  4 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/server/display-channel-private.h b/server/display-
> channel-private.h
> index 58179531..4cdae8dc 100644
> --- a/server/display-channel-private.h
> +++ b/server/display-channel-private.h
> @@ -87,6 +87,7 @@ struct DisplayChannelPrivate
>  MonitorsConfig *monitors_config;
>  
>  uint32_t renderer;
> +SpiceImageCompression image_compression;
>  int enable_jpeg;
>  int enable_zlib_glz_wrap;
>  
> diff --git a/server/display-channel.c b/server/display-channel.c
> index c2129c71..7b833e4c 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -2273,6 +2273,7 @@ display_channel_init(DisplayChannel *self)
>  /* must be manually allocated here since
> g_type_class_add_private() only
>   * supports structs smaller than 64k */
>  self->priv = g_new0(DisplayChannelPrivate, 1);
> +self->priv->image_compression =
> SPICE_IMAGE_COMPRESSION_AUTO_GLZ;
>  self->priv->pub = self;
>  
>  image_encoder_shared_init(&self->priv->encoder_shared_data);
> @@ -2611,10 +2612,9 @@ display_channel_connect(RedChannel *channel,
> RedClient *client,
>  
>  spice_debug("connect new client");
>  
> -// FIXME not sure how safe is reading directly from reds
>  SpiceServer *reds = red_channel_get_server(channel);
>  dcc = dcc_new(display, client, stream, migration, caps,
> -  spice_server_get_image_compression(reds),
> reds_get_jpeg_state(reds),
> +  display->priv->image_compression,
> reds_get_jpeg_state(reds),
>reds_get_zlib_glz_state(reds));
>  if (!dcc) {
>  return;
> @@ -2656,3 +2656,9 @@ static void
> display_channel_migrate(RedChannelClient *rcc)
>  DisplayChannel *display =
> DISPLAY_CHANNEL(red_channel_client_get_channel(rcc));
>  red_migrate_display(display, rcc);
>  }
> +
> +void display_channel_set_image_compression(DisplayChannel *display,
> +   SpiceImageCompression
> image_compression)
> +{
> +display->priv->image_compression = image_compression;
> +}
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 8dfdf523..6af9e129 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -159,6 +159,8 @@ void
> display_channel_reset_image_cache(DisplayChannel *self);
>  void display_channel_debug_oom(DisplayChannel *display, const char
> *msg);
>  
>  void display_channel_update_qxl_running(DisplayChannel *display,
> bool running);
> +void display_channel_set_image_compression(DisplayChannel *display,
> +   SpiceImageCompression
> image_compression);
>  
>  G_END_DECLS
>  
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 7382e24d..bc63fc34 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -76,8 +76,6 @@ struct RedWorker {
>  
>  RedMemSlotInfo mem_slots;
>  
> -SpiceImageCompression image_compression;
> -
>  uint32_t process_display_generation;
>  RedStatNode stat;
>  RedStatCounter wakeup_counter;
> @@ -684,7 +682,7 @@ static void handle_dev_set_compression(void
> *opaque, void *payload)
>  RedWorker *worker = opaque;
>  SpiceImageCompression image_compression = msg-
> >image_compression;
>  
> -worker->image_compression = image_compression;
> +display_channel_set_image_compression(worker->display_channel,
> image_compression);
>  
>  display_channel_compress_stats_print(worker->display_channel);
>  display_channel_compress_stats_reset(worker->display_channel);
> @@ -1078,7 +1076,6 @@ RedWorker* red_worker_new(QXLInstance *qxl)
>  dispatcher_register_universal_handler(dispatcher,
> worker_dispatcher_record);
>  }
>  
> -worker->image_compression =
> spice_server_get_image_compression(reds);
>  worker->driver_cap_monitors_config = 0;
>  char worker_str[SPICE_STAT_NODE_NAME_MAX];
>  snprintf(worker_str, sizeof(worker_str), "display[%d]", worker-
> >qxl->id & 0xff);
> @@ -1120,6 +1117,8 @@ RedWorker* red_worker_new(QXLInstance *qxl)
>init_info.n_surfac
> es);
>  channel = RED_CHANNEL(worker->display_channel);
>  red_channel_init_stat_node(channel, &worker->stat,
> "display_channel");
> +display_channel_set_image_compression(worker->display_channel,
> +  spice_server_get_image_com
> pression(reds));
>  
>  return worker;
>  }

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

Re: [Spice-devel] [PATCH spice-server v2 10/12] Check image compression value earlier

Acked-by: Jonathon Jongsma 


On Tue, 2019-03-26 at 19:10 +, Frediano Ziglio wrote:
> Do not check it after assigning to reds->config->image_compression,
> check the value as soon as possible.
> This prevent potential invalid settings.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-worker.c | 28 
>  server/reds.c   | 34 +++---
>  2 files changed, 31 insertions(+), 31 deletions(-)
> 
> diff --git a/server/red-worker.c b/server/red-worker.c
> index d883f419..7382e24d 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -684,34 +684,6 @@ static void handle_dev_set_compression(void
> *opaque, void *payload)
>  RedWorker *worker = opaque;
>  SpiceImageCompression image_compression = msg-
> >image_compression;
>  
> -switch (image_compression) {
> -case SPICE_IMAGE_COMPRESSION_AUTO_LZ:
> -spice_debug("ic auto_lz");
> -break;
> -case SPICE_IMAGE_COMPRESSION_AUTO_GLZ:
> -spice_debug("ic auto_glz");
> -break;
> -case SPICE_IMAGE_COMPRESSION_QUIC:
> -spice_debug("ic quic");
> -break;
> -#ifdef USE_LZ4
> -case SPICE_IMAGE_COMPRESSION_LZ4:
> -spice_debug("ic lz4");
> -break;
> -#endif
> -case SPICE_IMAGE_COMPRESSION_LZ:
> -spice_debug("ic lz");
> -break;
> -case SPICE_IMAGE_COMPRESSION_GLZ:
> -spice_debug("ic glz");
> -break;
> -case SPICE_IMAGE_COMPRESSION_OFF:
> -spice_debug("ic off");
> -break;
> -default:
> -spice_warning("ic invalid");
> -image_compression = worker->image_compression;
> -}
>  worker->image_compression = image_compression;
>  
>  display_channel_compress_stats_print(worker->display_channel);
> diff --git a/server/reds.c b/server/reds.c
> index 14e5728b..28542bd0 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3010,12 +3010,40 @@ static inline void
> on_activating_ticketing(RedsState *reds)
>  }
>  }
>  
> -static void reds_config_set_image_compression(RedsState *reds,
> SpiceImageCompression val)
> +static void reds_config_set_image_compression(RedsState *reds,
> SpiceImageCompression image_compression)
>  {
> -if (val == reds->config->image_compression) {
> +if (image_compression == reds->config->image_compression) {
>  return;
>  }
> -reds->config->image_compression = val;
> +switch (image_compression) {
> +case SPICE_IMAGE_COMPRESSION_AUTO_LZ:
> +spice_debug("ic auto_lz");
> +break;
> +case SPICE_IMAGE_COMPRESSION_AUTO_GLZ:
> +spice_debug("ic auto_glz");
> +break;
> +case SPICE_IMAGE_COMPRESSION_QUIC:
> +spice_debug("ic quic");
> +break;
> +#ifdef USE_LZ4
> +case SPICE_IMAGE_COMPRESSION_LZ4:
> +spice_debug("ic lz4");
> +break;
> +#endif
> +case SPICE_IMAGE_COMPRESSION_LZ:
> +spice_debug("ic lz");
> +break;
> +case SPICE_IMAGE_COMPRESSION_GLZ:
> +spice_debug("ic glz");
> +break;
> +case SPICE_IMAGE_COMPRESSION_OFF:
> +spice_debug("ic off");
> +break;
> +default:
> +spice_warning("ic invalid");
> +return;
> +}
> +reds->config->image_compression = image_compression;
>  reds_on_ic_change(reds);
>  }
>  

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

Re: [Spice-devel] [PATCH spice-server v2 09/12] red-worker: Remove only assigned fields

Acked-by: Jonathon Jongsma 



On Tue, 2019-03-26 at 19:10 +, Frediano Ziglio wrote:
> DisplayChannelClient get them directly from reds (they are changed
> only during initialisation so they can be read freely from any
> thread).
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/red-worker.c | 4 
>  1 file changed, 4 deletions(-)
> 
> diff --git a/server/red-worker.c b/server/red-worker.c
> index 129f9f93..d883f419 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -77,8 +77,6 @@ struct RedWorker {
>  RedMemSlotInfo mem_slots;
>  
>  SpiceImageCompression image_compression;
> -spice_wan_compression_t jpeg_state;
> -spice_wan_compression_t zlib_glz_state;
>  
>  uint32_t process_display_generation;
>  RedStatNode stat;
> @@ -1109,8 +1107,6 @@ RedWorker* red_worker_new(QXLInstance *qxl)
>  }
>  
>  worker->image_compression =
> spice_server_get_image_compression(reds);
> -worker->jpeg_state = reds_get_jpeg_state(reds);
> -worker->zlib_glz_state = reds_get_zlib_glz_state(reds);
>  worker->driver_cap_monitors_config = 0;
>  char worker_str[SPICE_STAT_NODE_NAME_MAX];
>  snprintf(worker_str, sizeof(worker_str), "display[%d]", worker-
> >qxl->id & 0xff);

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

Re: [Spice-devel] [PATCH spice-server v2 08/12] cursor-channel: Update some declarations and documentation

This also looks like a patch that should get squashed (maybe parts
should be squashed into two different previous commits, see below), but
I agree with all of the changes.

Acked-by: Jonathon Jongsma 


On Tue, 2019-03-26 at 19:10 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/cursor-channel-client.h |  6 ++
>  server/cursor-channel.c|  2 --
>  server/cursor-channel.h| 17 ++---
>  3 files changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/server/cursor-channel-client.h b/server/cursor-channel-
> client.h
> index 56b3b312..4deae535 100644
> --- a/server/cursor-channel-client.h
> +++ b/server/cursor-channel-client.h
> @@ -75,6 +75,12 @@ enum {
>  RED_PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE,
>  };
>  
> +/**
> + * Migrate a client channel from a CursorChannel.
> + * This is the equivalent of RedChannel client migrate callback.
> + */
> +void cursor_channel_client_migrate(RedChannelClient
> *client);
> +
>  G_END_DECLS

It seems that this hunk and the last should get squashed with patch
06/12


>  
>  #endif /* CURSOR_CHANNEL_CLIENT_H_ */
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index c88e5cd1..d936b791 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -337,8 +337,6 @@ void cursor_channel_set_mouse_mode(CursorChannel
> *cursor, uint32_t mode)
>  
>  /**
>   * Connect a new client to CursorChannel.
> - * This is the equivalent of RedChannel client connect callback.
> - * See comment on cursor_channel_new.
>   */
>  static void
>  cursor_channel_connect(CursorChannel *cursor, RedClient *client,
> RedStream *stream,
> diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> index ce1b92cc..dc48279a 100644
> --- a/server/cursor-channel.h
> +++ b/server/cursor-channel.h
> @@ -48,14 +48,8 @@ GType cursor_channel_get_type(void) G_GNUC_CONST;
>  /**
>   * Create CursorChannel.
>   * Since CursorChannel is intended to be run in a separate thread,
> - * it does not register its own client callbacks since they would
> - * be called from a different thread. Therefore users of this
> - * class are responsible for registering their own client callbacks
> - * for CursorChannel. These 'wrapper' client callbacks must forward
> - * execution on to the CursorChannel thread.
> - * cursor_channel_client_migrate() and cursor_channel_connect() are
> - * provided as helper functions and should only be called from the
> - * CursorChannel thread.
> + * the function accepts a dispatcher parameter to allows some
> + * operations to be executed in the channel thread.
>   */
>  CursorChannel* cursor_channel_new(RedsState *server, int id,
>const SpiceCoreInterfaceInternal
> *core,

I think these hunks should be squashed into patch 02/12


> @@ -66,13 +60,6 @@
> void cursor_channel_do_init (CursorChannel
> *cursor);
>  void cursor_channel_process_cmd (CursorChannel
> *cursor, RedCursorCmd *cursor_cmd);
>  void cursor_channel_set_mouse_mode(CursorChannel
> *cursor, uint32_t mode);
>  
> -/**
> - * Migrate a client channel from a CursorChannel.
> - * This is the equivalent of RedChannel client migrate callback.
> - * See comment on cursor_channel_new.
> - */
> -void cursor_channel_client_migrate(RedChannelClient
> *client);
> -
>  G_END_DECLS
>  
>  #endif /* CURSOR_CHANNEL_H_ */


Patch 06/12

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

Re: [Spice-devel] [PATCH spice-server v2 07/12] Make some function static

Personally, I would squash this patch with the previous one

Acked-by: Jonathon Jongsma 


On Tue, 2019-03-26 at 19:10 +, Frediano Ziglio wrote:
> Now that stuff are a bit more on their correct place some
> function can be static.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/cursor-channel.c  | 11 ---
>  server/cursor-channel.h  | 10 --
>  server/display-channel.c | 12 +---
>  server/display-channel.h |  5 -
>  4 files changed, 17 insertions(+), 21 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index d1294322..c88e5cd1 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -335,9 +335,14 @@ void cursor_channel_set_mouse_mode(CursorChannel
> *cursor, uint32_t mode)
>  cursor->mouse_mode = mode;
>  }
>  
> -void cursor_channel_connect(CursorChannel *cursor, RedClient
> *client, RedStream *stream,
> -int migrate,
> -RedChannelCapabilities *caps)
> +/**
> + * Connect a new client to CursorChannel.
> + * This is the equivalent of RedChannel client connect callback.
> + * See comment on cursor_channel_new.
> + */
> +static void
> +cursor_channel_connect(CursorChannel *cursor, RedClient *client,
> RedStream *stream,
> +   int migrate, RedChannelCapabilities *caps)
>  {
>  CursorChannelClient *ccc;
>  
> diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> index 767325ea..ce1b92cc 100644
> --- a/server/cursor-channel.h
> +++ b/server/cursor-channel.h
> @@ -66,16 +66,6 @@
> void cursor_channel_do_init (CursorChannel
> *cursor);
>  void cursor_channel_process_cmd (CursorChannel
> *cursor, RedCursorCmd *cursor_cmd);
>  void cursor_channel_set_mouse_mode(CursorChannel
> *cursor, uint32_t mode);
>  
> -/**
> - * Connect a new client to CursorChannel.
> - * This is the equivalent of RedChannel client connect callback.
> - * See comment on cursor_channel_new.
> - */
> -void cursor_channel_connect (CursorChannel
> *cursor, RedClient *client,
> - RedStream *stream,
> - int migrate,
> - RedChannelCapabilit
> ies *caps);
> -
>  /**
>   * Migrate a client channel from a CursorChannel.
>   * This is the equivalent of RedChannel client migrate callback.
> diff --git a/server/display-channel.c b/server/display-channel.c
> index b8f85682..c2129c71 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -26,6 +26,12 @@
>  
>  G_DEFINE_TYPE(DisplayChannel, display_channel,
> TYPE_COMMON_GRAPHICS_CHANNEL)
>  
> +static void display_channel_connect(RedChannel *channel, RedClient
> *client,
> +RedStream *stream, int
> migration,
> +RedChannelCapabilities *caps);
> +static void display_channel_disconnect(RedChannelClient *rcc);
> +static void display_channel_migrate(RedChannelClient *rcc);
> +
>  enum {
>  PROP0,
>  PROP_N_SURFACES,
> @@ -2595,7 +2601,7 @@ void
> display_channel_update_qxl_running(DisplayChannel *display, bool
> running)
>  }
>  }
>  
> -void
> +static void
>  display_channel_connect(RedChannel *channel, RedClient *client,
>  RedStream *stream, int migration,
>  RedChannelCapabilities *caps)
> @@ -2618,7 +2624,7 @@ display_channel_connect(RedChannel *channel,
> RedClient *client,
>  dcc_start(dcc);
>  }
>  
> -void display_channel_disconnect(RedChannelClient *rcc)
> +static void display_channel_disconnect(RedChannelClient *rcc)
>  {
>  DisplayChannel *display =
> DISPLAY_CHANNEL(red_channel_client_get_channel(rcc));
>  
> @@ -2645,7 +2651,7 @@ static void red_migrate_display(DisplayChannel
> *display, RedChannelClient *rcc)
>  }
>  }
>  
> -void display_channel_migrate(RedChannelClient *rcc)
> +static void display_channel_migrate(RedChannelClient *rcc)
>  {
>  DisplayChannel *display =
> DISPLAY_CHANNEL(red_channel_client_get_channel(rcc));
>  red_migrate_display(display, rcc);
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 7dfedd75..8dfdf523 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -159,11 +159,6 @@ void
> display_channel_reset_image_cache(DisplayChannel *self);
>  void display_channel_debug_oom(DisplayChannel *display, const char
> *msg);
>  
>  void display_channel_update_qxl_running(DisplayChannel *display,
> bool running);
> -void display_channel_connect(RedChannel *channel, RedClient *client,
> - RedStream *stream, int migration,
> - RedChannelCapabilities *caps);
> -void display_channel_disconnect(RedChannelClient *rcc);
> -void display_channel_migrate(RedChannelClient *rcc);
>  
>  G_END_DECLS
>  

__

Re: [Spice-devel] [PATCH spice-server v2 05/12] Check running state in red_qxl_set_client_capabilities

Acked-by: Jonathon Jongsma 


On Tue, 2019-03-26 at 19:10 +, Frediano Ziglio wrote:
> No reasons to expose red_qxl_is_running, this was used to not
> send capability is the state was not running.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/display-channel.c | 3 ---
>  server/red-qxl.c | 5 -
>  server/red-qxl.h | 3 ---
>  server/red-worker.h  | 1 +
>  4 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 1af87ba4..e9368668 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -2566,9 +2566,6 @@ static void
> guest_set_client_capabilities(DisplayChannel *display)
>  #define
> CLEAR_CAP(a,c)  \
>  ((a)[(c) / 8] &= ~(1 << ((c) % 8)))
>  
> -if (!red_qxl_is_running(display->priv->qxl)) {
> -return;
> -}
>  if ((red_channel_get_n_clients(RED_CHANNEL(display)) == 0)) {
>  red_qxl_set_client_capabilities(display->priv->qxl, FALSE,
> caps);
>  } else {
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index e3fbf7b7..6dbd224c 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -68,6 +68,7 @@ struct QXLState {
>  
>  #define GL_DRAW_COOKIE_INVALID (~((uint64_t) 0))
>  
> +/* used by RedWorker */
>  bool red_qxl_is_running(QXLInstance *qxl)
>  {
>  return qxl->st->running;
> @@ -1048,7 +1049,9 @@ void
> red_qxl_set_client_capabilities(QXLInstance *qxl,
>  {
>  QXLInterface *interface = qxl_get_interface(qxl);
>  
> -interface->set_client_capabilities(qxl, client_present, caps);
> +if (qxl->st->running) {
> +interface->set_client_capabilities(qxl, client_present,
> caps);
> +}
>  }
>  
>  void red_qxl_async_complete(QXLInstance *qxl, uint64_t cookie)
> diff --git a/server/red-qxl.h b/server/red-qxl.h
> index 521f3659..94753948 100644
> --- a/server/red-qxl.h
> +++ b/server/red-qxl.h
> @@ -44,9 +44,6 @@ const char* red_qxl_get_device_address(const
> QXLInstance *qxl);
>  const uint32_t* red_qxl_get_device_display_ids(const QXLInstance
> *qxl);
>  size_t red_qxl_get_monitors_count(const QXLInstance *qxl);
>  
> -/* check if QXL is running, should be used inside the worker thread
> */
> -bool red_qxl_is_running(QXLInstance *qxl);
> -
>  /* Wrappers around QXLInterface vfuncs */
>  void red_qxl_get_init_info(QXLInstance *qxl, QXLDevInitInfo *info);
>  int red_qxl_get_command(QXLInstance *qxl, struct QXLCommandExt
> *cmd);
> diff --git a/server/red-worker.h b/server/red-worker.h
> index 54ab4da8..34c5b4af 100644
> --- a/server/red-worker.h
> +++ b/server/red-worker.h
> @@ -35,6 +35,7 @@ void red_worker_free(RedWorker *worker);
>  struct Dispatcher *red_qxl_get_dispatcher(QXLInstance *qxl);
>  void red_qxl_destroy_primary_surface_complete(QXLState *qxl_state);
>  void red_qxl_create_primary_surface_complete(QXLState *qxl_state,
> const QXLDevSurfaceCreate* surface);
> +bool red_qxl_is_running(QXLInstance *qxl);
>  void red_qxl_set_running(QXLInstance *qxl, bool running);
>  
>  typedef uint32_t RedWorkerMessage;

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

Re: [Spice-devel] [PATCH spice-server v2 04/12] Move DisplayChannel callbacks from RedWorker to DisplayChannel

Acked-by: Jonathon Jongsma 


On Tue, 2019-03-26 at 19:10 +, Frediano Ziglio wrote:
> Signed-off-by: Frediano Ziglio 
> ---
>  server/display-channel.c | 108 +
>  server/display-channel.h |   7 +++
>  server/red-worker.c  | 114 ++---
> --
>  3 files changed, 120 insertions(+), 109 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index cb052bfc..1af87ba4 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -2540,3 +2540,111 @@ void display_channel_debug_oom(DisplayChannel
> *display, const char *msg)
>  ring_get_length(&display->priv->current_list),
>  red_channel_sum_pipes_size(channel));
>  }
> +
> +static void guest_set_client_capabilities(DisplayChannel *display)
> +{
> +int i;
> +RedChannelClient *rcc;
> +uint8_t caps[SPICE_CAPABILITIES_SIZE] = { 0 };
> +int caps_available[] = {
> +SPICE_DISPLAY_CAP_SIZED_STREAM,
> +SPICE_DISPLAY_CAP_MONITORS_CONFIG,
> +SPICE_DISPLAY_CAP_COMPOSITE,
> +SPICE_DISPLAY_CAP_A8_SURFACE,
> +};
> +QXLInterface *qif = qxl_get_interface(display->priv->qxl);
> +
> +if (!red_qxl_check_qxl_version(display->priv->qxl, 3, 2)) {
> +return;
> +}
> +if (!qif->set_client_capabilities) {
> +return;
> +}
> +#define
> SET_CAP(a,c)\
> +((a)[(c) / 8] |= (1 << ((c) % 8)))
> +
> +#define
> CLEAR_CAP(a,c)  \
> +((a)[(c) / 8] &= ~(1 << ((c) % 8)))
> +
> +if (!red_qxl_is_running(display->priv->qxl)) {
> +return;
> +}
> +if ((red_channel_get_n_clients(RED_CHANNEL(display)) == 0)) {
> +red_qxl_set_client_capabilities(display->priv->qxl, FALSE,
> caps);
> +} else {
> +// Take least common denominator
> +for (i = 0 ; i < SPICE_N_ELEMENTS(caps_available); ++i) {
> +SET_CAP(caps, caps_available[i]);
> +}
> +FOREACH_CLIENT(display, rcc) {
> +for (i = 0 ; i < SPICE_N_ELEMENTS(caps_available); ++i)
> {
> +if (!red_channel_client_test_remote_cap(rcc,
> caps_available[i]))
> +CLEAR_CAP(caps, caps_available[i]);
> +}
> +}
> +red_qxl_set_client_capabilities(display->priv->qxl, TRUE,
> caps);
> +}
> +}
> +
> +void display_channel_update_qxl_running(DisplayChannel *display,
> bool running)
> +{
> +if (running) {
> +guest_set_client_capabilities(display);
> +}
> +}
> +
> +void
> +display_channel_connect(RedChannel *channel, RedClient *client,
> +RedStream *stream, int migration,
> +RedChannelCapabilities *caps)
> +{
> +DisplayChannel *display = DISPLAY_CHANNEL(channel);
> +DisplayChannelClient *dcc;
> +
> +spice_debug("connect new client");
> +
> +// FIXME not sure how safe is reading directly from reds
> +SpiceServer *reds = red_channel_get_server(channel);
> +dcc = dcc_new(display, client, stream, migration, caps,
> +  spice_server_get_image_compression(reds),
> reds_get_jpeg_state(reds),
> +  reds_get_zlib_glz_state(reds));
> +if (!dcc) {
> +return;
> +}
> +display_channel_update_compression(display, dcc);
> +guest_set_client_capabilities(display);
> +dcc_start(dcc);
> +}
> +
> +void display_channel_disconnect(RedChannelClient *rcc)
> +{
> +DisplayChannel *display =
> DISPLAY_CHANNEL(red_channel_client_get_channel(rcc));
> +
> +guest_set_client_capabilities(display);
> +
> +red_channel_client_disconnect(rcc);
> +}
> +
> +static void red_migrate_display(DisplayChannel *display,
> RedChannelClient *rcc)
> +{
> +/* We need to stop the streams, and to send upgrade_items to the
> client.
> + * Otherwise, (1) the client might display lossy regions that we
> don't track
> + * (streams are not part of the migration data) (2)
> streams_timeout may occur
> + * after the MIGRATE message has been sent. This can result in
> messages
> + * being sent to the client after MSG_MIGRATE and before
> MSG_MIGRATE_DATA (e.g.,
> + * STREAM_CLIP, STREAM_DESTROY, DRAW_COPY)
> + * No message besides MSG_MIGRATE_DATA should be sent after
> MSG_MIGRATE.
> + * Notice that detach_and_stop_streams won't lead to any dev ram
> changes, since
> + * handle_dev_stop already took care of releasing all the dev
> ram resources.
> + */
> +video_stream_detach_and_stop(display);
> +if (red_channel_client_is_connected(rcc)) {
> +red_channel_client_default_migrate(rcc);
> +}
> +}
> +
> +void display_channel_migrate(RedChannelClient *rcc)
> +{
> +DisplayChannel *display =
> DISPLAY_CHANNEL(red_channel_client_get_channel(rcc));
> +red_migrate_display(display, rcc);
> +}
> diff --git a/server/display-channel.h b/s

Re: [Spice-devel] [spice-common 2/8] backtrace: Add missing include

> 
> This fixes a warning about missing prototype for backtrace()
> 
> Signed-off-by: Christophe Fergeau 

Acked-by: Frediano Ziglio 

> ---
>  common/backtrace.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/common/backtrace.c b/common/backtrace.c
> index c4edde1..ff72d1b 100644
> --- a/common/backtrace.c
> +++ b/common/backtrace.c
> @@ -25,6 +25,8 @@
>  #include 
>  #endif
>  
> +#include "backtrace.h"
> +
>  #include 
>  #include 
>  #include 

Not sure we updated the style document with this style but
fine for me.

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

Re: [Spice-devel] [spice-common 7/8] quic: Fix QUIC_VERSION definition

> 
> QUIC_VERSION_MINOR is never used.. Set QUIC_VERSION_MINOR to the same
> version as QUIC_VERSION_MAJOR to avoid breaking backwards compatibility,
> and fix the QUIC_VERSION macro.
> 
> Signed-off-by: Christophe Fergeau 

Acked-by: Frediano Ziglio 

> ---
>  common/quic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/common/quic.c b/common/quic.c
> index 1760274..f91b23f 100644
> --- a/common/quic.c
> +++ b/common/quic.c
> @@ -31,8 +31,8 @@
>  /* ASCII "QUIC" */
>  #define QUIC_MAGIC 0x43495551
>  #define QUIC_VERSION_MAJOR 0U
> -#define QUIC_VERSION_MINOR 1U
> -#define QUIC_VERSION ((QUIC_VERSION_MAJOR << 16) | (QUIC_VERSION_MAJOR &
> 0x))
> +#define QUIC_VERSION_MINOR 0U
> +#define QUIC_VERSION ((QUIC_VERSION_MAJOR << 16) | (QUIC_VERSION_MINOR &
> 0x))
>  
>  typedef uint8_t BYTE;
>  

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

Re: [Spice-devel] [spice-common 8/8] log: Let gcc know about the logging macros which abort

> 
> The for(;;) hack was taken from glib's logging macros.
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  common/log.h | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/common/log.h b/common/log.h
> index 7c67e7a..b397306 100644
> --- a/common/log.h
> +++ b/common/log.h
> @@ -20,6 +20,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -42,6 +43,7 @@ void spice_log(GLogLevelFlags log_level,
>  #define spice_return_if_fail(x) G_STMT_START {  \
>  if G_LIKELY(x) { } else {   \
>  spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, G_STRFUNC, "condition
>  `%s' failed", #x); \
> +abort();
> \
>  return; \
>  }   \
>  } G_STMT_END
> @@ -49,6 +51,7 @@ void spice_log(GLogLevelFlags log_level,
>  #define spice_return_val_if_fail(x, val) G_STMT_START { \
>  if G_LIKELY(x) { } else {   \
>  spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__,
>  "condition `%s' failed", #x); \
> +abort();
> \
>  return (val);   \
>  }   \
>  } G_STMT_END
> @@ -69,12 +72,17 @@ void spice_log(GLogLevelFlags log_level,
>  spice_log(G_LOG_LEVEL_WARNING, SPICE_STRLOC, __FUNCTION__, "" format, ##
>  __VA_ARGS__); \
>  } G_STMT_END
>  
> +/* for(;;) ; so that GCC knows that control doesn't go past g_error().

g_error? copy&paste error?

> + * Put space before ending semicolon to avoid C++ build warnings.
> + */
>  #define spice_critical(format, ...) G_STMT_START {
>  \
>  spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, "" format,
>  ## __VA_ARGS__); \
> +for (;;) ;

I suppose you can use "for(;;) continue;" and remove the comment (that
"continue" was an old suggestion I had, I agree with C++ warning like
I agreed at that time with the continue).

Why some are for and some abort?

> \
>  } G_STMT_END
>  
>  #define spice_error(format, ...) G_STMT_START { \
>  spice_log(G_LOG_LEVEL_ERROR, SPICE_STRLOC, __FUNCTION__, "" format, ##
>  __VA_ARGS__); \
> +for (;;) ;
> \
>  } G_STMT_END
>  
>  #define spice_warn_if_fail(x) G_STMT_START {\

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

Re: [Spice-devel] [spice-common 5/8] build: Update verify.h to latest version

> 
> Signed-off-by: Christophe Fergeau 
> ---
>  common/verify.h | 24 +++-
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/common/verify.h b/common/verify.h
> index 267de29..b2e5f64 100644
> --- a/common/verify.h
> +++ b/common/verify.h
> @@ -1,19 +1,19 @@
>  /* Compile-time assert-like macros.
>  
> -   Copyright (C) 2005-2006, 2009-2016 Free Software Foundation, Inc.
> +   Copyright (C) 2005-2006, 2009-2019 Free Software Foundation, Inc.
>  
> This program is free software: you can redistribute it and/or modify
> -   it under the terms of the GNU Lesser General Public License as published
> by
> -   the Free Software Foundation; either version 2.1 of the License, or
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> (at your option) any later version.
>  
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> -   GNU Lesser General Public License for more details.
> +   GNU General Public License for more details.
>  

What ?? Sure about this?

> -   You should have received a copy of the GNU Lesser General Public License
> -   along with this program.  If not, see .  */
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see .
> */
>  
>  /* Written by Paul Eggert, Bruno Haible, and Jim Meyering.  */
>  
> @@ -248,7 +248,12 @@ template 
>  /* Verify requirement R at compile-time, as a declaration without a
> trailing ';'.  */
>  
> -#define verify(R) _GL_VERIFY (R, "verify (" #R ")")
> +#ifdef __GNUC__
> +# define verify(R) _GL_VERIFY (R, "verify (" #R ")")
> +#else
> +/* PGI barfs if R is long.  Play it safe.  */
> +# define verify(R) _GL_VERIFY (R, "verify (...)")
> +#endif
>  
>  #ifndef __has_builtin
>  # define __has_builtin(x) 0
> @@ -263,7 +268,7 @@ template 
>  # define assume(R) ((R) ? (void) 0 : __builtin_unreachable ())
>  #elif 1200 <= _MSC_VER
>  # define assume(R) __assume (R)
> -#elif (defined lint \
> +#elif ((defined GCC_LINT || defined lint) \
> && (__has_builtin (__builtin_trap) \
> || 3 < __GNUC__ + (3 < __GNUC_MINOR__ + (4 <=
> || __GNUC_PATCHLEVEL__
>/* Doing it this way helps various packages when configured with
> @@ -271,7 +276,8 @@ template 
>   when 'assume' silences warnings even with older GCCs.  */
>  # define assume(R) ((R) ? (void) 0 : __builtin_trap ())
>  #else
> -# define assume(R) ((void) (0 && (R)))
> +  /* Some tools grok NOTREACHED, e.g., Oracle Studio 12.6.  */
> +# define assume(R) ((R) ? (void) 0 : /*NOTREACHED*/ (void) 0)
>  #endif
>  
>  /* @assert.h omit end@  */

As far as I know I would say nack.

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

Re: [Spice-devel] [spice-common 3/8] lz: Don't try to print uninitialized variable

> 
> encoder->type is only going to be set by lz_set_sizes() after the
> error() call. We can use 'type' directly which is what encoder->type is
> going to be set to.
> 
> Signed-off-by: Christophe Fergeau 

Acked-by: Frediano Ziglio 

> ---
>  common/lz.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/lz.c b/common/lz.c
> index 167e118..f92c638 100644
> --- a/common/lz.c
> +++ b/common/lz.c
> @@ -616,7 +616,7 @@ void lz_decode_begin(LzContext *lz, uint8_t *io_ptr,
> unsigned int num_io_bytes,
>  
>  int type = decode_32(encoder);
>  if (type <= LZ_IMAGE_TYPE_INVALID || type > LZ_IMAGE_TYPE_A8) {
> -encoder->usr->error(encoder->usr, "invalid lz type %d\n",
> encoder->type);
> +encoder->usr->error(encoder->usr, "invalid lz type %d\n", type);
>  }
>  int width = decode_32(encoder);
>  int height = decode_32(encoder);
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [spice-common 6/8] test-marshallers: Fix header guard

> 
> test-marshallers.h is missing a #define _H_TEST_MARSHALLERS in order to
> prevent multiple #include for the same header.
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  tests/test-marshallers.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/test-marshallers.h b/tests/test-marshallers.h
> index 4eab90f..5b113b7 100644
> --- a/tests/test-marshallers.h
> +++ b/tests/test-marshallers.h
> @@ -1,6 +1,7 @@
>  #include 
>  

OT: why this is outside the guard?

>  #ifndef _H_TEST_MARSHALLERS
> +#define _H_TEST_MARSHALLERS
>  

OT 2: Why not H_SPICE_COMMON_ prefix?

>  typedef struct {
>  uint32_t data_size;

beside, 

acked

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

Re: [Spice-devel] [spice-common 4/8] build: Add missing G_GNUC_PRINTF annotations

> 
> They were suggested by gcc when using -Wsuggest-attribute=format
> 
> Signed-off-by: Christophe Fergeau 
> ---
>  common/log.c | 13 +++--
>  tests/test-logging.c |  1 +
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/common/log.c b/common/log.c
> index b73da71..7cb3e36 100644
> --- a/common/log.c
> +++ b/common/log.c
> @@ -33,12 +33,13 @@ SPICE_CONSTRUCTOR_FUNC(spice_log_init)
>  recorder_dump_on_common_signals(0, 0);
>  }
>  
> -static void spice_logv(const char *log_domain,
> -   GLogLevelFlags log_level,
> -   const char *strloc,
> -   const char *function,
> -   const char *format,
> -   va_list args)
> +static G_GNUC_PRINTF(5, 0)
> +void spice_logv(const char *log_domain,
> +GLogLevelFlags log_level,
> +const char *strloc,
> +const char *function,
> +const char *format,
> +va_list args)
>  {
>  GString *log_msg;
>  
> diff --git a/tests/test-logging.c b/tests/test-logging.c
> index 6a79ca9..32b0c33 100644
> --- a/tests/test-logging.c
> +++ b/tests/test-logging.c
> @@ -27,6 +27,7 @@
>  
>  #define OTHER_LOG_DOMAIN "Other"
>  #define LOG_OTHER_HELPER(suffix, level)
>  \
> +G_GNUC_PRINTF(1, 2)
> \
>  static void G_PASTE(other_, suffix)(const gchar *format, ...)
>  \
>  {
>  \
>  va_list args;
>  \

Before the static or after the static? That is the question! :-)
(2 hunks, one with one style the other with the other)

Beside that it's fine.

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

[Spice-devel] [spice-common 2/8] backtrace: Add missing include

This fixes a warning about missing prototype for backtrace()

Signed-off-by: Christophe Fergeau 
---
 common/backtrace.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/backtrace.c b/common/backtrace.c
index c4edde1..ff72d1b 100644
--- a/common/backtrace.c
+++ b/common/backtrace.c
@@ -25,6 +25,8 @@
 #include 
 #endif
 
+#include "backtrace.h"
+
 #include 
 #include 
 #include 
-- 
2.21.0

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

[Spice-devel] [spice-common 6/8] test-marshallers: Fix header guard

test-marshallers.h is missing a #define _H_TEST_MARSHALLERS in order to
prevent multiple #include for the same header.

Signed-off-by: Christophe Fergeau 
---
 tests/test-marshallers.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/test-marshallers.h b/tests/test-marshallers.h
index 4eab90f..5b113b7 100644
--- a/tests/test-marshallers.h
+++ b/tests/test-marshallers.h
@@ -1,6 +1,7 @@
 #include 
 
 #ifndef _H_TEST_MARSHALLERS
+#define _H_TEST_MARSHALLERS
 
 typedef struct {
 uint32_t data_size;
-- 
2.21.0

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

[Spice-devel] [spice-common 1/8] canvas_base: Fix variable shadowing warning

canvas_base.c is #included by spice-common users. They currently don't
enable this warning, but if/when they do, we don't want code from
spice-common to trigger it.

Signed-off-by: Christophe Fergeau 
---
 common/canvas_base.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/common/canvas_base.c b/common/canvas_base.c
index 9ffca3e..00a8801 100644
--- a/common/canvas_base.c
+++ b/common/canvas_base.c
@@ -1880,11 +1880,11 @@ static void canvas_clip_pixman(CanvasBase *canvas,
 uint32_t n = clip->rects->num_rects;
 SpiceRect *now = clip->rects->rects;
 
-pixman_region32_t clip;
+pixman_region32_t pixman_clip;
 
-if (spice_pixman_region32_init_rects(&clip, now, n)) {
-pixman_region32_intersect(dest_region, dest_region, &clip);
-pixman_region32_fini(&clip);
+if (spice_pixman_region32_init_rects(&pixman_clip, now, n)) {
+pixman_region32_intersect(dest_region, dest_region, &pixman_clip);
+pixman_region32_fini(&pixman_clip);
 }
 
 break;
-- 
2.21.0

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

[Spice-devel] [spice-common 8/8] log: Let gcc know about the logging macros which abort

The for(;;) hack was taken from glib's logging macros.

Signed-off-by: Christophe Fergeau 
---
 common/log.h | 8 
 1 file changed, 8 insertions(+)

diff --git a/common/log.h b/common/log.h
index 7c67e7a..b397306 100644
--- a/common/log.h
+++ b/common/log.h
@@ -20,6 +20,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -42,6 +43,7 @@ void spice_log(GLogLevelFlags log_level,
 #define spice_return_if_fail(x) G_STMT_START {  \
 if G_LIKELY(x) { } else {   \
 spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, G_STRFUNC, "condition 
`%s' failed", #x); \
+abort();   
\
 return; \
 }   \
 } G_STMT_END
@@ -49,6 +51,7 @@ void spice_log(GLogLevelFlags log_level,
 #define spice_return_val_if_fail(x, val) G_STMT_START { \
 if G_LIKELY(x) { } else {   \
 spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, "condition 
`%s' failed", #x); \
+abort();   
   \
 return (val);   \
 }   \
 } G_STMT_END
@@ -69,12 +72,17 @@ void spice_log(GLogLevelFlags log_level,
 spice_log(G_LOG_LEVEL_WARNING, SPICE_STRLOC, __FUNCTION__, "" format, ## 
__VA_ARGS__); \
 } G_STMT_END
 
+/* for(;;) ; so that GCC knows that control doesn't go past g_error().
+ * Put space before ending semicolon to avoid C++ build warnings.
+ */
 #define spice_critical(format, ...) G_STMT_START {  \
 spice_log(G_LOG_LEVEL_CRITICAL, SPICE_STRLOC, __FUNCTION__, "" format, ## 
__VA_ARGS__); \
+for (;;) ; 
 \
 } G_STMT_END
 
 #define spice_error(format, ...) G_STMT_START { \
 spice_log(G_LOG_LEVEL_ERROR, SPICE_STRLOC, __FUNCTION__, "" format, ## 
__VA_ARGS__); \
+for (;;) ; 
 \
 } G_STMT_END
 
 #define spice_warn_if_fail(x) G_STMT_START {\
-- 
2.21.0

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

[Spice-devel] [spice-common 0/8] warning fixes

Hey,

This is the first set of hopefully more patches, long story short, I
wanted to add manywarnings.m4 to spice-common, but its compilation is
not fully clean at the moment. Here are patches for some of the warnings
I got when trying to add support for it.

Christophe




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

[Spice-devel] [spice-common 3/8] lz: Don't try to print uninitialized variable

encoder->type is only going to be set by lz_set_sizes() after the
error() call. We can use 'type' directly which is what encoder->type is
going to be set to.

Signed-off-by: Christophe Fergeau 
---
 common/lz.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/lz.c b/common/lz.c
index 167e118..f92c638 100644
--- a/common/lz.c
+++ b/common/lz.c
@@ -616,7 +616,7 @@ void lz_decode_begin(LzContext *lz, uint8_t *io_ptr, 
unsigned int num_io_bytes,
 
 int type = decode_32(encoder);
 if (type <= LZ_IMAGE_TYPE_INVALID || type > LZ_IMAGE_TYPE_A8) {
-encoder->usr->error(encoder->usr, "invalid lz type %d\n", 
encoder->type);
+encoder->usr->error(encoder->usr, "invalid lz type %d\n", type);
 }
 int width = decode_32(encoder);
 int height = decode_32(encoder);
-- 
2.21.0

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

[Spice-devel] [spice-common 7/8] quic: Fix QUIC_VERSION definition

QUIC_VERSION_MINOR is never used.. Set QUIC_VERSION_MINOR to the same
version as QUIC_VERSION_MAJOR to avoid breaking backwards compatibility,
and fix the QUIC_VERSION macro.

Signed-off-by: Christophe Fergeau 
---
 common/quic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/quic.c b/common/quic.c
index 1760274..f91b23f 100644
--- a/common/quic.c
+++ b/common/quic.c
@@ -31,8 +31,8 @@
 /* ASCII "QUIC" */
 #define QUIC_MAGIC 0x43495551
 #define QUIC_VERSION_MAJOR 0U
-#define QUIC_VERSION_MINOR 1U
-#define QUIC_VERSION ((QUIC_VERSION_MAJOR << 16) | (QUIC_VERSION_MAJOR & 
0x))
+#define QUIC_VERSION_MINOR 0U
+#define QUIC_VERSION ((QUIC_VERSION_MAJOR << 16) | (QUIC_VERSION_MINOR & 
0x))
 
 typedef uint8_t BYTE;
 
-- 
2.21.0

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

[Spice-devel] [spice-common 5/8] build: Update verify.h to latest version

Signed-off-by: Christophe Fergeau 
---
 common/verify.h | 24 +++-
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/common/verify.h b/common/verify.h
index 267de29..b2e5f64 100644
--- a/common/verify.h
+++ b/common/verify.h
@@ -1,19 +1,19 @@
 /* Compile-time assert-like macros.
 
-   Copyright (C) 2005-2006, 2009-2016 Free Software Foundation, Inc.
+   Copyright (C) 2005-2006, 2009-2019 Free Software Foundation, Inc.
 
This program is free software: you can redistribute it and/or modify
-   it under the terms of the GNU Lesser General Public License as published by
-   the Free Software Foundation; either version 2.1 of the License, or
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
(at your option) any later version.
 
This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-   GNU Lesser General Public License for more details.
+   GNU General Public License for more details.
 
-   You should have received a copy of the GNU Lesser General Public License
-   along with this program.  If not, see .  */
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see .  */
 
 /* Written by Paul Eggert, Bruno Haible, and Jim Meyering.  */
 
@@ -248,7 +248,12 @@ template 
 /* Verify requirement R at compile-time, as a declaration without a
trailing ';'.  */
 
-#define verify(R) _GL_VERIFY (R, "verify (" #R ")")
+#ifdef __GNUC__
+# define verify(R) _GL_VERIFY (R, "verify (" #R ")")
+#else
+/* PGI barfs if R is long.  Play it safe.  */
+# define verify(R) _GL_VERIFY (R, "verify (...)")
+#endif
 
 #ifndef __has_builtin
 # define __has_builtin(x) 0
@@ -263,7 +268,7 @@ template 
 # define assume(R) ((R) ? (void) 0 : __builtin_unreachable ())
 #elif 1200 <= _MSC_VER
 # define assume(R) __assume (R)
-#elif (defined lint \
+#elif ((defined GCC_LINT || defined lint) \
&& (__has_builtin (__builtin_trap) \
|| 3 < __GNUC__ + (3 < __GNUC_MINOR__ + (4 <= 
__GNUC_PATCHLEVEL__
   /* Doing it this way helps various packages when configured with
@@ -271,7 +276,8 @@ template 
  when 'assume' silences warnings even with older GCCs.  */
 # define assume(R) ((R) ? (void) 0 : __builtin_trap ())
 #else
-# define assume(R) ((void) (0 && (R)))
+  /* Some tools grok NOTREACHED, e.g., Oracle Studio 12.6.  */
+# define assume(R) ((R) ? (void) 0 : /*NOTREACHED*/ (void) 0)
 #endif
 
 /* @assert.h omit end@  */
-- 
2.21.0

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

[Spice-devel] [spice-common 4/8] build: Add missing G_GNUC_PRINTF annotations

They were suggested by gcc when using -Wsuggest-attribute=format

Signed-off-by: Christophe Fergeau 
---
 common/log.c | 13 +++--
 tests/test-logging.c |  1 +
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/common/log.c b/common/log.c
index b73da71..7cb3e36 100644
--- a/common/log.c
+++ b/common/log.c
@@ -33,12 +33,13 @@ SPICE_CONSTRUCTOR_FUNC(spice_log_init)
 recorder_dump_on_common_signals(0, 0);
 }
 
-static void spice_logv(const char *log_domain,
-   GLogLevelFlags log_level,
-   const char *strloc,
-   const char *function,
-   const char *format,
-   va_list args)
+static G_GNUC_PRINTF(5, 0)
+void spice_logv(const char *log_domain,
+GLogLevelFlags log_level,
+const char *strloc,
+const char *function,
+const char *format,
+va_list args)
 {
 GString *log_msg;
 
diff --git a/tests/test-logging.c b/tests/test-logging.c
index 6a79ca9..32b0c33 100644
--- a/tests/test-logging.c
+++ b/tests/test-logging.c
@@ -27,6 +27,7 @@
 
 #define OTHER_LOG_DOMAIN "Other"
 #define LOG_OTHER_HELPER(suffix, level)
  \
+G_GNUC_PRINTF(1, 2)
  \
 static void G_PASTE(other_, suffix)(const gchar *format, ...)  
  \
 {  
  \
 va_list args;  
  \
-- 
2.21.0

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

Re: [Spice-devel] [PATCH spice-protocol 3/3] vdagent: introduce VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL

> 
> ..Hi
> 
> On Thu, Mar 28, 2019 at 4:14 PM Frediano Ziglio  wrote:
> > > The role of the grab message is to take ownership of the clipboard (to
> > > advertize clipboard data available). It may come at any time from both
> > > side, and override the current grab owner. It may come from the guest
> > > (after C-c in guest app, for ex), so the client grabs the clipboard.
> > > Or it may come from the client (C-c in client side app), and the agent
> > > will grab the guest clipboard.
> > >
> >
> > Yes, I realized this morning thinking about how clipboards works
> > (very rusty in my mind).
> > Instead of setting it you get the ownership that is you are willing
> > to set a value on the clipboard but you defer setting it to avoid
> > the expense of data copy.
> > So, the old (original?) protocol was simply to sending entire data
> > on every change, this avoided to loose the clipboard data entirely.
> 
> I don't even remember that version. It looks like the original version
> was already "on-demand"
> 
> 
> commit 5fdd0ba6650919dcfd7740c041ad7d2b9c4560ab
> Author: Arnon Gilboa 
> Date:   Mon Oct 4 16:45:15 2010 +0200
> 
> vd_agent: add protocol messages for clipboard/selection-owner model
> 
> -VD_AGENT_CLIPBOARD_GRAB(type) - tell the other side that an
> application in our side ("we") got ownership of the clipboard.
> -VD_AGENT_CLIPBOARD_REQUEST(type) - after we know the other side
> owns the clipboard (GRAB received), we notify the os we are the owner.
> when we are asked by the os/app for the clipboard data, we send this
> RE
> QUEST msg to the other side.
> -VD_AGENT_CLIPBOARD(type, data) - the existing message for sending
> the clipboard, is now sent only in response to REQUEST.
> -VD_AGENT_CLIPBOARD_RELEASE - tell the other side that we are no
> longer the owner of the clipboard (e.g. the owner app was closed).
> 
> this patch will be followed by agent & client patches handling the
> above messages.
> 
> 
> 
> So VD_AGENT_CAP_CLIPBOARD_BY_DEMAND simply means clipboard support to me.
> 

I suppose the "now" in "the existing message for sending the clipboard, is
now sent only in response to REQUEST" means that was changed.

I personally think the code should be readable from a tarball/snapshot not
pretending people to dig into 10 years of history. I'll try to find some time
to put these lines into vd_agent.h.

> > The current is: if we get new local clipboard data send the "grab"
> > so remote knows that will have to read our data.
> 
> yes
> 
> > So either we have ownership/grab, meaning the data are remote
> > waiting to get fetched or we don't have ownership and the remote
> > should be grabbing as we have data to send (at least in a stable
> > state).
> 
> 
> That last sentence is confusing. There are 2 states the client can
> "have the grab".
> 
> 1. the client took the grab for the remote data: we are talking about
> the client app in the client desktop environment
> 2. the client took the grab, at the protocol level: it sent a grab
> over the protocol to announce it can provide clipboard data to the
> remote. In this case, the client app doesn't have the grab in the
> client desktop environment, but another client application.
> 
> In general, when talking about the protocol, "client has the grab"
> means 2) to me, iow it can provide data to the remote.
> 

I think I mean the opposite. The reason is that some application
have the ownership on either side, when you send a VD_AGENT_CLIPBOARD_GRAB
the current local application (so spice-gtk/vd_agent) does NOT have the
ownership and it's asking the remote to take to the application (so will
remove the ownership from another remote application).
From how I read the message (VD_AGENT_CLIPBOARD_GRAB) is telling the
other side to grab (take ownership) of the clipboard so will be the
remote to have the "grab", not the local.

> 
> > Not sure what is the initial state, when you connect.
> 
> Initial state is undefined, and currently whatever the guest or client
> side state is. Iow, Spice client/agent doen't do anything afaik. They
> will only react to further grab events.
> 

I suppose the agent will keep its state while the client if was not
connected get some initial state (boolean variables have to be true or
false at the end).

> We could change the client or the guest to take the grab on connect,
> if clipboard data is available. That doesn't require protocol change.
> Although in case of conflict, we would probably end in the same
> situation that this protocol change is aiming to solve.
> 

When there's a disconnection we should drop the ownership as we
cannot anymore get the data from the remote in case other application
are asking so the "disconnected" state should be no ownership of
the clipboard on both ends (although I suppose the client will be
closed at that time).

Once a connection happens we could however have both ends with
data on the clipboard (the total states are 3 (a) client/agent have
ownership (b) other ap

Re: [Spice-devel] [PATCH linux/vd-agent 10/11] clipboard: only send release when no immediate grab

> 
> From: Marc-André Lureau 
> 
> Do not send a release event between two grabs, this helps with window
> manager interaction issues on peer side.
> 

I would explain which kind of issue this is supposed to fix.

> Advertise this behaviour via a capability introduced in spice-protocol
> 0.12.16, so the client doesn't need to do some time-based filtering.
> 
> (the capability shouldn't need to be negotiated, a client shouldn't
> expect a release between two grabs)
> 
> Signed-off-by: Marc-André Lureau 

I suppose Jakub/Victor could do a much better review/test than me.

> ---
>  src/vdagent/clipboard.c | 12 ++--
>  src/vdagent/x11.c   |  7 +++
>  src/vdagentd/vdagentd.c |  1 +
>  3 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/src/vdagent/clipboard.c b/src/vdagent/clipboard.c
> index 9fb87e3..097c6ee 100644
> --- a/src/vdagent/clipboard.c
> +++ b/src/vdagent/clipboard.c
> @@ -242,13 +242,13 @@ static void clipboard_owner_change_cb(GtkClipboard
> *clipboard,
>  return;
>  }
>  
> -if (sel->owner == OWNER_GUEST) {
> -clipboard_new_owner(c, sel_id, OWNER_NONE);
> -udscs_write(c->conn, VDAGENTD_CLIPBOARD_RELEASE, sel_id, 0, NULL,
> 0);
> -}
> -
> -if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER)
> +if (event->reason != GDK_OWNER_CHANGE_NEW_OWNER) {
> +if (sel->owner == OWNER_GUEST) {
> +clipboard_new_owner(c, sel_id, OWNER_NONE);
> +udscs_write(c->conn, VDAGENTD_CLIPBOARD_RELEASE, sel_id, 0,
> NULL, 0);
> +}
>  return;
> +}
>  
>  /* if there's a pending request for clipboard targets, cancel it */
>  if (sel->last_targets_req)
> diff --git a/src/vdagent/x11.c b/src/vdagent/x11.c
> index c2515a8..9409b53 100644
> --- a/src/vdagent/x11.c
> +++ b/src/vdagent/x11.c
> @@ -530,11 +530,10 @@ static void vdagent_x11_handle_event(struct vdagent_x11
> *x11, XEvent event)
>  if (ev.xfev.owner == x11->selection_window)
>  return;
>  
> -/* If the clipboard owner is changed we no longer own it */

Why not keeping this comment?

> -vdagent_x11_set_clipboard_owner(x11, selection, owner_none);
> -
> -if (ev.xfev.owner == None)
> +if (ev.xfev.owner == None) {
> +vdagent_x11_set_clipboard_owner(x11, selection, owner_none);
>  return;
> +}
>  
>  /* Request the supported targets from the new owner */
>  XConvertSelection(x11->display, ev.xfev.selection,
>  x11->targets_atom,
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 72a3e13..683e5d3 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -133,6 +133,7 @@ static void send_capabilities(struct vdagent_virtio_port
> *vport,
>  VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MAX_CLIPBOARD);
>  VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_AUDIO_VOLUME_SYNC);
>  VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_GRAPHICS_DEVICE_INFO);
> +VD_AGENT_SET_CAPABILITY(caps->caps,
> VD_AGENT_CAP_CLIPBOARD_NO_RELEASE_ON_REGRAB);
>  virtio_msg_uint32_to_le((uint8_t *)caps, size, 0);
>  
>  vdagent_virtio_port_write(vport, VDP_CLIENT_PORT,

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

Re: [Spice-devel] [PATCH linux/vd-agent 06/11] configure: bump gobject >= 2.50

Hi

On Sun, Mar 24, 2019 at 6:49 PM Jakub Janku  wrote:
>
> Hi,
>
> On Fri, Mar 22, 2019 at 4:13 PM  wrote:
> >
> > From: Marc-André Lureau 
> >
> > This is required for using the new GObject macros.
>
> Which macros are you referring to?
> G_DECLARE_FINAL_TYPE is available since 2.44

That's right, we could probably take 2.44. But since our min os
requirement is 2.50 already, it's easier to directly bump there.

> >
> > According to commit 61fc548fe1a323dd2344c8ae267e3ce05e86da7d ("Bump
> > GLib version to 2.34"), RHEL6 is no longer supported.
> >
> > GLib version across some distributions, from repology:
> > - Debian Stable (9): 2.50.3
> > - CentOS 7: 2.56.1
> > - Fedora 26: 2.52.3 (fwiw, Fedora 30: 2.60.0)
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  configure.ac | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index fb0675f..9808f58 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -100,7 +100,7 @@ AC_ARG_ENABLE([static-uinput],
> >[enable_static_uinput="$enableval"],
> >[enable_static_uinput="no"])
> >
> > -PKG_CHECK_MODULES([GOBJECT], [gobject-2.0 >= 2.34])
> > +PKG_CHECK_MODULES([GOBJECT], [gobject-2.0 >= 2.50])
>
> You could bump glib version as well

5/11 replaced glib by gobject, so it's implicit now.

>
> >  PKG_CHECK_MODULES(X, [xfixes xrandr >= 1.3 xinerama x11])
> >  PKG_CHECK_MODULES(SPICE, [spice-protocol >= 0.12.16])
> >  PKG_CHECK_MODULES(ALSA, [alsa >= 1.0.22])
> > --
> > 2.21.0.4.g36eb1cb9cf
> >
>
> Cheers,
> Jakub
> ___
> 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 linux/vd-agent 06/11] configure: bump gobject >= 2.50

Hi
On Thu, Mar 28, 2019 at 4:27 PM Frediano Ziglio  wrote:
>
> >
> > From: Marc-André Lureau 
> >
> > This is required for using the new GObject macros.
> >
> > According to commit 61fc548fe1a323dd2344c8ae267e3ce05e86da7d ("Bump
> > GLib version to 2.34"), RHEL6 is no longer supported.
> >
> > GLib version across some distributions, from repology:
> > - Debian Stable (9): 2.50.3
> > - CentOS 7: 2.56.1
> > - Fedora 26: 2.52.3 (fwiw, Fedora 30: 2.60.0)
> >
> > Signed-off-by: Marc-André Lureau 
> > ---
> >  configure.ac | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index fb0675f..9808f58 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -100,7 +100,7 @@ AC_ARG_ENABLE([static-uinput],
> >[enable_static_uinput="$enableval"],
> >[enable_static_uinput="no"])
> >
> > -PKG_CHECK_MODULES([GOBJECT], [gobject-2.0 >= 2.34])
> > +PKG_CHECK_MODULES([GOBJECT], [gobject-2.0 >= 2.50])
> >  PKG_CHECK_MODULES(X, [xfixes xrandr >= 1.3 xinerama x11])
> >  PKG_CHECK_MODULES(SPICE, [spice-protocol >= 0.12.16])
> >  PKG_CHECK_MODULES(ALSA, [alsa >= 1.0.22])
>
> I don't understand why not directly do it in 5/11, or why
> not also bumping glib-2.0 to 2.50, are they not directly
> related?

5/11 switches from glib dep to gobject dep.

6/11 bump version requirement.

The 2 should probably be done & discussed separately.


>
> Frediano
> ___
> 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 spice-server v2 02/12] Move thread/dispatching handling to RedChannel

Acked-by: Jonathon Jongsma 


On Tue, 2019-03-26 at 19:10 +, Frediano Ziglio wrote:
> Currently channel threading/handling is spread between RedQxl,
> RedWorker and RedChannel.
> Move more to RedChannel simplify RedQxl and RedWorker.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/cursor-channel.c|   4 +-
>  server/cursor-channel.h|   4 +-
>  server/display-channel.c   |   2 +
>  server/display-channel.h   |   1 +
>  server/red-channel.c   | 112 +++--
>  server/red-qxl.c   | 110 +---
>  server/red-replay-qxl.c|   3 -
>  server/red-stream-device.c |   2 +-
>  server/red-worker.c| 126 +
> 
>  server/red-worker.h|  46 +++---
>  10 files changed, 157 insertions(+), 253 deletions(-)
> 
> diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> index 4220084f..e8af01b0 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -228,7 +228,8 @@ static void
> cursor_channel_send_item(RedChannelClient *rcc, RedPipeItem *pipe_it
>  }
>  
>  CursorChannel* cursor_channel_new(RedsState *server, int id,
> -  const SpiceCoreInterfaceInternal
> *core)
> +  const SpiceCoreInterfaceInternal
> *core,
> +  Dispatcher *dispatcher)
>  {
>  spice_debug("create cursor channel");
>  return g_object_new(TYPE_CURSOR_CHANNEL,
> @@ -238,6 +239,7 @@ CursorChannel* cursor_channel_new(RedsState
> *server, int id,
>  "id", id,
>  "migration-flags", 0,
>  "handle-acks", TRUE,
> +"dispatcher", dispatcher,
>  NULL);
>  }
>  
> diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> index 603c2c0a..767325ea 100644
> --- a/server/cursor-channel.h
> +++ b/server/cursor-channel.h
> @@ -21,6 +21,7 @@
>  
>  #include "common-graphics-channel.h"
>  #include "red-parse-qxl.h"
> +#include "dispatcher.h"
>  
>  G_BEGIN_DECLS
>  
> @@ -57,7 +58,8 @@ GType cursor_channel_get_type(void) G_GNUC_CONST;
>   * CursorChannel thread.
>   */
>  CursorChannel* cursor_channel_new(RedsState *server, int id,
> -  const SpiceCoreInterfaceInternal
> *core);
> +  const SpiceCoreInterfaceInternal
> *core,
> +  Dispatcher *dispatcher);
>  
>  void cursor_channel_reset   (CursorChannel
> *cursor);
>  void cursor_channel_do_init (CursorChannel
> *cursor);
> diff --git a/server/display-channel.c b/server/display-channel.c
> index e179abfd..cb052bfc 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -2227,6 +2227,7 @@ static SpiceCanvas
> *image_surfaces_get(SpiceImageSurfaces *surfaces, uint32_t su
>  DisplayChannel* display_channel_new(RedsState *reds,
>  QXLInstance *qxl,
>  const SpiceCoreInterfaceInternal
> *core,
> +Dispatcher *dispatcher,
>  int migrate, int stream_video,
>  GArray *video_codecs,
>  uint32_t n_surfaces)
> @@ -2246,6 +2247,7 @@ DisplayChannel* display_channel_new(RedsState
> *reds,
> "n-surfaces", n_surfaces,
> "video-codecs", video_codecs,
> "handle-acks", TRUE,
> +   "dispatcher", dispatcher,
> NULL);
>  if (display) {
>  display_channel_set_stream_video(display, stream_video);
> diff --git a/server/display-channel.h b/server/display-channel.h
> index 948018cf..f64da0bd 100644
> --- a/server/display-channel.h
> +++ b/server/display-channel.h
> @@ -99,6 +99,7 @@ struct Drawable {
>  DisplayChannel*display_channel_new  
>  (RedsState *reds,
>  
>   QXLInstance *qxl,
>  
>   const SpiceCoreInterfaceInternal *core,
> +
>   Dispatcher *dispatcher,
>  
>   int migrate,
>  
>   int stream_video,
>  
>   GArray *video_codecs,
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 1d88739e..8d4992fa 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -90,6 +90,12 @@ struct RedChannelPrivate
>  // TODO: when different channel_clients are in different threads
>  // from Chan

Re: [Spice-devel] [PATCH spice-server v2 01/12] dispatcher: Allows to manage messages without registering them

> On Tue, 2019-03-26 at 19:10 +, Frediano Ziglio wrote:
> > The only way to add new message to Dispatcher was to register
> > using a number. These numbers corresponded to array indexes.
> > This is good if the list of messages is allocated statically
> > and contiguously, on the contrary this method is not that
> > flexible.
> > Writing a header of 4 or 16 bytes using system call does not
> > make much difference so pass all message information in the
> > payload header.
> > A new dispatcher_send_message_custom function allows to send
> > a message passing all message information, including the
> > pointer to the handler.
> > This will allow for instance a Dispatcher associate to a given
> > thread to be reused by different classes.
> > 
> > Signed-off-by: Frediano Ziglio 
> > ---
> >  server/dispatcher.c | 77 +++--
> > 
> >  server/dispatcher.h | 15 +
> >  2 files changed, 68 insertions(+), 24 deletions(-)
> > 
> > diff --git a/server/dispatcher.c b/server/dispatcher.c
> > index 5f839ec4..ed4037aa 100644
> > --- a/server/dispatcher.c
> > +++ b/server/dispatcher.c
> > @@ -38,10 +38,19 @@
> >  static void setup_dummy_signal_handler(void);
> >  #endif
> >  
> > +#define DISPATCHER_CUSTOM_TYPE 0x7fffu
> 
> Can I suggest DISPATCHER_MESSAGE_TYPE_CUSTOM ?
> 

Fine for me, I'll change

> 
> > +
> > +/* structure to store message header information.
> > + * That structure is sent through a socketpair so it's optimized
> > + * to be transfered via sockets.
> > + * Is also packaged to not leave holes in both 32 and 64
> > environments
> > + * so memory instrumentation tools should not find uninitialised
> > bytes.
> > + */
> >  typedef struct DispatcherMessage {
> > -size_t size;
> > -bool ack;
> >  dispatcher_handle_message handler;
> > +uint32_t size;
> > +uint32_t type:31;
> > +uint32_t ack:1;
> >  } DispatcherMessage;
> >  
> >  struct DispatcherPrivate {
> > @@ -249,12 +258,11 @@ static int write_safe(int fd, uint8_t *buf,
> > size_t size)
> >  static int dispatcher_handle_single_read(Dispatcher *dispatcher)
> >  {
> >  int ret;
> > -uint32_t type;
> > -DispatcherMessage *msg = NULL;
> > -uint8_t *payload = dispatcher->priv->payload;
> > +DispatcherMessage msg[1];
> > +uint8_t *payload;
> >  uint32_t ack = ACK;
> >  
> > -if ((ret = read_safe(dispatcher->priv->recv_fd, (uint8_t*)&type,
> > sizeof(type), 0)) == -1) {
> > +if ((ret = read_safe(dispatcher->priv->recv_fd, (uint8_t*)msg,
> > sizeof(msg), 0)) == -1) {
> >  g_warning("error reading from dispatcher: %d", errno);
> >  return 0;
> >  }
> > @@ -262,28 +270,28 @@ static int
> > dispatcher_handle_single_read(Dispatcher *dispatcher)
> >  /* no message */
> >  return 0;
> >  }
> > -if (type >= dispatcher->priv->max_message_type) {
> > -spice_error("Invalid message type for this dispatcher: %u",
> > type);
> > -return 0;
> > +if (G_UNLIKELY(msg->size > dispatcher->priv->payload_size)) {
> > +dispatcher->priv->payload = g_realloc(dispatcher->priv-
> > >payload, msg->size);
> > +dispatcher->priv->payload_size = msg->size;
> >  }
> > -msg = &dispatcher->priv->messages[type];
> > +payload = dispatcher->priv->payload;
> >  if (read_safe(dispatcher->priv->recv_fd, payload, msg->size, 1)
> > == -1) {
> >  g_warning("error reading from dispatcher: %d", errno);
> >  /* TODO: close socketpair? */
> >  return 0;
> >  }
> > -if (dispatcher->priv->any_handler) {
> > -dispatcher->priv->any_handler(dispatcher->priv->opaque,
> > type, payload);
> > +if (dispatcher->priv->any_handler && msg->type !=
> > DISPATCHER_CUSTOM_TYPE) {
> > +dispatcher->priv->any_handler(dispatcher->priv->opaque, msg-
> > >type, payload);
> >  }
> >  if (msg->handler) {
> >  msg->handler(dispatcher->priv->opaque, payload);
> >  } else {
> > -g_warning("error: no handler for message type %d", type);
> > +g_warning("error: no handler for message type %d", msg-
> > >type);
> >  }
> >  if (msg->ack) {
> >  if (write_safe(dispatcher->priv->recv_fd,
> > (uint8_t*)&ack, sizeof(ack)) == -1) {
> > -g_warning("error writing ack for message %d", type);
> > +g_warning("error writing ack for message %d", msg-
> > >type);
> >  /* TODO: close socketpair? */
> >  }
> >  }
> > @@ -300,25 +308,22 @@ void dispatcher_handle_recv_read(Dispatcher
> > *dispatcher)
> >  }
> >  }
> >  
> > -void dispatcher_send_message(Dispatcher *dispatcher, uint32_t
> > message_type,
> > - void *payload)
> > +static void
> > +dispatcher_send_message_internal(Dispatcher *dispatcher, const
> > DispatcherMessage*msg,
> > + void *payload)
> >  {
> > -DispatcherMessage *msg;
> >  uint32_t ack;

Re: [Spice-devel] [PATCH spice-protocol 3/3] vdagent: introduce VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL

..Hi

On Thu, Mar 28, 2019 at 4:14 PM Frediano Ziglio  wrote:
> > The role of the grab message is to take ownership of the clipboard (to
> > advertize clipboard data available). It may come at any time from both
> > side, and override the current grab owner. It may come from the guest
> > (after C-c in guest app, for ex), so the client grabs the clipboard.
> > Or it may come from the client (C-c in client side app), and the agent
> > will grab the guest clipboard.
> >
>
> Yes, I realized this morning thinking about how clipboards works
> (very rusty in my mind).
> Instead of setting it you get the ownership that is you are willing
> to set a value on the clipboard but you defer setting it to avoid
> the expense of data copy.
> So, the old (original?) protocol was simply to sending entire data
> on every change, this avoided to loose the clipboard data entirely.

I don't even remember that version. It looks like the original version
was already "on-demand"

commit 5fdd0ba6650919dcfd7740c041ad7d2b9c4560ab
Author: Arnon Gilboa 
Date:   Mon Oct 4 16:45:15 2010 +0200

vd_agent: add protocol messages for clipboard/selection-owner model

-VD_AGENT_CLIPBOARD_GRAB(type) - tell the other side that an
application in our side ("we") got ownership of the clipboard.
-VD_AGENT_CLIPBOARD_REQUEST(type) - after we know the other side
owns the clipboard (GRAB received), we notify the os we are the owner.
when we are asked by the os/app for the clipboard data, we send this
RE
QUEST msg to the other side.
-VD_AGENT_CLIPBOARD(type, data) - the existing message for sending
the clipboard, is now sent only in response to REQUEST.
-VD_AGENT_CLIPBOARD_RELEASE - tell the other side that we are no
longer the owner of the clipboard (e.g. the owner app was closed).

this patch will be followed by agent & client patches handling the
above messages.

So VD_AGENT_CAP_CLIPBOARD_BY_DEMAND simply means clipboard support to me.

> The current is: if we get new local clipboard data send the "grab"
> so remote knows that will have to read our data.

yes

> So either we have ownership/grab, meaning the data are remote
> waiting to get fetched or we don't have ownership and the remote
> should be grabbing as we have data to send (at least in a stable
> state).

That last sentence is confusing. There are 2 states the client can
"have the grab".

1. the client took the grab for the remote data: we are talking about
the client app in the client desktop environment
2. the client took the grab, at the protocol level: it sent a grab
over the protocol to announce it can provide clipboard data to the
remote. In this case, the client app doesn't have the grab in the
client desktop environment, but another client application.

In general, when talking about the protocol, "client has the grab"
means 2) to me, iow it can provide data to the remote.

> Not sure what is the initial state, when you connect.

Initial state is undefined, and currently whatever the guest or client
side state is. Iow, Spice client/agent doen't do anything afaik. They
will only react to further grab events.

We could change the client or the guest to take the grab on connect,
if clipboard data is available. That doesn't require protocol change.
Although in case of conflict, we would probably end in the same
situation that this protocol change is aiming to solve.

> But from the stable state (one and only one has the ownership)
> is not clear how you get both sending the grab message at the same
> time (the one without ownership should not send the grab).

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

Re: [Spice-devel] [PATCH linux/vd-agent 06/11] configure: bump gobject >= 2.50

> 
> From: Marc-André Lureau 
> 
> This is required for using the new GObject macros.
> 
> According to commit 61fc548fe1a323dd2344c8ae267e3ce05e86da7d ("Bump
> GLib version to 2.34"), RHEL6 is no longer supported.
> 
> GLib version across some distributions, from repology:
> - Debian Stable (9): 2.50.3
> - CentOS 7: 2.56.1
> - Fedora 26: 2.52.3 (fwiw, Fedora 30: 2.60.0)
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  configure.ac | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configure.ac b/configure.ac
> index fb0675f..9808f58 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -100,7 +100,7 @@ AC_ARG_ENABLE([static-uinput],
>[enable_static_uinput="$enableval"],
>[enable_static_uinput="no"])
>  
> -PKG_CHECK_MODULES([GOBJECT], [gobject-2.0 >= 2.34])
> +PKG_CHECK_MODULES([GOBJECT], [gobject-2.0 >= 2.50])
>  PKG_CHECK_MODULES(X, [xfixes xrandr >= 1.3 xinerama x11])
>  PKG_CHECK_MODULES(SPICE, [spice-protocol >= 0.12.16])
>  PKG_CHECK_MODULES(ALSA, [alsa >= 1.0.22])

I don't understand why not directly do it in 5/11, or why
not also bumping glib-2.0 to 2.50, are they not directly
related?

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

Re: [Spice-devel] [PATCH spice-server v2 01/12] dispatcher: Allows to manage messages without registering them

On Tue, 2019-03-26 at 19:10 +, Frediano Ziglio wrote:
> The only way to add new message to Dispatcher was to register
> using a number. These numbers corresponded to array indexes.
> This is good if the list of messages is allocated statically
> and contiguously, on the contrary this method is not that
> flexible.
> Writing a header of 4 or 16 bytes using system call does not
> make much difference so pass all message information in the
> payload header.
> A new dispatcher_send_message_custom function allows to send
> a message passing all message information, including the
> pointer to the handler.
> This will allow for instance a Dispatcher associate to a given
> thread to be reused by different classes.
> 
> Signed-off-by: Frediano Ziglio 
> ---
>  server/dispatcher.c | 77 +++--
> 
>  server/dispatcher.h | 15 +
>  2 files changed, 68 insertions(+), 24 deletions(-)
> 
> diff --git a/server/dispatcher.c b/server/dispatcher.c
> index 5f839ec4..ed4037aa 100644
> --- a/server/dispatcher.c
> +++ b/server/dispatcher.c
> @@ -38,10 +38,19 @@
>  static void setup_dummy_signal_handler(void);
>  #endif
>  
> +#define DISPATCHER_CUSTOM_TYPE 0x7fffu

Can I suggest DISPATCHER_MESSAGE_TYPE_CUSTOM ?


> +
> +/* structure to store message header information.
> + * That structure is sent through a socketpair so it's optimized
> + * to be transfered via sockets.
> + * Is also packaged to not leave holes in both 32 and 64
> environments
> + * so memory instrumentation tools should not find uninitialised
> bytes.
> + */
>  typedef struct DispatcherMessage {
> -size_t size;
> -bool ack;
>  dispatcher_handle_message handler;
> +uint32_t size;
> +uint32_t type:31;
> +uint32_t ack:1;
>  } DispatcherMessage;
>  
>  struct DispatcherPrivate {
> @@ -249,12 +258,11 @@ static int write_safe(int fd, uint8_t *buf,
> size_t size)
>  static int dispatcher_handle_single_read(Dispatcher *dispatcher)
>  {
>  int ret;
> -uint32_t type;
> -DispatcherMessage *msg = NULL;
> -uint8_t *payload = dispatcher->priv->payload;
> +DispatcherMessage msg[1];
> +uint8_t *payload;
>  uint32_t ack = ACK;
>  
> -if ((ret = read_safe(dispatcher->priv->recv_fd, (uint8_t*)&type,
> sizeof(type), 0)) == -1) {
> +if ((ret = read_safe(dispatcher->priv->recv_fd, (uint8_t*)msg,
> sizeof(msg), 0)) == -1) {
>  g_warning("error reading from dispatcher: %d", errno);
>  return 0;
>  }
> @@ -262,28 +270,28 @@ static int
> dispatcher_handle_single_read(Dispatcher *dispatcher)
>  /* no message */
>  return 0;
>  }
> -if (type >= dispatcher->priv->max_message_type) {
> -spice_error("Invalid message type for this dispatcher: %u",
> type);
> -return 0;
> +if (G_UNLIKELY(msg->size > dispatcher->priv->payload_size)) {
> +dispatcher->priv->payload = g_realloc(dispatcher->priv-
> >payload, msg->size);
> +dispatcher->priv->payload_size = msg->size;
>  }
> -msg = &dispatcher->priv->messages[type];
> +payload = dispatcher->priv->payload;
>  if (read_safe(dispatcher->priv->recv_fd, payload, msg->size, 1)
> == -1) {
>  g_warning("error reading from dispatcher: %d", errno);
>  /* TODO: close socketpair? */
>  return 0;
>  }
> -if (dispatcher->priv->any_handler) {
> -dispatcher->priv->any_handler(dispatcher->priv->opaque,
> type, payload);
> +if (dispatcher->priv->any_handler && msg->type !=
> DISPATCHER_CUSTOM_TYPE) {
> +dispatcher->priv->any_handler(dispatcher->priv->opaque, msg-
> >type, payload);
>  }
>  if (msg->handler) {
>  msg->handler(dispatcher->priv->opaque, payload);
>  } else {
> -g_warning("error: no handler for message type %d", type);
> +g_warning("error: no handler for message type %d", msg-
> >type);
>  }
>  if (msg->ack) {
>  if (write_safe(dispatcher->priv->recv_fd,
> (uint8_t*)&ack, sizeof(ack)) == -1) {
> -g_warning("error writing ack for message %d", type);
> +g_warning("error writing ack for message %d", msg-
> >type);
>  /* TODO: close socketpair? */
>  }
>  }
> @@ -300,25 +308,22 @@ void dispatcher_handle_recv_read(Dispatcher
> *dispatcher)
>  }
>  }
>  
> -void dispatcher_send_message(Dispatcher *dispatcher, uint32_t
> message_type,
> - void *payload)
> +static void
> +dispatcher_send_message_internal(Dispatcher *dispatcher, const
> DispatcherMessage*msg,
> + void *payload)
>  {
> -DispatcherMessage *msg;
>  uint32_t ack;
>  int send_fd = dispatcher->priv->send_fd;
>  
> -assert(dispatcher->priv->max_message_type > message_type);
> -assert(dispatcher->priv->messages[message_type].handler);

You've removed a bunch of asserts here, but do we want to assert (or at
least g_return_if_fail) if msg is NULL

Re: [Spice-devel] [PATCH spice-protocol 3/3] vdagent: introduce VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL

> Hi
> 
> On Wed, Mar 27, 2019 at 4:50 PM Frediano Ziglio  wrote:
> >
> > > Hi
> > >
> > > On Wed, Mar 27, 2019 at 8:23 AM Frediano Ziglio 
> > > wrote:
> > > >
> > > > >
> > > > > From: Marc-André Lureau 
> > > > >
> > > > > When this capability is negoticated by both the client & the agent,
> > > >
> > > > negotiated
> > > >
> > > > > the clipboard grab messages have an associated serial counter.
> > > > >
> > > > > The serial is reset to 0 upon client connection.
> > > > >
> > > > > The counter is increment by 1 on each grab message, by both sides.
> > > > >
> > > >
> > > > What would happen in case of restart of the guest? How the guest is
> > > > supposed to keep the old serial?
> > >
> > > This is like a new client-agent connection in this case, it starts again
> > > from
> > > 0.
> > >
> > > > I think you can achieve sending the serial with an additional separate
> > > > message at the beginning.
> > >
> > > I don't think it's necessary, but I am may be missing something.
> > >
> >
> > Well, adding a field or a message are both changes, just different.
> >
> > > > I don't think this new protocol is designed to support multiple
> > > > clients.
> > >
> > > clipboard sharing isn't designed for multiple client either.
> > >
> > > >
> > > > > The sender of the message with the highest serial should be the
> > > > > clipboard grab owner, and the current session serial should be
> > > > > updated.
> > > > >
> > > > > If a lower serial than the current session serial is received, the
> > > > > grab should be discarded.
> > > > >
> > > > > Whenever two grabs share the same serial, the one coming from the
> > > > > client should have a higher priority and the client should gain the
> > > > > clipboard ownership.
> > > > >
> > > > > No special treatement is done for the unlikely case of overflowing
> > > > > the
> > > >
> > > > treatment
> > >
> > > ok
> > >
> > > >
> > > > > counter. It may temporarily inverse the priority, until both side
> > > > > have
> > > > > overflown and/or synchronized.
> > > > >
> > > >
> > > > synchronized? So there's a single counter for both guest and client?
> > > > I though were two counters, one for each side.
> > >
> > > Conceptually, it's the "same" counter.
> > >
> >
> > Okay, it was not clear.
> >
> > > >
> > > > > Note: this mechanism isn't aiming at making "the most recent" (as in
> > > > > time) side gaining the ownership. One side sending subsequent grab
> > > > > messages earlier will likely take the ownership over a side sending a
> > > > > single message simultaneously the other way. It only clears the
> > > > > situation where both side believe that the other is the current
> > > > > clipboard owner, by having a global ordering and priority in case of
> > > > > serial conflict.
> > > > >
> > > > > Signed-off-by: Marc-André Lureau 
> > > > > ---
> > > > >  spice/vd_agent.h | 4 
> > > > >  1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> > > > > index 862cb5c..9ae3cc7 100644
> > > > > --- a/spice/vd_agent.h
> > > > > +++ b/spice/vd_agent.h
> > > > > @@ -218,6 +218,9 @@ typedef struct SPICE_ATTR_PACKED
> > > > > VDAgentClipboardGrab
> > > > > {
> > > > >  #if 0 /* VD_AGENT_CAP_CLIPBOARD_SELECTION */
> > > > >  uint8_t selection;
> > > > >  uint8_t __reserved[sizeof(uint32_t) - 1 * sizeof(uint8_t)];
> > > > > +#endif
> > > > > +#if 0 /* VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL */
> > > > > +uint32_t serial;
> > > > >  #endif
> > > >
> > > > I would prefer a new message instead of partial structures.
> > >
> > > VDAgentClipboardGrabSelectionAndSerialAnd... ?
> > >
> >
> > Usually a version schema is used like VDAgentClipboardGrabSelection_v2
> >
> > > yeah, it's not ideal. I wish we would use a better protocol, be it
> > > json or protobuf etc..
> > >
> >
> > This is not a justification to produce even worse code.
> > A plain C structure would be fine, many binary protocols (like IP)
> > uses this method.
> > A semi-defined C structure with missing fields is the worst I
> > ever seen, continuing to use is not so sensible.
> 
> Different trade-offs, version vs capabilities.
> 
> Since we don't have missing fields in the protocol, this means
> VDAgentClipboardGrabSelection_v2 should have selection capability
> included.
> 

I don't think this is an issue

> This may not be a problem for vdagent, but for many cases,
> capabilities offer more flexibility than versioning.
> 

Sure. What I don't like, if is not really clear is having a
semi-defined C structure. I would prefer to not having the structure
instead. There's no C rule to define "this field is present if you
have this capability", VDAgentClipboardGrabSelection does not
represent a message (unless there's no selection) but currently
the end of a message so it's misleading.

> > It would be better to not define the C structure at all.
> > OT: I think it was discussed to use an external library/tool, somebody
> > suggested to implement new messages with d

Re: [Spice-devel] [PATCH spice-server 2/2] docs: Add some notes on event scheduling and threading

> On Thu, Mar 28, 2019 at 04:25:31AM -0400, Frediano Ziglio wrote:
> > > 
> > > On Mon, Mar 11, 2019 at 02:03:33PM +, Frediano Ziglio wrote:
> > > > Signed-off-by: Frediano Ziglio 
> > > > ---
> > > >  docs/spice_threading_model.txt | 8 
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/docs/spice_threading_model.txt
> > > > b/docs/spice_threading_model.txt
> > > > index 9351141c8..25a3a030c 100644
> > > > --- a/docs/spice_threading_model.txt
> > > > +++ b/docs/spice_threading_model.txt
> > > > @@ -39,6 +39,14 @@ connect, disconnect and migrate. Connect and migrate
> > > > are
> > > > asynchronous (the job
> > > >  is done while the current thread is doing something else) while
> > > >  disconnect
> > > >  is
> > > >  synchronous (the main thread will wait for termination).
> > > >  
> > > > +One aspect to take into consideration is the event scheduling. SPICE
> > > > uses
> > > > some
> > > > +`SpiceCoreInterface` to handle events. As the events will be handled
> > > > from
> > > > a
> > > > +thread based on the core interface you have to use the correct core.
> > > > Each
> > > > +channel has an associated core interface which can be retrieved using
> > > > +`red_channel_get_core_interface`. There's also a main core interface
> > > > you
> > > > can get
> > > > +using `reds_get_core_interface`. `reds_core_timer_*` and
> > > > `reds_core_watch_*`
> > > > +functions use the main core interface.
> > > 
> > > Do we need a few words as to when to use the main core interface?
> > > Apart from this, looks good to me.
> > > 
> > > Christophe
> > > 
> > 
> > It sounds a nice idea.
> > 
> > But honestly I cannot came with an easy rule beside "If code runs on
> > main thread like Qemu character devices or everything not running in
> > a channel you can use the main core interface."
> 
> Yes, something like your rule would work "Code running in the QEMU
> thread should use the main core interface. Code running in the cursor or
> display channel (through RedWorker) should use xxx interface.. Code
> running in other channels should use yyy. Be aware that a channel's

IMHO all code running in channels should use channel core, no matter
if they run on main or not (in the past I adjusted this).
Note that cursor channel code can run in both main and not main
for instance, not all cursor channels runs under RedWorker.
I think it's easier to avoid too much exceptions.

> ClientCbs run in a different thread context than the rest of the
> channel" (though the last sentence may no longer be accurate with the
> work you are doing in that area).

ClientCbs will be removed, one less thing to know, and new callbacks/vfunc
will work on the channel thread so not different from other channel code.

> 
> Christophe
> 

Taking into account that ClientCbs part will be soon (I hope) obsolete
and that part of "channel core for channel core" part is partially there
I would update to

+One aspect to take into consideration is the event scheduling. SPICE uses some
+`SpiceCoreInterface` to handle events. As the events will be handled from a
+thread based on the core interface you have to use the correct core. Each
+channel has an associated core interface which can be retrieved using
+`red_channel_get_core_interface`. There's also a main core interface you can 
get
+using `reds_get_core_interface`. `reds_core_timer_*` and `reds_core_watch_*`
+functions use the main core interface.
+Even though multiple channel types run in the main thread and so could use
+directly the main code interface, for coherence, rule simplicity and to allows
+the code to be moved in a separate thread easily if needed, it's advisable to
+use the channel core interface (that will be the main core interface in this
+case).
+Currently character devices and not channel code runs in the main thread.
+

OT: I though QEMU moved to GLib to handle events (so the main core interface
was using GLib) but for performance reasons epoll or other implementations
are used (that's the reason for SOCKET limitation for Windows).

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

[Spice-devel] Two monitors Windows

2019-03-28 Thread Mathias Egekvist

Hi Spice Developer(s),

Not sure this email should go to you, but I am not sure where else to go.

Recently started using Spice with libvirt and found great performance.
I have started a Windows 10 guest vm and it is working fine with 1 monitor.
I saw on your website that
to add 2 monitors I had to add an extra QXL device and it worked.
Suddenly the mouse stopped working correctly though and I have been unable to
find a solution. When I remove the extra video tag everything works fine again.

Is it you I need to ask or someone else?
If it is a general problem and I can help, please let me know.
I'll add all my information below.

Best regards,

Mathias Egekvist

Problem in details:
The mouse either "glues" it self to the furthest left-side or only move in Y
and X-axis as in only one at a time. Sometimes the mouse move normally though,
but the problem which persists is every time I click it goes to the top left
corner and registers the click there.

Setup Details:
System is Arch Linux and window manager is dwm.
VM is Windows 10 running on KVM through libvirt, seen through virt-viewer (have
also tried remote-viewer).
I have the EvTouch USB Graphics Tablet added together with a mouse and keyboard.
I have two QXL displays added, where I have tried adjusting the different ram
options, currently ram & vram 65536 and vgamem 16384.
Screen/graphics is Spice server, listen type have tried both none and address.
Tried with address 'All Interfaces' with port and tls and opengl on and off.
XML you can see here:
https://unix.stackexchange.com/questions/507725/cursor-jumps-to-left-corner-windows-10-vm-kvm

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

Re: [Spice-devel] [PATCH spice-server 2/2] docs: Add some notes on event scheduling and threading

On Thu, Mar 28, 2019 at 04:25:31AM -0400, Frediano Ziglio wrote:
> > 
> > On Mon, Mar 11, 2019 at 02:03:33PM +, Frediano Ziglio wrote:
> > > Signed-off-by: Frediano Ziglio 
> > > ---
> > >  docs/spice_threading_model.txt | 8 
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/docs/spice_threading_model.txt
> > > b/docs/spice_threading_model.txt
> > > index 9351141c8..25a3a030c 100644
> > > --- a/docs/spice_threading_model.txt
> > > +++ b/docs/spice_threading_model.txt
> > > @@ -39,6 +39,14 @@ connect, disconnect and migrate. Connect and migrate 
> > > are
> > > asynchronous (the job
> > >  is done while the current thread is doing something else) while 
> > > disconnect
> > >  is
> > >  synchronous (the main thread will wait for termination).
> > >  
> > > +One aspect to take into consideration is the event scheduling. SPICE uses
> > > some
> > > +`SpiceCoreInterface` to handle events. As the events will be handled from
> > > a
> > > +thread based on the core interface you have to use the correct core. Each
> > > +channel has an associated core interface which can be retrieved using
> > > +`red_channel_get_core_interface`. There's also a main core interface you
> > > can get
> > > +using `reds_get_core_interface`. `reds_core_timer_*` and
> > > `reds_core_watch_*`
> > > +functions use the main core interface.
> > 
> > Do we need a few words as to when to use the main core interface?
> > Apart from this, looks good to me.
> > 
> > Christophe
> > 
> 
> It sounds a nice idea.
> 
> But honestly I cannot came with an easy rule beside "If code runs on
> main thread like Qemu character devices or everything not running in
> a channel you can use the main core interface."

Yes, something like your rule would work "Code running in the QEMU
thread should use the main core interface. Code running in the cursor or
display channel (through RedWorker) should use xxx interface.. Code
running in other channels should use yyy. Be aware that a channel's
ClientCbs run in a different thread context than the rest of the
channel" (though the last sentence may no longer be accurate with the
work you are doing in that area).

Christophe


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

Re: [Spice-devel] [PATCH spice-protocol 3/3] vdagent: introduce VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL

Hi

On Wed, Mar 27, 2019 at 4:50 PM Frediano Ziglio  wrote:
>
> > Hi
> >
> > On Wed, Mar 27, 2019 at 8:23 AM Frediano Ziglio  wrote:
> > >
> > > >
> > > > From: Marc-André Lureau 
> > > >
> > > > When this capability is negoticated by both the client & the agent,
> > >
> > > negotiated
> > >
> > > > the clipboard grab messages have an associated serial counter.
> > > >
> > > > The serial is reset to 0 upon client connection.
> > > >
> > > > The counter is increment by 1 on each grab message, by both sides.
> > > >
> > >
> > > What would happen in case of restart of the guest? How the guest is
> > > supposed to keep the old serial?
> >
> > This is like a new client-agent connection in this case, it starts again 
> > from
> > 0.
> >
> > > I think you can achieve sending the serial with an additional separate
> > > message at the beginning.
> >
> > I don't think it's necessary, but I am may be missing something.
> >
>
> Well, adding a field or a message are both changes, just different.
>
> > > I don't think this new protocol is designed to support multiple
> > > clients.
> >
> > clipboard sharing isn't designed for multiple client either.
> >
> > >
> > > > The sender of the message with the highest serial should be the
> > > > clipboard grab owner, and the current session serial should be
> > > > updated.
> > > >
> > > > If a lower serial than the current session serial is received, the
> > > > grab should be discarded.
> > > >
> > > > Whenever two grabs share the same serial, the one coming from the
> > > > client should have a higher priority and the client should gain the
> > > > clipboard ownership.
> > > >
> > > > No special treatement is done for the unlikely case of overflowing the
> > >
> > > treatment
> >
> > ok
> >
> > >
> > > > counter. It may temporarily inverse the priority, until both side have
> > > > overflown and/or synchronized.
> > > >
> > >
> > > synchronized? So there's a single counter for both guest and client?
> > > I though were two counters, one for each side.
> >
> > Conceptually, it's the "same" counter.
> >
>
> Okay, it was not clear.
>
> > >
> > > > Note: this mechanism isn't aiming at making "the most recent" (as in
> > > > time) side gaining the ownership. One side sending subsequent grab
> > > > messages earlier will likely take the ownership over a side sending a
> > > > single message simultaneously the other way. It only clears the
> > > > situation where both side believe that the other is the current
> > > > clipboard owner, by having a global ordering and priority in case of
> > > > serial conflict.
> > > >
> > > > Signed-off-by: Marc-André Lureau 
> > > > ---
> > > >  spice/vd_agent.h | 4 
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> > > > index 862cb5c..9ae3cc7 100644
> > > > --- a/spice/vd_agent.h
> > > > +++ b/spice/vd_agent.h
> > > > @@ -218,6 +218,9 @@ typedef struct SPICE_ATTR_PACKED 
> > > > VDAgentClipboardGrab
> > > > {
> > > >  #if 0 /* VD_AGENT_CAP_CLIPBOARD_SELECTION */
> > > >  uint8_t selection;
> > > >  uint8_t __reserved[sizeof(uint32_t) - 1 * sizeof(uint8_t)];
> > > > +#endif
> > > > +#if 0 /* VD_AGENT_CAP_CLIPBOARD_GRAB_SERIAL */
> > > > +uint32_t serial;
> > > >  #endif
> > >
> > > I would prefer a new message instead of partial structures.
> >
> > VDAgentClipboardGrabSelectionAndSerialAnd... ?
> >
>
> Usually a version schema is used like VDAgentClipboardGrabSelection_v2
>
> > yeah, it's not ideal. I wish we would use a better protocol, be it
> > json or protobuf etc..
> >
>
> This is not a justification to produce even worse code.
> A plain C structure would be fine, many binary protocols (like IP)
> uses this method.
> A semi-defined C structure with missing fields is the worst I
> ever seen, continuing to use is not so sensible.

Different trade-offs, version vs capabilities.

Since we don't have missing fields in the protocol, this means
VDAgentClipboardGrabSelection_v2 should have selection capability
included.

This may not be a problem for vdagent, but for many cases,
capabilities offer more flexibility than versioning.

> It would be better to not define the C structure at all.
> OT: I think it was discussed to use an external library/tool, somebody
> suggested to implement new messages with different serialization
> but nobody came with a RFC/proposal.

I guess we could start by having a gitlab issue to discuss it.

> >
> >
> > > Why not reusing part of __reserved?
> >
> > Because it's only 24 bits there, and if we make it too small, we would
> > have to deal explicitly with rounding issues.
> >
>
> I don't think a "(char)(remote_serial - local_serial) > 0" is so
> complicated to read.

I am not good at wrapped arithmetic. How would that work? for ex
(char)(255 - 100) == -101, that doesn't look right.

> But this is second to use a semi-defined structure, just a
> workaround using the already present ugly hack.
>
> > >
> > > >  uint32_t types[0];
> > > >  } V

Re: [Spice-devel] [PATCH spice-server 2/2] docs: Add some notes on event scheduling and threading