Re: [Spice-devel] [PATCH vdagent 2/2] vdagent: handle clipboard using GTK+

2018-02-18 Thread Victor Toso 
Hi,

On Sun, Feb 18, 2018 at 11:44:38PM +0100, Jakub Janku wrote:
> >> +typedef struct {
> >> +GMainLoop*loop;
> >> +GtkSelectionData *sel_data;
> >> +} DataRetrieval;
> >> +
> >> +struct VDAgentClipboards {
> >> +struct udscs_connection*conn;
> >> +
> >> +GtkClipboard   *clipboard[SELECTION_COUNT];
> >> +guint   owner[SELECTION_COUNT];
> >> +
> >> +GList  *data_retrievals[SELECTION_COUNT];
> >> +GList  *data_requests[SELECTION_COUNT];
> >
> > I think we could use something more clear instead of
> > 'data_retrievals' and 'data_requests' but I'm terrible with
> > names myself so I suggest 'pending_set_request' and
> > 'pending_get_requests' or just 'set_requests' and
> > 'get_requests'.
> 
> I agree, the names are far from ideal and rather confusing, but
> 'set_requests' and 'get_requests' aren't making it more clear
> either imho - "get request" could stand for a request from the
> client as well as a request from an app on the vdagent side.
> What about 'request_from_client' and 'request_from_app'?

Works for me :)

> > A comment would help too, like /* Client -> Guest */ and vice
> > versa.
> 
> Definitely, I should have added some comments.
> >
> >> +gpointer   *last_targets_req[SELECTION_COUNT];
> >> +
> >> +GdkAtom targets[SELECTION_COUNT][TYPE_COUNT];
> >
> > Maybe wrap them in inner struct { ... } selection[SELECTION_TYPE] ?
> >
> > e.g.
> > - c->data_requests[sel] = ...
> > + c->selection[id].data_request = ...
> >
> Makes sense probably. It might also be handy to typedef something like
> DataSelection and use
>   DataSelection *sel = >selection[sel_id];
>   sel->data_requests = ...
> where the struct members are accessed frequently.

It was more like a possible suggestion then a request but I like
it as we would access the `selection[]` array only once to get
the right struct instead of accessing the array for each field.

> >> +};
> >> +
> >> +static const struct {
> >> +guint type;
> >> +const gchar  *atom_name;
> >> +} atom2agent[] = {
> >> +{VD_AGENT_CLIPBOARD_UTF8_TEXT, "UTF8_STRING"},
> >> +{VD_AGENT_CLIPBOARD_UTF8_TEXT, "text/plain;charset=utf-8"},
> >> +{VD_AGENT_CLIPBOARD_UTF8_TEXT, "STRING"},
> >> +{VD_AGENT_CLIPBOARD_UTF8_TEXT, "TEXT"},
> >> +{VD_AGENT_CLIPBOARD_UTF8_TEXT, "text/plain"},
> >> +{VD_AGENT_CLIPBOARD_IMAGE_PNG, "image/png"},
> >> +{VD_AGENT_CLIPBOARD_IMAGE_BMP, "image/bmp"},
> >> +{VD_AGENT_CLIPBOARD_IMAGE_BMP, "image/x-bmp"},
> >> +{VD_AGENT_CLIPBOARD_IMAGE_BMP, "image/x-MS-bmp"},
> >> +{VD_AGENT_CLIPBOARD_IMAGE_BMP, "image/x-win-bitmap"},
> >> +{VD_AGENT_CLIPBOARD_IMAGE_TIFF,"image/tiff"},
> >> +{VD_AGENT_CLIPBOARD_IMAGE_JPG, "image/jpeg"},
> >> +};
> >> +
> >> +static guint get_type_from_atom(GdkAtom atom)
> >> +{
> >> +int i;
> >> +gchar *name = gdk_atom_name(atom);
> >> +for (i = 0; i < G_N_ELEMENTS(atom2agent); i++)
> >> +if (!g_ascii_strcasecmp(name, atom2agent[i].atom_name))
> >> +break;
> >> +g_free(name);
> >> +return i < G_N_ELEMENTS(atom2agent) ? atom2agent[i].type
> >> +: VD_AGENT_CLIPBOARD_NONE;
> >> +}
> >> +
> >> +/* gtk_clipboard_request_(, callback, user_data) cannot be cancelled. 
> >> Instead,
> >> +   gpointer *ref = get_weak_ref(VDAgentClipboards *) is passed to the 
> >> callback.
> >> +   The callback returns if free_weak_ref(ref) == NULL, which can be done
> >> +   by setting *ref = NULL. This substitutes cancellation of the request.
> >> + */
> >> +static gpointer *get_weak_ref(gpointer data)
> >> +{
> >> +gpointer *ref = g_new(gpointer, 1);
> >> +*ref = data;
> >> +return ref;
> >> +}
> >
> > So, this is similar to GWeakRef but we are not using a GObject at
> > all.
> >
> > I might be missing something but why not an actual structure for
> > the pending request? I think it would be easier to understand and
> > track what is going.
> >
> > At least we should have some functions with the clear meaning
> > around this, like weak_ref_cancel(gpointer ref) { /* check and
> > set to NULL */ } ... but I do think something around GCancellable
> > would be nicer or even bool is_cancelled; in a custom struct.
> >
> The get_weak_ref() is only used with callbacks since we need to pass a
> reference to VDAgentClipboards. So I'm not sure GCancellable would
> help us here.
> The struct would need to contain pointer to VDAgentClipboards as well
> as the bool is_cancelled, so using something like GWeakRef seemed to
> be the easiest solution, although creating a struct might be nicer...

Right, maybe some helper functions would be enough. For instance:

 |  /* if there's a pending request for clipboard targets, cancel it */
 |  if (c->last_targets_req[sel])
 |  *(c->last_targets_req[sel]) = NULL;

Should use some

Re: [Spice-devel] [PATCH vdagent 1/2] vdagent-x11: remove clipboard handling

2018-02-18 Thread Jakub Janku 
Hi,

On Mon, Feb 12, 2018 at 10:40 AM, Victor Toso  wrote:
> Hi,
>
> Follow up after the comment in the cover letter.
>
> On Sun, Jan 21, 2018 at 09:03:13PM +0100, Jakub Janků wrote:
>> From: Jakub Janků 
>>
>> The code will be replaced by GTK+ in the following patch.
>> ---
>>  src/vdagent/vdagent.c  |7 -
>>  src/vdagent/x11-priv.h |   91 
>>  src/vdagent/x11.c  | 1074 
>> +---
>>  src/vdagent/x11.h  |   10 -
>>  4 files changed, 1 insertion(+), 1181 deletions(-)
>>
>> diff --git a/src/vdagent/vdagent.c b/src/vdagent/vdagent.c
>> index d86ee25..92ffcf3 100644
>> --- a/src/vdagent/vdagent.c
>> +++ b/src/vdagent/vdagent.c
>> @@ -164,18 +164,12 @@ static void daemon_read_complete(struct 
>> udscs_connection **connp,
>>  vdagent_x11_set_monitor_config(agent->x11, (VDAgentMonitorsConfig 
>> *)data, 0);
>>  break;
>>  case VDAGENTD_CLIPBOARD_REQUEST:
>> -vdagent_x11_clipboard_request(agent->x11, header->arg1, 
>> header->arg2);
>>  break;
>>  case VDAGENTD_CLIPBOARD_GRAB:
>> -vdagent_x11_clipboard_grab(agent->x11, header->arg1, (uint32_t 
>> *)data,
>> -   header->size / sizeof(uint32_t));
>>  break;
>>  case VDAGENTD_CLIPBOARD_DATA:
>> -vdagent_x11_clipboard_data(agent->x11, header->arg1, header->arg2,
>> -   data, header->size);
>>  break;
>>  case VDAGENTD_CLIPBOARD_RELEASE:
>> -vdagent_x11_clipboard_release(agent->x11, header->arg1);
>>  break;
>>  case VDAGENTD_VERSION:
>>  if (strcmp((char *)data, VERSION) != 0) {
>> @@ -228,7 +222,6 @@ static void daemon_read_complete(struct udscs_connection 
>> **connp,
>>  }
>>  break;
>>  case VDAGENTD_CLIENT_DISCONNECTED:
>> -vdagent_x11_client_disconnected(agent->x11);
>
> So, instead of removing the vdagent_x11 calls, you could
> introduce the clipboard.[ch] and use the renamed calls here that
> will call the x11 code for now.

Seems reasonable (including the proposal in cover letter comment),
I'll change it with v2.

  Jakub
>
> Reviewed-by: Victor Toso 
>
> toso
>
>>  if (vdagent_finalize_file_xfer(agent)) {
>>  vdagent_init_file_xfer(agent);
>>  }
>> diff --git a/src/vdagent/x11-priv.h b/src/vdagent/x11-priv.h
>> index 3776098..627e0ed 100644
>> --- a/src/vdagent/x11-priv.h
>> +++ b/src/vdagent/x11-priv.h
>> @@ -1,118 +1,27 @@
>>  #ifndef VDAGENT_X11_PRIV
>>  #define VDAGENT_X11_PRIV
>>
>> -#include 
>> -#include 
>> -
>> -#include 
>> -
>>  #include 
>>
>> -/* Macros to print a message to the logfile prefixed by the selection */
>> -#define SELPRINTF(format, ...) \
>> -syslog(LOG_ERR, "%s: " format, \
>> -vdagent_x11_sel_to_str(selection), ##__VA_ARGS__)
>> -
>> -#define VSELPRINTF(format, ...) \
>> -do { \
>> -if (x11->debug) { \
>> -syslog(LOG_DEBUG, "%s: " format, \
>> -vdagent_x11_sel_to_str(selection), ##__VA_ARGS__); \
>> -} \
>> -} while (0)
>> -
>>  #define MAX_SCREENS 16
>>  /* Same as qxl_dev.h client_monitors_config.heads count */
>>  #define MONITOR_SIZE_COUNT 64
>>
>> -enum { owner_none, owner_guest, owner_client };
>> -
>> -/* X11 terminology is confusing a selection request is a request from an
>> -   app to get clipboard data from us, so iow from the spice client through
>> -   the vdagent channel. We handle these one at a time and queue any which
>> -   come in while we are still handling the current one. */
>> -struct vdagent_x11_selection_request {
>> -XEvent event;
>> -uint8_t selection;
>> -struct vdagent_x11_selection_request *next;
>> -};
>> -
>> -/* A conversion request is X11 speak for asking another app to give its
>> -   clipboard data to us, we do these on behalf of the spice client to copy
>> -   data from the guest to the client. Like selection requests we process
>> -   these one at a time. */
>> -struct vdagent_x11_conversion_request {
>> -Atom target;
>> -uint8_t selection;
>> -struct vdagent_x11_conversion_request *next;
>> -};
>> -
>> -struct clipboard_format_tmpl {
>> -uint32_t type;
>> -const char *atom_names[16];
>> -};
>> -
>> -struct clipboard_format_info {
>> -uint32_t type;
>> -Atom atoms[16];
>> -int atom_count;
>> -};
>> -
>>  struct monitor_size {
>>  int width;
>>  int height;
>>  };
>>
>> -static const struct clipboard_format_tmpl clipboard_format_templates[] = {
>> -{ VD_AGENT_CLIPBOARD_UTF8_TEXT, { "UTF8_STRING", 
>> "text/plain;charset=UTF-8",
>> -  "text/plain;charset=utf-8", "STRING", NULL }, },
>> -{ VD_AGENT_CLIPBOARD_IMAGE_PNG, { "image/png", NULL }, },
>> -{ VD_AGENT_CLIPBOARD_IMAGE_BMP, { "image/bmp", "image/x-bmp",
>> -  "image/x-MS-bmp", "image/x-win-bitmap", NULL }, },
>> -{

Re: [Spice-devel] [PATCH vdagent 2/2] vdagent: handle clipboard using GTK+

2018-02-18 Thread Jakub Janku 
Hi Victor,

sorry for the delayed reply.

On Mon, Feb 12, 2018 at 2:11 PM, Victor Toso  wrote:
> Hi,
>
> On Sun, Jan 21, 2018 at 09:03:14PM +0100, Jakub Janků wrote:
>> From: Jakub Janků 
>>
>> Place the code that handles clipboard
>> into a separate file - clipboard.c
>> ---
>>  Makefile.am |   2 +
>>  src/vdagent/clipboard.c | 401 
>> 
>>  src/vdagent/clipboard.h |  42 +
>>  src/vdagent/vdagent.c   |  31 +++-
>>  4 files changed, 471 insertions(+), 5 deletions(-)
>>  create mode 100644 src/vdagent/clipboard.c
>>  create mode 100644 src/vdagent/clipboard.h
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index c4bd3dd..88550c6 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -33,6 +33,8 @@ src_spice_vdagent_SOURCES = \
>>   $(common_sources)   \
>>   src/vdagent/audio.c \
>>   src/vdagent/audio.h \
>> + src/vdagent/clipboard.c \
>> + src/vdagent/clipboard.h \
>>   src/vdagent/file-xfers.c\
>>   src/vdagent/file-xfers.h\
>>   src/vdagent/x11-priv.h  \
>> diff --git a/src/vdagent/clipboard.c b/src/vdagent/clipboard.c
>> new file mode 100644
>> index 000..657a6b4
>> --- /dev/null
>> +++ b/src/vdagent/clipboard.c
>> @@ -0,0 +1,401 @@
>> +/*  clipboard.c - vdagent clipboard handling code
>> +
>> +Copyright 2017 Red Hat, Inc.
>> +
>> +This program is free software: you can redistribute it and/or modify
>> +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 General Public License for more details.
>> +
>> +You should have received a copy of the GNU General Public License
>> +along with this program.  If not, see .
>> +*/
>> +
>> +#ifdef HAVE_CONFIG_H
>> +# include 
>> +#endif
>> +
>> +#include 
>> +#include 
>> +
>> +#include "vdagentd-proto.h"
>> +#include "spice/vd_agent.h"
>> +#include "udscs.h"
>> +#include "clipboard.h"
>> +
>> +/* 2 selections supported - _SELECTION_CLIPBOARD = 0, _SELECTION_PRIMARY = 
>> 1 */
>> +#define SELECTION_COUNT (VD_AGENT_CLIPBOARD_SELECTION_PRIMARY + 1)
>> +#define TYPE_COUNT  (VD_AGENT_CLIPBOARD_IMAGE_JPG + 1)
>> +
>> +#define sel_from_clip(clipboard) \
>> +GPOINTER_TO_UINT(g_object_get_data(G_OBJECT(clipboard), 
>> "vdagent-selection"))
>> +
>> +enum {
>> +OWNER_NONE,
>> +OWNER_GUEST,
>> +OWNER_CLIENT
>> +};
>> +
>> +typedef struct {
>> +GMainLoop*loop;
>> +GtkSelectionData *sel_data;
>> +} DataRetrieval;
>> +
>> +struct VDAgentClipboards {
>> +struct udscs_connection*conn;
>> +
>> +GtkClipboard   *clipboard[SELECTION_COUNT];
>> +guint   owner[SELECTION_COUNT];
>> +
>> +GList  *data_retrievals[SELECTION_COUNT];
>> +GList  *data_requests[SELECTION_COUNT];
>
> I think we could use something more clear instead of
> 'data_retrievals' and 'data_requests' but I'm terrible with
> names myself so I suggest 'pending_set_request' and
> 'pending_get_requests' or just 'set_requests' and 'get_requests'.

I agree, the names are far from ideal and rather confusing, but
'set_requests' and 'get_requests' aren't making it more clear either
imho - "get request" could stand for a request from the client as well
as a request from an app on the vdagent side. What about
'request_from_client' and 'request_from_app'?
>
> A comment would help too, like /* Client -> Guest */ and vice
> versa.

Definitely, I should have added some comments.
>
>> +gpointer   *last_targets_req[SELECTION_COUNT];
>> +
>> +GdkAtom targets[SELECTION_COUNT][TYPE_COUNT];
>
> Maybe wrap them in inner struct { ... } selection[SELECTION_TYPE] ?
>
> e.g.
> - c->data_requests[sel] = ...
> + c->selection[id].data_request = ...
>
Makes sense probably. It might also be handy to typedef something like
DataSelection and use
  DataSelection *sel = >selection[sel_id];
  sel->data_requests = ...
where the struct members are accessed frequently.

>> +};
>> +
>> +static const struct {
>> +guint type;
>> +const gchar  *atom_name;
>> +} atom2agent[] = {
>> +{VD_AGENT_CLIPBOARD_UTF8_TEXT, "UTF8_STRING"},
>> +{VD_AGENT_CLIPBOARD_UTF8_TEXT, "text/plain;charset=utf-8"},
>> +{VD_AGENT_CLIPBOARD_UTF8_TEXT, "STRING"},
>> +{VD_AGENT_CLIPBOARD_UTF8_TEXT, "TEXT"},
>> +{VD_AGENT_CLIPBOARD_UTF8_TEXT, "text/plain"},
>> +

[Spice-devel] [PATCH spice-server 4/8] stream-device: Disable guest device on errors

2018-02-18 Thread Frediano Ziglio 
To communicate the error and avoiding to have to read any data the
guest want to write disable the device.

Signed-off-by: Frediano Ziglio 
---
 server/stream-device.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/server/stream-device.c b/server/stream-device.c
index f22946fb..688a4e07 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -94,6 +94,7 @@ stream_device_partial_read(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 while (sif->read(sin, buf, sizeof(buf)) > 0) {
 continue;
 }
+sif->state(sin, 0);
 return false;
 }
 
@@ -560,6 +561,19 @@ reset_channels(StreamDevice *dev)
 }
 }
 
+static void
+char_device_enable(RedCharDevice *char_dev)
+{
+SpiceCharDeviceInstance *sin = NULL;
+g_object_get(char_dev, "sin", , NULL);
+spice_assert(sin != NULL);
+
+SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
+if (sif->state) {
+sif->state(sin, 1);
+}
+}
+
 static void
 stream_device_port_event(RedCharDevice *char_dev, uint8_t event)
 {
@@ -580,6 +594,12 @@ stream_device_port_event(RedCharDevice *char_dev, uint8_t 
event)
 dev->flow_stopped = false;
 red_char_device_reset(char_dev);
 reset_channels(dev);
+
+// enable the device again. We try to enable it even on close as
+// this could prevent a possible race condition where we open the
+// device from the guest and it take some time to enable causing
+// temporary writing issues.
+char_device_enable(char_dev);
 }
 
 static void
-- 
2.14.3

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

[Spice-devel] [PATCH spice-server 3/8] stream-device: Implements properly device reset on open/close

2018-02-18 Thread Frediano Ziglio 
Due to the way Qemu handle the device we must consume all pending
data inside the callback to read data from the device.
We need to consume all data from previous device dialog to avoid
that next device usage is still seeing old data.
This as Qemu return 0 if you call SpiceCharDeviceInterface::read
outside this callback (which is called by Qemu using
spice_server_char_device_wakeup).
On the test now we must test that we receive an error from the device.
This as previously we checked that last part of the data was not read
but now potentially all data are readed so we need another way to check
the device detected the error.

Signed-off-by: Frediano Ziglio 
---
 server/stream-device.c|  15 +-
 server/tests/test-stream-device.c | 108 +-
 2 files changed, 85 insertions(+), 38 deletions(-)

diff --git a/server/stream-device.c b/server/stream-device.c
index cbd34463..f22946fb 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -84,11 +84,22 @@ stream_device_partial_read(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 int n;
 bool handled = false;
 
-if (dev->has_error || dev->flow_stopped || !dev->stream_channel) {
+sif = spice_char_device_get_interface(sin);
+
+// in order to get in sync every time we open the device we need
+// to discard data here. This as Qemu keeps a buffer of data which
+// is used only during spice_server_char_device_wakeup from Qemu
+if (G_UNLIKELY(dev->has_error)) {
+uint8_t buf[16 * 1024];
+while (sif->read(sin, buf, sizeof(buf)) > 0) {
+continue;
+}
 return false;
 }
 
-sif = spice_char_device_get_interface(sin);
+if (dev->flow_stopped || !dev->stream_channel) {
+return false;
+}
 
 /* read header */
 while (dev->hdr_pos < sizeof(dev->hdr)) {
diff --git a/server/tests/test-stream-device.c 
b/server/tests/test-stream-device.c
index 72b3f382..a75fe83c 100644
--- a/server/tests/test-stream-device.c
+++ b/server/tests/test-stream-device.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -42,13 +43,20 @@ static unsigned pos;
 // then the size is reach we move on next one till exausted
 static unsigned message_sizes[16];
 static unsigned *message_sizes_end, *message_sizes_curr;
+static bool device_enabled = false;
+
+static unsigned vmc_write_pos;
+static uint8_t vmc_write_buf[2048];
 
 // handle writes to the device
 static int vmc_write(SPICE_GNUC_UNUSED SpiceCharDeviceInstance *sin,
  SPICE_GNUC_UNUSED const uint8_t *buf,
  int len)
 {
-// currently we don't test anything on write so reply we wrote it
+// just copy into the buffer
+unsigned copy = MIN(sizeof(vmc_write_buf) - vmc_write_pos, len);
+memcpy(vmc_write_buf+vmc_write_pos, buf, copy);
+vmc_write_pos += copy;
 return len;
 }
 
@@ -81,6 +89,7 @@ static int vmc_read(SPICE_GNUC_UNUSED SpiceCharDeviceInstance 
*sin,
 static void vmc_state(SPICE_GNUC_UNUSED SpiceCharDeviceInstance *sin,
   SPICE_GNUC_UNUSED int connected)
 {
+device_enabled = !!connected;
 }
 
 static SpiceCharDeviceInterface vmc_interface = {
@@ -126,57 +135,80 @@ static uint8_t *add_format(uint8_t *p, uint32_t w, 
uint32_t h, SpiceVideoCodecTy
 return p + sizeof(fmt);
 }
 
+// check we have an error message on the write buffer
+static void
+check_vmc_error_message(void)
+{
+StreamDevHeader hdr;
+
+g_assert(vmc_write_pos >= sizeof(hdr));
+
+memcpy(, vmc_write_buf, sizeof(hdr));
+g_assert_cmpint(hdr.protocol_version, ==, STREAM_DEVICE_PROTOCOL);
+g_assert_cmpint(GUINT16_FROM_LE(hdr.type), ==, STREAM_TYPE_NOTIFY_ERROR);
+g_assert_cmpint(GUINT32_FROM_LE(hdr.size), <=, vmc_write_pos - 
sizeof(hdr));
+}
+
 static void test_stream_device(void)
 {
 uint8_t *p = message;
 SpiceCoreInterface *core = basic_event_loop_init();
 Test *test = test_new(core);
 
-message_sizes_curr = message_sizes;
-message_sizes_end = message_sizes;
+for (int test_num=0; test_num < 2; ++test_num) {
+pos = 0;
+vmc_write_pos = 0;
+message_sizes_curr = message_sizes;
+message_sizes_end = message_sizes;
 
-// add some messages into device buffer
-// here we are testing the device is reading at least two
-// consecutive format messages
-// first message part has 2 extra bytes to check for header split
-p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_MJPEG);
-*message_sizes_end = p - message + 2;
-++message_sizes_end;
+// add some messages into device buffer
+// here we are testing the device is reading at least two
+// consecutive format messages
+// first message part has 2 extra bytes to check for header split
+p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_MJPEG);
+*message_sizes_end = p - message + 2;
+

[Spice-devel] [PATCH spice-server 5/8] stream-device: Workaround Qemu bug closing device

2018-02-18 Thread Frediano Ziglio 
Previous patch causes a bug into Qemu if the patch
46764fe09ca2e0f15c0981a672c166ed8cf57e72 ("virtio-serial: fix segfault
on disconnect") is not include in that version of Qemu.
This crash happens when device is closed inside a write operation.
For SPICE character device a spice_server_char_device_wakeup is called
to write data which handles both read and write pending operations.
As we want to close the device but we can't do it inside the handler
operation schedule a timer that will close the guest device outside
this callback.
The proper solution would be to patch Qemu but making sure this
is not so easy so this workaround patch.
Code is marked with some comments to remember to remove this
hack in a safe future.

Signed-off-by: Frediano Ziglio 
---
 server/stream-device.c | 34 ++
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/server/stream-device.c b/server/stream-device.c
index 688a4e07..170c8637 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -57,6 +57,7 @@ struct StreamDevice {
 bool flow_stopped;
 StreamChannel *stream_channel;
 CursorChannel *cursor_channel;
+SpiceTimer *close_timer;
 };
 
 struct StreamDeviceClass {
@@ -68,6 +69,8 @@ static StreamDevice 
*stream_device_new(SpiceCharDeviceInstance *sin, RedsState *
 
 G_DEFINE_TYPE(StreamDevice, stream_device, RED_TYPE_CHAR_DEVICE)
 
+static void char_device_set_state(RedCharDevice *char_dev, int state);
+
 typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance *sin)
 SPICE_GNUC_WARN_UNUSED_RESULT;
 
@@ -77,6 +80,16 @@ static StreamMsgHandler handle_msg_format, handle_msg_data, 
handle_msg_cursor_se
 static bool handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance *sin,
const char *error_msg) 
SPICE_GNUC_WARN_UNUSED_RESULT;
 
+static void
+close_timer_func(void *opaque)
+{
+StreamDevice *dev = opaque;
+
+if (dev->opened && dev->has_error) {
+char_device_set_state(RED_CHAR_DEVICE(dev), 0);
+}
+}
+
 static bool
 stream_device_partial_read(StreamDevice *dev, SpiceCharDeviceInstance *sin)
 {
@@ -94,7 +107,17 @@ stream_device_partial_read(StreamDevice *dev, 
SpiceCharDeviceInstance *sin)
 while (sif->read(sin, buf, sizeof(buf)) > 0) {
 continue;
 }
-sif->state(sin, 0);
+
+// This code is a workaround for a Qemu bug, see patch
+// "stream-device: Workaround Qemu bug closing device"
+// as calling sif->state here can cause a crash schedule
+// a timer and do the call in it. Remove this code when
+// will be sure all Qemu versions has been patched.
+RedsState *reds = red_char_device_get_server(RED_CHAR_DEVICE(dev));
+if (!dev->close_timer) {
+dev->close_timer = reds_core_timer_add(reds, close_timer_func, 
dev);
+}
+reds_core_timer_start(reds, dev->close_timer, 0);
 return false;
 }
 
@@ -501,6 +524,9 @@ stream_device_dispose(GObject *object)
 {
 StreamDevice *dev = STREAM_DEVICE(object);
 
+RedsState *reds = red_char_device_get_server(RED_CHAR_DEVICE(dev));
+reds_core_timer_remove(reds, dev->close_timer);
+
 if (dev->stream_channel) {
 // close all current connections and drop the reference
 red_channel_destroy(RED_CHANNEL(dev->stream_channel));
@@ -562,7 +588,7 @@ reset_channels(StreamDevice *dev)
 }
 
 static void
-char_device_enable(RedCharDevice *char_dev)
+char_device_set_state(RedCharDevice *char_dev, int state)
 {
 SpiceCharDeviceInstance *sin = NULL;
 g_object_get(char_dev, "sin", , NULL);
@@ -570,7 +596,7 @@ char_device_enable(RedCharDevice *char_dev)
 
 SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
 if (sif->state) {
-sif->state(sin, 1);
+sif->state(sin, state);
 }
 }
 
@@ -599,7 +625,7 @@ stream_device_port_event(RedCharDevice *char_dev, uint8_t 
event)
 // this could prevent a possible race condition where we open the
 // device from the guest and it take some time to enable causing
 // temporary writing issues.
-char_device_enable(char_dev);
+char_device_set_state(char_dev, 1);
 }
 
 static void
-- 
2.14.3

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

[Spice-devel] [PATCH spice-server 1/8] test-stream-device: Better Qemu emulation for data reading

2018-02-18 Thread Frediano Ziglio 
Qemu does not trigger a new data read if we don't read all data in
the buffer.

Signed-off-by: Frediano Ziglio 
---
 server/tests/test-stream-device.c | 43 +--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/server/tests/test-stream-device.c 
b/server/tests/test-stream-device.c
index 656bf56b..edd618e2 100644
--- a/server/tests/test-stream-device.c
+++ b/server/tests/test-stream-device.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -68,8 +69,12 @@ static int vmc_read(SPICE_GNUC_UNUSED 
SpiceCharDeviceInstance *sin,
 pos += ret;
 // kick off next message read
 // currently Qemu kicks the device so we need to do it manually
-// here
-spice_server_char_device_wakeup(_instance);
+// here. If not all data are read the device goes into blocking
+// state and we get the wake only when we read from the device
+// again
+if (pos >= *message_sizes_curr) {
+spice_server_char_device_wakeup(_instance);
+}
 return ret;
 }
 
@@ -177,11 +182,45 @@ static void test_stream_device(void)
 basic_event_loop_destroy();
 }
 
+// check if sending a partial message cause issues
+static void test_stream_device_unfinished(void)
+{
+uint8_t *p = message;
+SpiceCoreInterface *core = basic_event_loop_init();
+Test *test = test_new(core);
+
+pos = 0;
+message_sizes_curr = message_sizes;
+message_sizes_end = message_sizes;
+
+// this long and not finished message should not cause an infinite loop
+p = add_stream_hdr(p, STREAM_TYPE_DATA, 10);
+*message_sizes_end = p - message;
+++message_sizes_end;
+
+vmc_instance.base.sif = _interface.base;
+spice_server_add_interface(test->server, _instance.base);
+
+// we need to open the device and kick the start
+// the alarm is to avoid program to stuck
+alarm(5);
+spice_server_port_event(_instance, SPICE_PORT_EVENT_OPENED);
+spice_server_char_device_wakeup(_instance);
+alarm(0);
+
+// we should read all data
+g_assert(message_sizes_curr - message_sizes == 1);
+
+test_destroy(test);
+basic_event_loop_destroy();
+}
+
 int main(int argc, char *argv[])
 {
 g_test_init(, , NULL);
 
 g_test_add_func("/server/stream-device", test_stream_device);
+g_test_add_func("/server/stream-device-unfinished", 
test_stream_device_unfinished);
 
 return g_test_run();
 }
-- 
2.14.3

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

[Spice-devel] [PATCH spice-server 8/8] stream-channel: Send the full frame in a single message

2018-02-18 Thread Frediano Ziglio 
The current implementation of server and client assume that a single
data message contains an encoded frame.
This is not a problem for most encoding but for MJPEG this causes
the client to fail decoding.

Signed-off-by: Frediano Ziglio 
---
 server/stream-channel.c | 97 ++---
 server/stream-channel.h | 12 ++
 server/stream-device.c  |  7 ++--
 3 files changed, 100 insertions(+), 16 deletions(-)

diff --git a/server/stream-channel.c b/server/stream-channel.c
index 88f859f6..79addec0 100644
--- a/server/stream-channel.c
+++ b/server/stream-channel.c
@@ -44,6 +44,7 @@
 
 typedef struct StreamChannelClient StreamChannelClient;
 typedef struct StreamChannelClientClass StreamChannelClientClass;
+typedef struct StreamDataItem StreamDataItem;
 
 /* we need to inherit from CommonGraphicsChannelClient
  * to get buffer handling */
@@ -74,6 +75,10 @@ struct StreamChannel {
 
 StreamQueueStat queue_stat;
 
+/* pending partial data item */
+StreamDataItem *data_item;
+uint32_t data_item_pos;
+
 /* callback to notify when a stream should be started or stopped */
 stream_channel_start_proc start_cb;
 void *start_opaque;
@@ -104,12 +109,12 @@ typedef struct StreamCreateItem {
 SpiceMsgDisplayStreamCreate stream_create;
 } StreamCreateItem;
 
-typedef struct StreamDataItem {
+struct StreamDataItem {
 RedPipeItem base;
 StreamChannel *channel;
 // NOTE: this must be the last field in the structure
 SpiceMsgDisplayStreamData data;
-} StreamDataItem;
+};
 
 #define PRIMARY_SURFACE_ID 0
 
@@ -129,6 +134,18 @@ stream_channel_client_init(StreamChannelClient *client)
 client->stream_id = -1;
 }
 
+static void
+stream_channel_unref_data_item(StreamChannel *channel)
+{
+if (channel->data_item) {
+// this is required in order to update statistics correctly
+channel->data_item->data.data_size = 0;
+red_pipe_item_unref(>data_item->base);
+channel->data_item = NULL;
+channel->data_item_pos = 0;
+}
+}
+
 static void
 request_new_stream(StreamChannel *channel, StreamMsgStartStop *start)
 {
@@ -152,6 +169,7 @@ stream_channel_client_on_disconnect(RedChannelClient *rcc)
 channel->stream_id = -1;
 channel->width = 0;
 channel->height = 0;
+stream_channel_unref_data_item(channel);
 
 // send stream stop to device
 StreamMsgStartStop stop = { 0, };
@@ -424,6 +442,16 @@ stream_channel_constructed(GObject *object)
 reds_register_channel(reds, red_channel);
 }
 
+static void
+stream_channel_finalize(GObject *object)
+{
+StreamChannel *channel = STREAM_CHANNEL(object);
+
+stream_channel_unref_data_item(channel);
+
+G_OBJECT_CLASS(stream_channel_parent_class)->finalize(object);
+}
+
 static void
 stream_channel_class_init(StreamChannelClass *klass)
 {
@@ -431,6 +459,7 @@ stream_channel_class_init(StreamChannelClass *klass)
 RedChannelClass *channel_class = RED_CHANNEL_CLASS(klass);
 
 object_class->constructed = stream_channel_constructed;
+object_class->finalize = stream_channel_finalize;
 
 channel_class->parser = 
spice_get_client_channel_parser(SPICE_CHANNEL_DISPLAY, NULL);
 channel_class->handle_message = handle_message;
@@ -508,6 +537,39 @@ data_item_free(RedPipeItem *base)
 g_free(pipe_item);
 }
 
+static StreamDataItem*
+stream_channel_new_data_item(StreamChannel *channel, size_t size, uint32_t 
mm_time)
+{
+stream_channel_unref_data_item(channel);
+
+StreamDataItem *item = g_malloc(sizeof(*item) + size);
+red_pipe_item_init_full(>base, RED_PIPE_ITEM_TYPE_STREAM_DATA,
+data_item_free);
+item->data.base.id = channel->stream_id;
+item->data.base.multi_media_time = mm_time;
+item->data.data_size = size;
+item->channel = channel;
+
+channel->data_item = item;
+channel->data_item_pos = 0;
+
+return item;
+}
+
+void
+stream_channel_start_data(StreamChannel *channel, size_t size, uint32_t 
mm_time)
+{
+// see stream_channel_send_data comment
+if (channel->stream_id < 0) {
+return;
+}
+
+// TODO this collects all chunks in a single message
+// up: we send a single frame together (more compatible)
+// down: guest can cause a crash due to DoS. As a safe measure we limit 
the maximum message
+stream_channel_new_data_item(channel, MIN(size, 32*1024*1024), mm_time);
+}
+
 void
 stream_channel_send_data(StreamChannel *channel, const void *data, size_t 
size, uint32_t mm_time)
 {
@@ -520,17 +582,25 @@ stream_channel_send_data(StreamChannel *channel, const 
void *data, size_t size,
 
 RedChannel *red_channel = RED_CHANNEL(channel);
 
-StreamDataItem *item = g_malloc(sizeof(*item) + size);
-red_pipe_item_init_full(>base, RED_PIPE_ITEM_TYPE_STREAM_DATA,
-data_item_free);
-item->data.base.id = channel->stream_id;
-item->data.base.multi_media_time = mm_time;
-

[Spice-devel] [PATCH spice-server 7/8] test-stream-device: Check we don't read past data message

2018-02-18 Thread Frediano Ziglio 
Test case for the issue fixed by previous commit.

Signed-off-by: Frediano Ziglio 
---
 server/tests/test-stream-device.c | 42 +++
 1 file changed, 42 insertions(+)

diff --git a/server/tests/test-stream-device.c 
b/server/tests/test-stream-device.c
index a75fe83c..db812d49 100644
--- a/server/tests/test-stream-device.c
+++ b/server/tests/test-stream-device.c
@@ -286,6 +286,47 @@ static void test_stream_device_multiple(void)
 basic_event_loop_destroy();
 }
 
+// check if data message consume even following message
+static void test_stream_device_format_after_data(void)
+{
+uint8_t *p = message;
+SpiceCoreInterface *core = basic_event_loop_init();
+Test *test = test_new(core);
+
+pos = 0;
+vmc_write_pos = 0;
+message_sizes_curr = message_sizes;
+message_sizes_end = message_sizes;
+
+// add some messages into device buffer
+p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_MJPEG);
+p = add_stream_hdr(p, STREAM_TYPE_DATA, 5);
+memcpy(p, "hello", 5);
+p += 5;
+p = add_stream_hdr(p, STREAM_TYPE_INVALID, 0);
+*message_sizes_end = p - message;
+++message_sizes_end;
+
+vmc_instance.base.sif = _interface.base;
+spice_server_add_interface(test->server, _instance.base);
+
+// we need to open the device and kick the start
+// the alarm is to avoid program to stuck
+alarm(5);
+spice_server_port_event(_instance, SPICE_PORT_EVENT_OPENED);
+spice_server_char_device_wakeup(_instance);
+alarm(0);
+
+// we should read all data
+g_assert(message_sizes_curr - message_sizes == 1);
+
+// we should have an error back
+check_vmc_error_message();
+
+test_destroy(test);
+basic_event_loop_destroy();
+}
+
 int main(int argc, char *argv[])
 {
 g_test_init(, , NULL);
@@ -293,6 +334,7 @@ int main(int argc, char *argv[])
 g_test_add_func("/server/stream-device", test_stream_device);
 g_test_add_func("/server/stream-device-unfinished", 
test_stream_device_unfinished);
 g_test_add_func("/server/stream-device-multiple", 
test_stream_device_multiple);
+g_test_add_func("/server/stream-device-format-after-data", 
test_stream_device_format_after_data);
 
 return g_test_run();
 }
-- 
2.14.3

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

[Spice-devel] [PATCH spice-server 2/8] test-stream-device: Test batched multiple messages

2018-02-18 Thread Frediano Ziglio 
Test all batched (send together) messages are handled correctly
and device is not stuck.

Signed-off-by: Frediano Ziglio 
---
 server/tests/test-stream-device.c | 36 
 1 file changed, 36 insertions(+)

diff --git a/server/tests/test-stream-device.c 
b/server/tests/test-stream-device.c
index edd618e2..72b3f382 100644
--- a/server/tests/test-stream-device.c
+++ b/server/tests/test-stream-device.c
@@ -215,12 +215,48 @@ static void test_stream_device_unfinished(void)
 basic_event_loop_destroy();
 }
 
+// check if sending multiple messages cause stall
+static void test_stream_device_multiple(void)
+{
+uint8_t *p = message;
+SpiceCoreInterface *core = basic_event_loop_init();
+Test *test = test_new(core);
+
+pos = 0;
+message_sizes_curr = message_sizes;
+message_sizes_end = message_sizes;
+
+// add some messages into device buffer
+p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_MJPEG);
+p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_MJPEG);
+p = add_format(p, 640, 480, SPICE_VIDEO_CODEC_TYPE_MJPEG);
+*message_sizes_end = p - message;
+++message_sizes_end;
+
+vmc_instance.base.sif = _interface.base;
+spice_server_add_interface(test->server, _instance.base);
+
+// we need to open the device and kick the start
+// the alarm is to avoid program to stuck
+alarm(5);
+spice_server_port_event(_instance, SPICE_PORT_EVENT_OPENED);
+spice_server_char_device_wakeup(_instance);
+alarm(0);
+
+// we should read all data
+g_assert(message_sizes_curr - message_sizes == 1);
+
+test_destroy(test);
+basic_event_loop_destroy();
+}
+
 int main(int argc, char *argv[])
 {
 g_test_init(, , NULL);
 
 g_test_add_func("/server/stream-device", test_stream_device);
 g_test_add_func("/server/stream-device-unfinished", 
test_stream_device_unfinished);
+g_test_add_func("/server/stream-device-multiple", 
test_stream_device_multiple);
 
 return g_test_run();
 }
-- 
2.14.3

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

[Spice-devel] [PATCH spice-server 6/8] stream-device: Do not read past data message

2018-02-18 Thread Frediano Ziglio 
If data message is followed by another message was theoretically
possible that device looses the sync with the guest.
The actual Qemu and agent implementation avoids it but better
to avoid it.

Signed-off-by: Frediano Ziglio 
---
 server/stream-device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/stream-device.c b/server/stream-device.c
index 170c8637..ddac0ca9 100644
--- a/server/stream-device.c
+++ b/server/stream-device.c
@@ -281,7 +281,7 @@ handle_msg_data(StreamDevice *dev, SpiceCharDeviceInstance 
*sin)
 
 while (1) {
 uint8_t buf[16 * 1024];
-n = sif->read(sin, buf, sizeof(buf));
+n = sif->read(sin, buf, MIN(sizeof(buf), dev->hdr.size));
 if (n <= 0) {
 break;
 }
-- 
2.14.3

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

[Spice-devel] [PATCH spice-server 0/8] stream-device tests and stability patches

2018-02-18 Thread Frediano Ziglio 
These patches fix some problem with streaming device adding also tests
for different issues.
Most of the patches was sent before on ML but still, for different
reason not merged (one a problem with Qemu bug for which one there
is a workaround in this series).
Last patch fix MJPEG encoding and remove some client warnings
due to frame data split into multiple message data.

Frediano Ziglio (8):
  test-stream-device: Better Qemu emulation for data reading
  test-stream-device: Test batched multiple messages
  stream-device: Implements properly device reset on open/close
  stream-device: Disable guest device on errors
  stream-device: Workaround Qemu bug closing device
  stream-device: Do not read past data message
  test-stream-device: Check we don't read past data message
  stream-channel: Send the full frame in a single message

 server/stream-channel.c   |  97 +++---
 server/stream-channel.h   |  12 +++
 server/stream-device.c|  70 +++--
 server/tests/test-stream-device.c | 205 +-
 4 files changed, 339 insertions(+), 45 deletions(-)

-- 
2.14.3

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

Re: [Spice-devel] [PATCH spice-streaming-agent] Remove unused macro

2018-02-18 Thread Frediano Ziglio 

> Signed-off-by: Snir Sheriber 

Acked

> ---
>  src/mjpeg-fallback.cpp | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
> index 634864f..53d50ed 100644
> --- a/src/mjpeg-fallback.cpp
> +++ b/src/mjpeg-fallback.cpp
> @@ -26,9 +26,6 @@ using namespace spice::streaming_agent;
>  throw std::runtime_error(_s.str()); \
>  } while(0)
>  
> -#define FBC_ERROR(function) \
> -ERROR(function " failed(" << fbcStatus << "): " <<
> pFn.nvFBCGetLastErrorStr(fbcHandle))
> -
>  static inline uint64_t get_time()
>  {
>  timespec now;
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice-streaming-agent] Remove unused macro

2018-02-18 Thread Snir Sheriber 
Signed-off-by: Snir Sheriber 
---
 src/mjpeg-fallback.cpp | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp
index 634864f..53d50ed 100644
--- a/src/mjpeg-fallback.cpp
+++ b/src/mjpeg-fallback.cpp
@@ -26,9 +26,6 @@ using namespace spice::streaming_agent;
 throw std::runtime_error(_s.str()); \
 } while(0)
 
-#define FBC_ERROR(function) \
-ERROR(function " failed(" << fbcStatus << "): " << 
pFn.nvFBCGetLastErrorStr(fbcHandle))
-
 static inline uint64_t get_time()
 {
 timespec now;
-- 
2.14.3

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

[Spice-devel] [PATCH vdagent-win] vdagent: fix loss of mouse movement events

2018-02-18 Thread free.user.name 
send_input() may not be immediately called from handle_mouse_event() on
movement. INPUT structure is generated and stored and a timer may be set
instead. If subsequent call to handle_mouse_event() occurs before timer
expires, prepared INPUT structure gets overwritten and MOUSEEVENTF_MOVE
bit is lost. Windows doesn't see updated mouse position as the result.

Make handle_mouse_event() merely store the new mouse state, and move
INPUT structure generation to send_input(). Shuffle new mouse state to
previous only after mouse events are submitted to SendInput() Windows
API function.

Signed-off-by: free.user.name 
---
This fixes a very annoying mouse input bug in Windows 7/10 guests for
me. Whenever vdagent-win is handling mouse input, mouse pointer position
seen by the guest sometimes lags behind the cursor drawn on screen,
which makes it impossible to reliably click on anything.

 vdagent/vdagent.cpp | 132 +---
 1 file changed, 63 insertions(+), 69 deletions(-)

diff --git a/vdagent/vdagent.cpp b/vdagent/vdagent.cpp
index f00fbf5..89b3c6a 100644
--- a/vdagent/vdagent.cpp
+++ b/vdagent/vdagent.cpp
@@ -89,8 +89,7 @@ private:
 void on_clipboard_grab();
 void on_clipboard_request(UINT format);
 void on_clipboard_release();
-DWORD get_buttons_change(DWORD last_buttons_state, DWORD new_buttons_state,
- DWORD mask, DWORD down_flag, DWORD up_flag);
+DWORD get_buttons_change(DWORD mask, DWORD down_flag, DWORD up_flag);
 static HGLOBAL utf8_alloc(LPCSTR data, int size);
 static LRESULT CALLBACK wnd_proc(HWND hwnd, UINT message, WPARAM wparam, 
LPARAM lparam);
 static DWORD WINAPI event_thread_proc(LPVOID param);
@@ -131,10 +130,8 @@ private:
 int _system_version;
 int _clipboard_owner;
 DWORD _clipboard_tick;
-DWORD _buttons_state;
-ULONG _mouse_x;
-ULONG _mouse_y;
-INPUT _input;
+VDAgentMouseState _new_mouse;
+VDAgentMouseState _last_mouse;
 DWORD _input_time;
 HANDLE _control_event;
 HANDLE _stop_event;
@@ -191,9 +188,6 @@ VDAgent::VDAgent()
 , _remove_clipboard_listener (NULL)
 , _clipboard_owner (owner_none)
 , _clipboard_tick (0)
-, _buttons_state (0)
-, _mouse_x (0)
-, _mouse_y (0)
 , _input_time (0)
 , _control_event (NULL)
 , _stop_event (NULL)
@@ -221,7 +215,8 @@ VDAgent::VDAgent()
 swprintf_s(log_path, MAX_PATH, VD_AGENT_LOG_PATH, temp_path);
 _log = VDLog::get(log_path);
 }
-ZeroMemory(&_input, sizeof(_input));
+ZeroMemory(&_new_mouse, sizeof(_new_mouse));
+ZeroMemory(&_last_mouse, sizeof(_last_mouse));
 ZeroMemory(&_read_overlapped, sizeof(_read_overlapped));
 ZeroMemory(&_write_overlapped, sizeof(_write_overlapped));
 ZeroMemory(_read_buf, sizeof(_read_buf));
@@ -522,13 +517,12 @@ void VDAgent::event_dispatcher(DWORD timeout, DWORD 
wake_mask)
 }
 }
 
-DWORD VDAgent::get_buttons_change(DWORD last_buttons_state, DWORD 
new_buttons_state,
-  DWORD mask, DWORD down_flag, DWORD up_flag)
+DWORD VDAgent::get_buttons_change(DWORD mask, DWORD down_flag, DWORD up_flag)
 {
 DWORD ret = 0;
-if (!(last_buttons_state & mask) && (new_buttons_state & mask)) {
+if (!(_last_mouse.buttons & mask) && (_new_mouse.buttons & mask)) {
 ret = down_flag;
-} else if ((last_buttons_state & mask) && !(new_buttons_state & mask)) {
+} else if ((_last_mouse.buttons & mask) && !(_new_mouse.buttons & mask)) {
 ret = up_flag;
 }
 return ret;
@@ -536,101 +530,101 @@ DWORD VDAgent::get_buttons_change(DWORD 
last_buttons_state, DWORD new_buttons_st
 
 bool VDAgent::send_input()
 {
+DisplayMode* mode = NULL;
+DWORD mouse_move = 0;
+DWORD buttons_change = 0;
+DWORD mouse_wheel = 0;
 bool ret = true;
-_desktop_layout->lock();
+INPUT input;
+
 if (_pending_input) {
 if (KillTimer(_hwnd, VD_TIMER_ID)) {
 _pending_input = false;
 } else {
 vd_printf("KillTimer failed: %lu", GetLastError());
 _running = false;
-_desktop_layout->unlock();
 return false;
 }
 }
-if (!SendInput(1, &_input, sizeof(INPUT))) {
-DWORD err = GetLastError();
-// Don't stop agent due to UIPI blocking, which is usually only for 
specific windows
-// of system security applications (anti-viruses etc.)
-if (err != ERROR_SUCCESS && err != ERROR_ACCESS_DENIED) {
-vd_printf("SendInput failed: %lu", err);
-ret = _running = false;
-}
-}
-_input_time = GetTickCount();
-_desktop_layout->unlock();
-return ret;
-}
-
-bool VDAgent::handle_mouse_event(VDAgentMouseState* state)
-{
-DisplayMode* mode = NULL;
-DWORD mouse_move = 0;
-DWORD buttons_change = 0;
-DWORD mouse_wheel = 0;
-bool ret = true;
 
 ASSERT(_desktop_layout);