[Spice-devel] Do endian swapping.

Submitted by Michal Suchanek on Nov. 28, 2016, 2:08 p.m.

Details

Message ID 20161128140834.9419-1-msuchanek@suse.de
State New
Headers show
Series "Do endian swapping." ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Michal Suchanek Nov. 28, 2016, 2:08 p.m.
This allows running big endian and little endian guest side by side using
cut&paste between them.

There is some general design idea that swapping should come as cloce to
virtio_read/virtio_write as possible. In particular, the protocol between
vdagent and vdagentd is guest-specific and in native endian. With muliple
layers of headers this is a bit tricky. A few message types have to be swapped
fully before passing through vdagentd.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 src/vdagentd/uinput.c      |  4 +++
 src/vdagentd/vdagentd.c    | 68 ++++++++++++++++++++++++++++++++++------------
 src/vdagentd/virtio-port.c | 35 +++++++++++++++---------
 3 files changed, 76 insertions(+), 31 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/vdagentd/uinput.c b/src/vdagentd/uinput.c
index e2966c4..21292cb 100644
--- a/src/vdagentd/uinput.c
+++ b/src/vdagentd/uinput.c
@@ -200,6 +200,10 @@  void vdagentd_uinput_do_mouse(struct vdagentd_uinput **uinputp,
     };
     int i, down;
 
+    mouse->x = le32toh(mouse->x);
+    mouse->y = le32toh(mouse->y);
+    mouse->buttons = le32toh(mouse->buttons);
+
     if (*uinputp) {
         if (mouse->display_id >= uinput->screen_count) {
             syslog(LOG_WARNING, "mouse event for unknown monitor (%d >= %d)",
diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
index a1faf23..f91434d 100644
--- a/src/vdagentd/vdagentd.c
+++ b/src/vdagentd/vdagentd.c
@@ -83,7 +83,7 @@  static void send_capabilities(struct vdagent_virtio_port *vport,
     uint32_t request)
 {
     VDAgentAnnounceCapabilities *caps;
-    uint32_t size;
+    uint32_t size, i;
 
     size = sizeof(*caps) + VD_AGENT_CAPS_BYTES;
     caps = calloc(1, size);
@@ -92,7 +92,7 @@  static void send_capabilities(struct vdagent_virtio_port *vport,
         return;
     }
 
-    caps->request = request;
+    caps->request = htole32(request);
     VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MOUSE_STATE);
     VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MONITORS_CONFIG);
     VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_REPLY);
@@ -102,6 +102,8 @@  static void send_capabilities(struct vdagent_virtio_port *vport,
     VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_GUEST_LINEEND_LF);
     VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MAX_CLIPBOARD);
     VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_AUDIO_VOLUME_SYNC);
+    for (i = 0; i < VD_AGENT_CAPS_SIZE; i++)
+        caps->caps[i] = le32toh(caps->caps[i]);
 
     vdagent_virtio_port_write(vport, VDP_CLIENT_PORT,
                               VD_AGENT_ANNOUNCE_CAPABILITIES, 0,
@@ -122,7 +124,10 @@  static void do_client_monitors(struct vdagent_virtio_port *vport, int port_nr,
     VDAgentMessage *message_header, VDAgentMonitorsConfig *new_monitors)
 {
     VDAgentReply reply;
-    uint32_t size;
+    uint32_t size, i;
+
+    new_monitors->num_of_monitors = le32toh(new_monitors->num_of_monitors);
+    new_monitors->flags = le32toh(new_monitors->flags);
 
     /* Store monitor config to send to agents when they connect */
     size = sizeof(VDAgentMonitorsConfig) +
@@ -132,6 +137,14 @@  static void do_client_monitors(struct vdagent_virtio_port *vport, int port_nr,
         return;
     }
 
+    for (i = 0; i < new_monitors->num_of_monitors; i++) {
+        new_monitors->monitors[i].height = le32toh(new_monitors->monitors[i].height);
+        new_monitors->monitors[i].width = le32toh(new_monitors->monitors[i].width);
+        new_monitors->monitors[i].depth = le32toh(new_monitors->monitors[i].depth);
+        new_monitors->monitors[i].x = le32toh(new_monitors->monitors[i].x);
+        new_monitors->monitors[i].y = le32toh(new_monitors->monitors[i].y);
+    }
+
     vdagentd_write_xorg_conf(new_monitors);
 
     if (!mon_config ||
@@ -151,8 +164,8 @@  static void do_client_monitors(struct vdagent_virtio_port *vport, int port_nr,
                     (uint8_t *)mon_config, size);
 
     /* Acknowledge reception of monitors config to spice server / client */
-    reply.type  = VD_AGENT_MONITORS_CONFIG;
-    reply.error = VD_AGENT_SUCCESS;
+    reply.type  = htole32(VD_AGENT_MONITORS_CONFIG);
+    reply.error = htole32(VD_AGENT_SUCCESS);
     vdagent_virtio_port_write(vport, port_nr, VD_AGENT_REPLY, 0,
                               (uint8_t *)&reply, sizeof(reply));
 }
@@ -161,11 +174,15 @@  static void do_client_volume_sync(struct vdagent_virtio_port *vport, int port_nr
     VDAgentMessage *message_header,
     VDAgentAudioVolumeSync *avs)
 {
+    int i;
     if (active_session_conn == NULL) {
         syslog(LOG_DEBUG, "No active session - Can't volume-sync");
         return;
     }
 
+    for (i = 0; i < avs->nchannels; i++)
+        avs->volume[i] = le16toh(avs->volume[i]);
+
     udscs_write(active_session_conn, VDAGENTD_AUDIO_VOLUME_SYNC, 0, 0,
                 (uint8_t *)avs, message_header->size);
 }
@@ -174,7 +191,7 @@  static void do_client_capabilities(struct vdagent_virtio_port *vport,
     VDAgentMessage *message_header,
     VDAgentAnnounceCapabilities *caps)
 {
-    int new_size = VD_AGENT_CAPS_SIZE_FROM_MSG_SIZE(message_header->size);
+    int i, new_size = VD_AGENT_CAPS_SIZE_FROM_MSG_SIZE(message_header->size);
 
     if (capabilities_size != new_size) {
         capabilities_size = new_size;
@@ -186,7 +203,8 @@  static void do_client_capabilities(struct vdagent_virtio_port *vport,
             return;
         }
     }
-    memcpy(capabilities, caps->caps, capabilities_size * sizeof(uint32_t));
+    for (i = 0; i < capabilities_size; i++)
+        capabilities[i] = le32toh(caps->caps[i]);
     if (caps->request) {
         /* Report the previous client has disconneced. */
         do_client_disconnect();
@@ -225,7 +243,7 @@  static void do_client_clipboard(struct vdagent_virtio_port *vport,
     case VD_AGENT_CLIPBOARD_REQUEST: {
         VDAgentClipboardRequest *req = (VDAgentClipboardRequest *)data;
         msg_type = VDAGENTD_CLIPBOARD_REQUEST;
-        data_type = req->type;
+        data_type = le32toh(req->type);
         data = NULL;
         size = 0;
         break;
@@ -233,7 +251,7 @@  static void do_client_clipboard(struct vdagent_virtio_port *vport,
     case VD_AGENT_CLIPBOARD: {
         VDAgentClipboard *clipboard = (VDAgentClipboard *)data;
         msg_type = VDAGENTD_CLIPBOARD_DATA;
-        data_type = clipboard->type;
+        data_type = le32toh(clipboard->type);
         size = size - sizeof(VDAgentClipboard);
         data = clipboard->data;
         break;
@@ -255,8 +273,8 @@  static void send_file_xfer_status(struct vdagent_virtio_port *vport,
                                   const char *msg, uint32_t id, uint32_t xfer_status)
 {
     VDAgentFileXferStatusMessage status = {
-        .id = id,
-        .result = xfer_status,
+        .id = htole32(id),
+        .result = htole32(xfer_status),
     };
     syslog(LOG_WARNING, msg, id);
     if (vport)
@@ -275,6 +293,7 @@  static void do_client_file_xfer(struct vdagent_virtio_port *vport,
     switch (message_header->type) {
     case VD_AGENT_FILE_XFER_START: {
         VDAgentFileXferStartMessage *s = (VDAgentFileXferStartMessage *)data;
+        s->id = le32toh(s->id);
         if (!active_session_conn) {
             send_file_xfer_status(vport,
                "Could not find an agent connnection belonging to the "
@@ -295,12 +314,16 @@  static void do_client_file_xfer(struct vdagent_virtio_port *vport,
     }
     case VD_AGENT_FILE_XFER_STATUS: {
         VDAgentFileXferStatusMessage *s = (VDAgentFileXferStatusMessage *)data;
+        s->id = le32toh(s->id);
+        s->result = le64toh(s->result);
         msg_type = VDAGENTD_FILE_XFER_STATUS;
         id = s->id;
         break;
     }
     case VD_AGENT_FILE_XFER_DATA: {
         VDAgentFileXferDataMessage *d = (VDAgentFileXferDataMessage *)data;
+        d->id = le32toh(d->id);
+        d->size = le64toh(d->size);
         msg_type = VDAGENTD_FILE_XFER_DATA;
         id = d->id;
         break;
@@ -399,15 +422,18 @@  static int virtio_port_read_complete(
         if (message_header->size != sizeof(VDAgentMaxClipboard))
             goto size_error;
         VDAgentMaxClipboard *msg = (VDAgentMaxClipboard *)data;
-        syslog(LOG_DEBUG, "Set max clipboard: %d", msg->max);
-        max_clipboard = msg->max;
+        syslog(LOG_DEBUG, "Set max clipboard: %d", le32toh(msg->max));
+        max_clipboard = le32toh(msg->max);
         break;
     case VD_AGENT_AUDIO_VOLUME_SYNC:
         if (message_header->size < sizeof(VDAgentAudioVolumeSync))
             goto size_error;
+        VDAgentAudioVolumeSync *vdata = (VDAgentAudioVolumeSync *)data;
+        if (message_header->size < sizeof(VDAgentAudioVolumeSync) +
+                vdata->nchannels * sizeof(vdata->volume[0]))
+            goto size_error;
 
-        do_client_volume_sync(vport, port_nr, message_header,
-                (VDAgentAudioVolumeSync *)data);
+        do_client_volume_sync(vport, port_nr, message_header, vdata);
         break;
     default:
         syslog(LOG_WARNING, "unknown message type %d, ignoring",
@@ -444,6 +470,7 @@  static void virtio_write_clipboard(uint8_t selection, uint32_t msg_type,
         vdagent_virtio_port_write_append(virtio_port, sel, 4);
     }
     if (data_type != -1) {
+        data_type = htole32(data_type);
         vdagent_virtio_port_write_append(virtio_port, (uint8_t*)&data_type, 4);
     }
 
@@ -452,10 +479,11 @@  static void virtio_write_clipboard(uint8_t selection, uint32_t msg_type,
 
 /* vdagentd <-> vdagent communication handling */
 static int do_agent_clipboard(struct udscs_connection *conn,
-        struct udscs_message_header *header, const uint8_t *data)
+        struct udscs_message_header *header, uint8_t *data)
 {
     uint8_t selection = header->arg1;
     uint32_t msg_type = 0, data_type = -1, size = header->size;
+    int i;
 
     if (!VD_AGENT_HAS_CAPABILITY(capabilities, capabilities_size,
                                  VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
@@ -483,6 +511,10 @@  static int do_agent_clipboard(struct udscs_connection *conn,
     switch (header->type) {
     case VDAGENTD_CLIPBOARD_GRAB:
         msg_type = VD_AGENT_CLIPBOARD_GRAB;
+        if (size % sizeof(uint32_t))
+                syslog(LOG_ERR, "Clipboard grab imessage size not multiple of %zu", sizeof(uint32_t));
+        for(i = 0; i < size / sizeof(uint32_t); i++)
+                ((uint32_t*)data)[i] = htole32(((uint32_t*)data)[i]);
         agent_owns_clipboard[selection] = 1;
         break;
     case VDAGENTD_CLIPBOARD_REQUEST:
@@ -763,8 +795,8 @@  static void agent_read_complete(struct udscs_connection **connp,
         break;
     case VDAGENTD_FILE_XFER_STATUS:{
         VDAgentFileXferStatusMessage status;
-        status.id = header->arg1;
-        status.result = header->arg2;
+        status.id = htole32(header->arg1);
+        status.result = htole32(header->arg2);
         vdagent_virtio_port_write(virtio_port, VDP_CLIENT_PORT,
                                   VD_AGENT_FILE_XFER_STATUS, 0,
                                   (uint8_t *)&status, sizeof(status));
diff --git a/src/vdagentd/virtio-port.c b/src/vdagentd/virtio-port.c
index cedda4d..ac4d805 100644
--- a/src/vdagentd/virtio-port.c
+++ b/src/vdagentd/virtio-port.c
@@ -216,16 +216,16 @@  int vdagent_virtio_port_write_start(
         return -1;
     }
 
-    chunk_header.port = port_nr;
-    chunk_header.size = sizeof(message_header) + data_size;
+    chunk_header.port = htole32(port_nr);
+    chunk_header.size = htole32(sizeof(message_header) + data_size);
     memcpy(new_wbuf->buf + new_wbuf->write_pos, &chunk_header,
            sizeof(chunk_header));
     new_wbuf->write_pos += sizeof(chunk_header);
 
-    message_header.protocol = VD_AGENT_PROTOCOL;
-    message_header.type = message_type;
-    message_header.opaque = message_opaque;
-    message_header.size = data_size;
+    message_header.protocol = htole32(VD_AGENT_PROTOCOL);
+    message_header.type = htole32(message_type);
+    message_header.opaque = htole64(message_opaque);
+    message_header.size = htole32(data_size);
     memcpy(new_wbuf->buf + new_wbuf->write_pos, &message_header,
            sizeof(message_header));
     new_wbuf->write_pos += sizeof(message_header);
@@ -309,13 +309,20 @@  static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp)
         memcpy((uint8_t *)&port->message_header + port->message_header_read,
                vport->chunk_data, read);
         port->message_header_read += read;
-        if (port->message_header_read == sizeof(port->message_header) &&
-                port->message_header.size) {
-            port->message_data = malloc(port->message_header.size);
-            if (!port->message_data) {
-                syslog(LOG_ERR, "out of memory, disconnecting virtio");
-                vdagent_virtio_port_destroy(vportp);
-                return;
+        if (port->message_header_read == sizeof(port->message_header)) {
+
+            port->message_header.protocol = le32toh(port->message_header.protocol);
+            port->message_header.type = le32toh(port->message_header.type);
+            port->message_header.opaque = le64toh(port->message_header.opaque);
+            port->message_header.size = le32toh(port->message_header.size);
+
+            if (port->message_header.size) {
+                port->message_data = malloc(port->message_header.size);
+                if (!port->message_data) {
+                    syslog(LOG_ERR, "out of memory, disconnecting virtio");
+                    vdagent_virtio_port_destroy(vportp);
+                    return;
+                }
             }
         }
         pos = read;
@@ -420,6 +427,8 @@  static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
     if (vport->chunk_header_read < sizeof(vport->chunk_header)) {
         vport->chunk_header_read += n;
         if (vport->chunk_header_read == sizeof(vport->chunk_header)) {
+            vport->chunk_header.size = le32toh(vport->chunk_header.size);
+            vport->chunk_header.port = le32toh(vport->chunk_header.port);
             if (vport->chunk_header.size > VD_AGENT_MAX_DATA_SIZE) {
                 syslog(LOG_ERR, "chunk size %u too large",
                        vport->chunk_header.size);

Comments

Hello,
On Mon, 28 Nov 2016 15:08:34 +0100
Michal Suchanek <msuchanek@suse.de> wrote:

> This allows running big endian and little endian guest side by side
> using cut&paste between them.
> 
> There is some general design idea that swapping should come as cloce
> to virtio_read/virtio_write as possible. In particular, the protocol
> between vdagent and vdagentd is guest-specific and in native endian.
> With muliple layers of headers this is a bit tricky. A few message
> types have to be swapped fully before passing through vdagentd.

Is there any problem with this patch?

I was able to test

 - mouse events
 - cut & paste
 - audio volume sync

unused

 - file transfer

unsupported

 - monitor size sync (qemu ppc64 does not support qxl graphics)

Due to switch from big endian to little endian as default on ppc64 the
ability to use the agent is very useful for GUI testing of older
distribution releases.

Thanks

Michal
Hi,

On Mon, Nov 28, 2016 at 03:08:34PM +0100, Michal Suchanek wrote:
> This allows running big endian and little endian guest side by side using
> cut&paste between them.
>
> There is some general design idea that swapping should come as cloce to
> virtio_read/virtio_write as possible. In particular, the protocol between
> vdagent and vdagentd is guest-specific and in native endian. With muliple
> layers of headers this is a bit tricky. A few message types have to be swapped
> fully before passing through vdagentd.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>  src/vdagentd/uinput.c      |  4 +++
>  src/vdagentd/vdagentd.c    | 68 ++++++++++++++++++++++++++++++++++------------
>  src/vdagentd/virtio-port.c | 35 +++++++++++++++---------
>  3 files changed, 76 insertions(+), 31 deletions(-)
>
> diff --git a/src/vdagentd/uinput.c b/src/vdagentd/uinput.c
> index e2966c4..21292cb 100644
> --- a/src/vdagentd/uinput.c
> +++ b/src/vdagentd/uinput.c
> @@ -200,6 +200,10 @@ void vdagentd_uinput_do_mouse(struct vdagentd_uinput **uinputp,
>      };
>      int i, down;
>
> +    mouse->x = le32toh(mouse->x);
> +    mouse->y = le32toh(mouse->y);
> +    mouse->buttons = le32toh(mouse->buttons);
> +
>      if (*uinputp) {
>          if (mouse->display_id >= uinput->screen_count) {
>              syslog(LOG_WARNING, "mouse event for unknown monitor (%d >= %d)",
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index a1faf23..f91434d 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -83,7 +83,7 @@ static void send_capabilities(struct vdagent_virtio_port *vport,
>      uint32_t request)
>  {
>      VDAgentAnnounceCapabilities *caps;
> -    uint32_t size;
> +    uint32_t size, i;
>
>      size = sizeof(*caps) + VD_AGENT_CAPS_BYTES;
>      caps = calloc(1, size);
> @@ -92,7 +92,7 @@ static void send_capabilities(struct vdagent_virtio_port *vport,
>          return;
>      }
>
> -    caps->request = request;
> +    caps->request = htole32(request);
>      VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MOUSE_STATE);
>      VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MONITORS_CONFIG);
>      VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_REPLY);
> @@ -102,6 +102,8 @@ static void send_capabilities(struct vdagent_virtio_port *vport,
>      VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_GUEST_LINEEND_LF);
>      VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MAX_CLIPBOARD);
>      VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_AUDIO_VOLUME_SYNC);
> +    for (i = 0; i < VD_AGENT_CAPS_SIZE; i++)
> +        caps->caps[i] = le32toh(caps->caps[i]);

hmm, I got confused here... I guess that the macros should take in
consideration the guest/client endianness as well..

>
>      vdagent_virtio_port_write(vport, VDP_CLIENT_PORT,
>                                VD_AGENT_ANNOUNCE_CAPABILITIES, 0,
> @@ -122,7 +124,10 @@ static void do_client_monitors(struct vdagent_virtio_port *vport, int port_nr,
>      VDAgentMessage *message_header, VDAgentMonitorsConfig *new_monitors)
>  {
>      VDAgentReply reply;
> -    uint32_t size;
> +    uint32_t size, i;
> +
> +    new_monitors->num_of_monitors = le32toh(new_monitors->num_of_monitors);
> +    new_monitors->flags = le32toh(new_monitors->flags);

Instead handling the swapping in every message handler, i think it might
be nicer to do a helper function to be called at
virtio_port_read_complete()

>
>      /* Store monitor config to send to agents when they connect */
>      size = sizeof(VDAgentMonitorsConfig) +
> @@ -132,6 +137,14 @@ static void do_client_monitors(struct vdagent_virtio_port *vport, int port_nr,
>          return;
>      }
>
> +    for (i = 0; i < new_monitors->num_of_monitors; i++) {
> +        new_monitors->monitors[i].height = le32toh(new_monitors->monitors[i].height);
> +        new_monitors->monitors[i].width = le32toh(new_monitors->monitors[i].width);
> +        new_monitors->monitors[i].depth = le32toh(new_monitors->monitors[i].depth);
> +        new_monitors->monitors[i].x = le32toh(new_monitors->monitors[i].x);
> +        new_monitors->monitors[i].y = le32toh(new_monitors->monitors[i].y);
> +    }
> +
>      vdagentd_write_xorg_conf(new_monitors);
>  
>      if (!mon_config ||
> @@ -151,8 +164,8 @@ static void do_client_monitors(struct vdagent_virtio_port *vport, int port_nr,
>                      (uint8_t *)mon_config, size);
>  
>      /* Acknowledge reception of monitors config to spice server / client */
> -    reply.type  = VD_AGENT_MONITORS_CONFIG;
> -    reply.error = VD_AGENT_SUCCESS;
> +    reply.type  = htole32(VD_AGENT_MONITORS_CONFIG);
> +    reply.error = htole32(VD_AGENT_SUCCESS);
>      vdagent_virtio_port_write(vport, port_nr, VD_AGENT_REPLY, 0,
>                                (uint8_t *)&reply, sizeof(reply));
>  }
> @@ -161,11 +174,15 @@ static void do_client_volume_sync(struct vdagent_virtio_port *vport, int port_nr
>      VDAgentMessage *message_header,
>      VDAgentAudioVolumeSync *avs)
>  {
> +    int i;
>      if (active_session_conn == NULL) {
>          syslog(LOG_DEBUG, "No active session - Can't volume-sync");
>          return;
>      }
>  
> +    for (i = 0; i < avs->nchannels; i++)
> +        avs->volume[i] = le16toh(avs->volume[i]);
> +
>      udscs_write(active_session_conn, VDAGENTD_AUDIO_VOLUME_SYNC, 0, 0,
>                  (uint8_t *)avs, message_header->size);
>  }
> @@ -174,7 +191,7 @@ static void do_client_capabilities(struct vdagent_virtio_port *vport,
>      VDAgentMessage *message_header,
>      VDAgentAnnounceCapabilities *caps)
>  {
> -    int new_size = VD_AGENT_CAPS_SIZE_FROM_MSG_SIZE(message_header->size);
> +    int i, new_size = VD_AGENT_CAPS_SIZE_FROM_MSG_SIZE(message_header->size);
>  
>      if (capabilities_size != new_size) {
>          capabilities_size = new_size;
> @@ -186,7 +203,8 @@ static void do_client_capabilities(struct vdagent_virtio_port *vport,
>              return;
>          }
>      }
> -    memcpy(capabilities, caps->caps, capabilities_size * sizeof(uint32_t));
> +    for (i = 0; i < capabilities_size; i++)
> +        capabilities[i] = le32toh(caps->caps[i]);
>      if (caps->request) {
>          /* Report the previous client has disconneced. */
>          do_client_disconnect();
> @@ -225,7 +243,7 @@ static void do_client_clipboard(struct vdagent_virtio_port *vport,
>      case VD_AGENT_CLIPBOARD_REQUEST: {
>          VDAgentClipboardRequest *req = (VDAgentClipboardRequest *)data;
>          msg_type = VDAGENTD_CLIPBOARD_REQUEST;
> -        data_type = req->type;
> +        data_type = le32toh(req->type);
>          data = NULL;
>          size = 0;
>          break;
> @@ -233,7 +251,7 @@ static void do_client_clipboard(struct vdagent_virtio_port *vport,
>      case VD_AGENT_CLIPBOARD: {
>          VDAgentClipboard *clipboard = (VDAgentClipboard *)data;
>          msg_type = VDAGENTD_CLIPBOARD_DATA;
> -        data_type = clipboard->type;
> +        data_type = le32toh(clipboard->type);
>          size = size - sizeof(VDAgentClipboard);
>          data = clipboard->data;
>          break;
> @@ -255,8 +273,8 @@ static void send_file_xfer_status(struct vdagent_virtio_port *vport,
>                                    const char *msg, uint32_t id, uint32_t xfer_status)
>  {
>      VDAgentFileXferStatusMessage status = {
> -        .id = id,
> -        .result = xfer_status,
> +        .id = htole32(id),
> +        .result = htole32(xfer_status),
>      };
>      syslog(LOG_WARNING, msg, id);
>      if (vport)
> @@ -275,6 +293,7 @@ static void do_client_file_xfer(struct vdagent_virtio_port *vport,
>      switch (message_header->type) {
>      case VD_AGENT_FILE_XFER_START: {
>          VDAgentFileXferStartMessage *s = (VDAgentFileXferStartMessage *)data;
> +        s->id = le32toh(s->id);
>          if (!active_session_conn) {
>              send_file_xfer_status(vport,
>                 "Could not find an agent connnection belonging to the "
> @@ -295,12 +314,16 @@ static void do_client_file_xfer(struct vdagent_virtio_port *vport,
>      }
>      case VD_AGENT_FILE_XFER_STATUS: {
>          VDAgentFileXferStatusMessage *s = (VDAgentFileXferStatusMessage *)data;
> +        s->id = le32toh(s->id);
> +        s->result = le64toh(s->result);
>          msg_type = VDAGENTD_FILE_XFER_STATUS;
>          id = s->id;
>          break;
>      }
>      case VD_AGENT_FILE_XFER_DATA: {
>          VDAgentFileXferDataMessage *d = (VDAgentFileXferDataMessage *)data;
> +        d->id = le32toh(d->id);
> +        d->size = le64toh(d->size);
>          msg_type = VDAGENTD_FILE_XFER_DATA;
>          id = d->id;
>          break;
> @@ -399,15 +422,18 @@ static int virtio_port_read_complete(
>          if (message_header->size != sizeof(VDAgentMaxClipboard))
>              goto size_error;
>          VDAgentMaxClipboard *msg = (VDAgentMaxClipboard *)data;

VDAgentMaxClipboard *msg = payload_to_vdagent_msg (data, VD_AGENT_MAX_CLIPBOARD);
and handle the byte swapping there for each message.

I'll try to test it tomorrow.
Sorry for taking some time to reply back and many thanks for the patches
:)

  toso

> -        syslog(LOG_DEBUG, "Set max clipboard: %d", msg->max);
> -        max_clipboard = msg->max;
> +        syslog(LOG_DEBUG, "Set max clipboard: %d", le32toh(msg->max));
> +        max_clipboard = le32toh(msg->max);
>          break;
>      case VD_AGENT_AUDIO_VOLUME_SYNC:
>          if (message_header->size < sizeof(VDAgentAudioVolumeSync))
>              goto size_error;
> +        VDAgentAudioVolumeSync *vdata = (VDAgentAudioVolumeSync *)data;
> +        if (message_header->size < sizeof(VDAgentAudioVolumeSync) +
> +                vdata->nchannels * sizeof(vdata->volume[0]))
> +            goto size_error;
>
> -        do_client_volume_sync(vport, port_nr, message_header,
> -                (VDAgentAudioVolumeSync *)data);
> +        do_client_volume_sync(vport, port_nr, message_header, vdata);
>          break;
>      default:
>          syslog(LOG_WARNING, "unknown message type %d, ignoring",
> @@ -444,6 +470,7 @@ static void virtio_write_clipboard(uint8_t selection, uint32_t msg_type,
>          vdagent_virtio_port_write_append(virtio_port, sel, 4);
>      }
>      if (data_type != -1) {
> +        data_type = htole32(data_type);
>          vdagent_virtio_port_write_append(virtio_port, (uint8_t*)&data_type, 4);
>      }
>  
> @@ -452,10 +479,11 @@ static void virtio_write_clipboard(uint8_t selection, uint32_t msg_type,
>  
>  /* vdagentd <-> vdagent communication handling */
>  static int do_agent_clipboard(struct udscs_connection *conn,
> -        struct udscs_message_header *header, const uint8_t *data)
> +        struct udscs_message_header *header, uint8_t *data)
>  {
>      uint8_t selection = header->arg1;
>      uint32_t msg_type = 0, data_type = -1, size = header->size;
> +    int i;
>  
>      if (!VD_AGENT_HAS_CAPABILITY(capabilities, capabilities_size,
>                                   VD_AGENT_CAP_CLIPBOARD_BY_DEMAND))
> @@ -483,6 +511,10 @@ static int do_agent_clipboard(struct udscs_connection *conn,
>      switch (header->type) {
>      case VDAGENTD_CLIPBOARD_GRAB:
>          msg_type = VD_AGENT_CLIPBOARD_GRAB;
> +        if (size % sizeof(uint32_t))
> +                syslog(LOG_ERR, "Clipboard grab imessage size not multiple of %zu", sizeof(uint32_t));
> +        for(i = 0; i < size / sizeof(uint32_t); i++)
> +                ((uint32_t*)data)[i] = htole32(((uint32_t*)data)[i]);
>          agent_owns_clipboard[selection] = 1;
>          break;
>      case VDAGENTD_CLIPBOARD_REQUEST:
> @@ -763,8 +795,8 @@ static void agent_read_complete(struct udscs_connection **connp,
>          break;
>      case VDAGENTD_FILE_XFER_STATUS:{
>          VDAgentFileXferStatusMessage status;
> -        status.id = header->arg1;
> -        status.result = header->arg2;
> +        status.id = htole32(header->arg1);
> +        status.result = htole32(header->arg2);
>          vdagent_virtio_port_write(virtio_port, VDP_CLIENT_PORT,
>                                    VD_AGENT_FILE_XFER_STATUS, 0,
>                                    (uint8_t *)&status, sizeof(status));
> diff --git a/src/vdagentd/virtio-port.c b/src/vdagentd/virtio-port.c
> index cedda4d..ac4d805 100644
> --- a/src/vdagentd/virtio-port.c
> +++ b/src/vdagentd/virtio-port.c
> @@ -216,16 +216,16 @@ int vdagent_virtio_port_write_start(
>          return -1;
>      }
>  
> -    chunk_header.port = port_nr;
> -    chunk_header.size = sizeof(message_header) + data_size;
> +    chunk_header.port = htole32(port_nr);
> +    chunk_header.size = htole32(sizeof(message_header) + data_size);
>      memcpy(new_wbuf->buf + new_wbuf->write_pos, &chunk_header,
>             sizeof(chunk_header));
>      new_wbuf->write_pos += sizeof(chunk_header);
>  
> -    message_header.protocol = VD_AGENT_PROTOCOL;
> -    message_header.type = message_type;
> -    message_header.opaque = message_opaque;
> -    message_header.size = data_size;
> +    message_header.protocol = htole32(VD_AGENT_PROTOCOL);
> +    message_header.type = htole32(message_type);
> +    message_header.opaque = htole64(message_opaque);
> +    message_header.size = htole32(data_size);
>      memcpy(new_wbuf->buf + new_wbuf->write_pos, &message_header,
>             sizeof(message_header));
>      new_wbuf->write_pos += sizeof(message_header);
> @@ -309,13 +309,20 @@ static void vdagent_virtio_port_do_chunk(struct vdagent_virtio_port **vportp)
>          memcpy((uint8_t *)&port->message_header + port->message_header_read,
>                 vport->chunk_data, read);
>          port->message_header_read += read;
> -        if (port->message_header_read == sizeof(port->message_header) &&
> -                port->message_header.size) {
> -            port->message_data = malloc(port->message_header.size);
> -            if (!port->message_data) {
> -                syslog(LOG_ERR, "out of memory, disconnecting virtio");
> -                vdagent_virtio_port_destroy(vportp);
> -                return;
> +        if (port->message_header_read == sizeof(port->message_header)) {
> +
> +            port->message_header.protocol = le32toh(port->message_header.protocol);
> +            port->message_header.type = le32toh(port->message_header.type);
> +            port->message_header.opaque = le64toh(port->message_header.opaque);
> +            port->message_header.size = le32toh(port->message_header.size);
> +
> +            if (port->message_header.size) {
> +                port->message_data = malloc(port->message_header.size);
> +                if (!port->message_data) {
> +                    syslog(LOG_ERR, "out of memory, disconnecting virtio");
> +                    vdagent_virtio_port_destroy(vportp);
> +                    return;
> +                }
>              }
>          }
>          pos = read;
> @@ -420,6 +427,8 @@ static void vdagent_virtio_port_do_read(struct vdagent_virtio_port **vportp)
>      if (vport->chunk_header_read < sizeof(vport->chunk_header)) {
>          vport->chunk_header_read += n;
>          if (vport->chunk_header_read == sizeof(vport->chunk_header)) {
> +            vport->chunk_header.size = le32toh(vport->chunk_header.size);
> +            vport->chunk_header.port = le32toh(vport->chunk_header.port);
>              if (vport->chunk_header.size > VD_AGENT_MAX_DATA_SIZE) {
>                  syslog(LOG_ERR, "chunk size %u too large",
>                         vport->chunk_header.size);
> -- 
> 2.10.2
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
Hey,

On Mon, Nov 28, 2016 at 03:08:34PM +0100, Michal Suchanek wrote:
> This allows running big endian and little endian guest side by side using
> cut&paste between them.
> 
> There is some general design idea that swapping should come as cloce to
> virtio_read/virtio_write as possible. In particular, the protocol between
> vdagent and vdagentd is guest-specific and in native endian. With muliple
> layers of headers this is a bit tricky. A few message types have to be swapped
> fully before passing through vdagentd.
> 
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>  src/vdagentd/uinput.c      |  4 +++
>  src/vdagentd/vdagentd.c    | 68 ++++++++++++++++++++++++++++++++++------------
>  src/vdagentd/virtio-port.c | 35 +++++++++++++++---------
>  3 files changed, 76 insertions(+), 31 deletions(-)
> 
> diff --git a/src/vdagentd/uinput.c b/src/vdagentd/uinput.c
> index e2966c4..21292cb 100644
> --- a/src/vdagentd/uinput.c
> +++ b/src/vdagentd/uinput.c
> @@ -200,6 +200,10 @@ void vdagentd_uinput_do_mouse(struct vdagentd_uinput **uinputp,
>      };
>      int i, down;
>  
> +    mouse->x = le32toh(mouse->x);
> +    mouse->y = le32toh(mouse->y);
> +    mouse->buttons = le32toh(mouse->buttons);
> +
>      if (*uinputp) {
>          if (mouse->display_id >= uinput->screen_count) {

No swapping of mouse->display_id?

> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index a1faf23..f91434d 100644
> --- a/src/vdagentd/vdagentd.c
> +++ b/src/vdagentd/vdagentd.c
> @@ -399,15 +422,18 @@ static int virtio_port_read_complete(
>          if (message_header->size != sizeof(VDAgentMaxClipboard))
>              goto size_error;
>          VDAgentMaxClipboard *msg = (VDAgentMaxClipboard *)data;
> -        syslog(LOG_DEBUG, "Set max clipboard: %d", msg->max);
> -        max_clipboard = msg->max;
> +        syslog(LOG_DEBUG, "Set max clipboard: %d", le32toh(msg->max));
> +        max_clipboard = le32toh(msg->max);
>          break;
>      case VD_AGENT_AUDIO_VOLUME_SYNC:
>          if (message_header->size < sizeof(VDAgentAudioVolumeSync))
>              goto size_error;
> +        VDAgentAudioVolumeSync *vdata = (VDAgentAudioVolumeSync *)data;
> +        if (message_header->size < sizeof(VDAgentAudioVolumeSync) +
> +                vdata->nchannels * sizeof(vdata->volume[0]))
> +            goto size_error;

This last change seems unrelated, and would be missing a vdata->nchannels
byteswap if I'm not mistaken.

Christophe
On Wed, 14 Dec 2016 15:32:11 +0100
Christophe Fergeau <cfergeau@redhat.com> wrote:

> Hey,
> 
> On Mon, Nov 28, 2016 at 03:08:34PM +0100, Michal Suchanek wrote:
> > This allows running big endian and little endian guest side by side
> > using cut&paste between them.
> > 
> > There is some general design idea that swapping should come as
> > cloce to virtio_read/virtio_write as possible. In particular, the
> > protocol between vdagent and vdagentd is guest-specific and in
> > native endian. With muliple layers of headers this is a bit tricky.
> > A few message types have to be swapped fully before passing through
> > vdagentd.
> > 
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> >  src/vdagentd/uinput.c      |  4 +++
> >  src/vdagentd/vdagentd.c    | 68
> > ++++++++++++++++++++++++++++++++++------------
> > src/vdagentd/virtio-port.c | 35 +++++++++++++++--------- 3 files
> > changed, 76 insertions(+), 31 deletions(-)
> > 
> > diff --git a/src/vdagentd/uinput.c b/src/vdagentd/uinput.c
> > index e2966c4..21292cb 100644
> > --- a/src/vdagentd/uinput.c
> > +++ b/src/vdagentd/uinput.c
> > @@ -200,6 +200,10 @@ void vdagentd_uinput_do_mouse(struct
> > vdagentd_uinput **uinputp, };
> >      int i, down;
> >  
> > +    mouse->x = le32toh(mouse->x);
> > +    mouse->y = le32toh(mouse->y);
> > +    mouse->buttons = le32toh(mouse->buttons);
> > +
> >      if (*uinputp) {
> >          if (mouse->display_id >= uinput->screen_count) {  
> 
> No swapping of mouse->display_id?

/usr/include/spice-1/spice/vd_agent.h:    uint8_t display_id;

> 
> > diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> > index a1faf23..f91434d 100644
> > --- a/src/vdagentd/vdagentd.c
> > +++ b/src/vdagentd/vdagentd.c
> > @@ -399,15 +422,18 @@ static int virtio_port_read_complete(
> >          if (message_header->size != sizeof(VDAgentMaxClipboard))
> >              goto size_error;
> >          VDAgentMaxClipboard *msg = (VDAgentMaxClipboard *)data;
> > -        syslog(LOG_DEBUG, "Set max clipboard: %d", msg->max);
> > -        max_clipboard = msg->max;
> > +        syslog(LOG_DEBUG, "Set max clipboard: %d",
> > le32toh(msg->max));
> > +        max_clipboard = le32toh(msg->max);
> >          break;
> >      case VD_AGENT_AUDIO_VOLUME_SYNC:
> >          if (message_header->size < sizeof(VDAgentAudioVolumeSync))
> >              goto size_error;
> > +        VDAgentAudioVolumeSync *vdata = (VDAgentAudioVolumeSync
> > *)data;
> > +        if (message_header->size < sizeof(VDAgentAudioVolumeSync) +
> > +                vdata->nchannels * sizeof(vdata->volume[0]))
> > +            goto size_error;  
> 
> This last change seems unrelated, 

vdagentd now touches the data to swap it so it must ensure the data is
present.

> and would be missing a
> vdata->nchannels byteswap if I'm not mistaken.

/usr/include/spice-1/spice/vd_agent.h:    uint8_t nchannels;

> 
> Christophe


On Tue, 13 Dec 2016 18:33:54 +0100
Victor Toso <victortoso@redhat.com> wrote:

> Hi,
> 
> On Mon, Nov 28, 2016 at 03:08:34PM +0100, Michal Suchanek wrote:
> > This allows running big endian and little endian guest side by side
> > using cut&paste between them.
> >
> > There is some general design idea that swapping should come as
> > cloce to virtio_read/virtio_write as possible. In particular, the
> > protocol between vdagent and vdagentd is guest-specific and in
> > native endian. With muliple layers of headers this is a bit tricky.
> > A few message types have to be swapped fully before passing through
> > vdagentd.
> >
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> >  src/vdagentd/uinput.c      |  4 +++
> >  src/vdagentd/vdagentd.c    | 68
> > ++++++++++++++++++++++++++++++++++------------
> > src/vdagentd/virtio-port.c | 35 +++++++++++++++--------- 3 files
> > changed, 76 insertions(+), 31 deletions(-)
> >
> > diff --git a/src/vdagentd/uinput.c b/src/vdagentd/uinput.c
> > index e2966c4..21292cb 100644
> > --- a/src/vdagentd/uinput.c
> > +++ b/src/vdagentd/uinput.c
> > @@ -200,6 +200,10 @@ void vdagentd_uinput_do_mouse(struct
> > vdagentd_uinput **uinputp, };
> >      int i, down;
> >
> > +    mouse->x = le32toh(mouse->x);
> > +    mouse->y = le32toh(mouse->y);
> > +    mouse->buttons = le32toh(mouse->buttons);
> > +
> >      if (*uinputp) {
> >          if (mouse->display_id >= uinput->screen_count) {
> >              syslog(LOG_WARNING, "mouse event for unknown monitor
> > (%d >= %d)", diff --git a/src/vdagentd/vdagentd.c
> > b/src/vdagentd/vdagentd.c index a1faf23..f91434d 100644
> > --- a/src/vdagentd/vdagentd.c
> > +++ b/src/vdagentd/vdagentd.c
> > @@ -83,7 +83,7 @@ static void send_capabilities(struct
> > vdagent_virtio_port *vport, uint32_t request)
> >  {
> >      VDAgentAnnounceCapabilities *caps;
> > -    uint32_t size;
> > +    uint32_t size, i;
> >
> >      size = sizeof(*caps) + VD_AGENT_CAPS_BYTES;
> >      caps = calloc(1, size);
> > @@ -92,7 +92,7 @@ static void send_capabilities(struct
> > vdagent_virtio_port *vport, return;
> >      }
> >
> > -    caps->request = request;
> > +    caps->request = htole32(request);
> >      VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MOUSE_STATE);
> >      VD_AGENT_SET_CAPABILITY(caps->caps,
> > VD_AGENT_CAP_MONITORS_CONFIG); VD_AGENT_SET_CAPABILITY(caps->caps,
> > VD_AGENT_CAP_REPLY); @@ -102,6 +102,8 @@ static void
> > send_capabilities(struct vdagent_virtio_port *vport,
> > VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_GUEST_LINEEND_LF);
> > VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MAX_CLIPBOARD);
> > VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_AUDIO_VOLUME_SYNC);
> > +    for (i = 0; i < VD_AGENT_CAPS_SIZE; i++)
> > +        caps->caps[i] = le32toh(caps->caps[i]);  
> 
> hmm, I got confused here... I guess that the macros should take in
> consideration the guest/client endianness as well..

That's what the name and description says and it works on both endian
guests.

> 
> >
> >      vdagent_virtio_port_write(vport, VDP_CLIENT_PORT,
> >                                VD_AGENT_ANNOUNCE_CAPABILITIES, 0,
> > @@ -122,7 +124,10 @@ static void do_client_monitors(struct
> > vdagent_virtio_port *vport, int port_nr, VDAgentMessage
> > *message_header, VDAgentMonitorsConfig *new_monitors) {
> >      VDAgentReply reply;
> > -    uint32_t size;
> > +    uint32_t size, i;
> > +
> > +    new_monitors->num_of_monitors =
> > le32toh(new_monitors->num_of_monitors);
> > +    new_monitors->flags = le32toh(new_monitors->flags);  
> 
> Instead handling the swapping in every message handler, i think it
> might be nicer to do a helper function to be called at
> virtio_port_read_complete()

The problem is the message structure differs. Maybe I missed some
pattern common to multiple message types but in general the message may
contain any combination of single byte and multibyte values.

If there are a few message types that are purely an arrray of 32bit
integers it might make sense to make a common function for them, true.

The other one that is swapped completely is audio and that is 16bit so
doing 32bit swap would probably swap the channels.

> 
> >
> >      /* Store monitor config to send to agents when they connect */
> >      size = sizeof(VDAgentMonitorsConfig) +
> > @@ -132,6 +137,14 @@ static void do_client_monitors(struct
> > vdagent_virtio_port *vport, int port_nr, return;
> >      }
> >
> > +    for (i = 0; i < new_monitors->num_of_monitors; i++) {
> > +        new_monitors->monitors[i].height =
> > le32toh(new_monitors->monitors[i].height);
> > +        new_monitors->monitors[i].width =
> > le32toh(new_monitors->monitors[i].width);
> > +        new_monitors->monitors[i].depth =
> > le32toh(new_monitors->monitors[i].depth);
> > +        new_monitors->monitors[i].x =
> > le32toh(new_monitors->monitors[i].x);
> > +        new_monitors->monitors[i].y =
> > le32toh(new_monitors->monitors[i].y);
> > +    }
> > +
> >      vdagentd_write_xorg_conf(new_monitors);
> >  
> >      if (!mon_config ||
> > @@ -151,8 +164,8 @@ static void do_client_monitors(struct
> > vdagent_virtio_port *vport, int port_nr, (uint8_t *)mon_config,
> > size); 
> >      /* Acknowledge reception of monitors config to spice server /
> > client */
> > -    reply.type  = VD_AGENT_MONITORS_CONFIG;
> > -    reply.error = VD_AGENT_SUCCESS;
> > +    reply.type  = htole32(VD_AGENT_MONITORS_CONFIG);
> > +    reply.error = htole32(VD_AGENT_SUCCESS);
> >      vdagent_virtio_port_write(vport, port_nr, VD_AGENT_REPLY, 0,
> >                                (uint8_t *)&reply, sizeof(reply));
> >  }
> > @@ -161,11 +174,15 @@ static void do_client_volume_sync(struct
> > vdagent_virtio_port *vport, int port_nr VDAgentMessage
> > *message_header, VDAgentAudioVolumeSync *avs)
> >  {
> > +    int i;
> >      if (active_session_conn == NULL) {
> >          syslog(LOG_DEBUG, "No active session - Can't volume-sync");
> >          return;
> >      }
> >  
> > +    for (i = 0; i < avs->nchannels; i++)
> > +        avs->volume[i] = le16toh(avs->volume[i]);
> > +
> >      udscs_write(active_session_conn, VDAGENTD_AUDIO_VOLUME_SYNC,
> > 0, 0, (uint8_t *)avs, message_header->size);
> >  }
> > @@ -174,7 +191,7 @@ static void do_client_capabilities(struct
> > vdagent_virtio_port *vport, VDAgentMessage *message_header,
> >      VDAgentAnnounceCapabilities *caps)
> >  {
> > -    int new_size =
> > VD_AGENT_CAPS_SIZE_FROM_MSG_SIZE(message_header->size);
> > +    int i, new_size =
> > VD_AGENT_CAPS_SIZE_FROM_MSG_SIZE(message_header->size); 
> >      if (capabilities_size != new_size) {
> >          capabilities_size = new_size;
> > @@ -186,7 +203,8 @@ static void do_client_capabilities(struct
> > vdagent_virtio_port *vport, return;
> >          }
> >      }
> > -    memcpy(capabilities, caps->caps, capabilities_size *
> > sizeof(uint32_t));
> > +    for (i = 0; i < capabilities_size; i++)
> > +        capabilities[i] = le32toh(caps->caps[i]);
> >      if (caps->request) {
> >          /* Report the previous client has disconneced. */
> >          do_client_disconnect();
> > @@ -225,7 +243,7 @@ static void do_client_clipboard(struct
> > vdagent_virtio_port *vport, case VD_AGENT_CLIPBOARD_REQUEST: {
> >          VDAgentClipboardRequest *req = (VDAgentClipboardRequest
> > *)data; msg_type = VDAGENTD_CLIPBOARD_REQUEST;
> > -        data_type = req->type;
> > +        data_type = le32toh(req->type);
> >          data = NULL;
> >          size = 0;
> >          break;
> > @@ -233,7 +251,7 @@ static void do_client_clipboard(struct
> > vdagent_virtio_port *vport, case VD_AGENT_CLIPBOARD: {
> >          VDAgentClipboard *clipboard = (VDAgentClipboard *)data;
> >          msg_type = VDAGENTD_CLIPBOARD_DATA;
> > -        data_type = clipboard->type;
> > +        data_type = le32toh(clipboard->type);
> >          size = size - sizeof(VDAgentClipboard);
> >          data = clipboard->data;
> >          break;
> > @@ -255,8 +273,8 @@ static void send_file_xfer_status(struct
> > vdagent_virtio_port *vport, const char *msg, uint32_t id, uint32_t
> > xfer_status) {
> >      VDAgentFileXferStatusMessage status = {
> > -        .id = id,
> > -        .result = xfer_status,
> > +        .id = htole32(id),
> > +        .result = htole32(xfer_status),
> >      };
> >      syslog(LOG_WARNING, msg, id);
> >      if (vport)
> > @@ -275,6 +293,7 @@ static void do_client_file_xfer(struct
> > vdagent_virtio_port *vport, switch (message_header->type) {
> >      case VD_AGENT_FILE_XFER_START: {
> >          VDAgentFileXferStartMessage *s =
> > (VDAgentFileXferStartMessage *)data;
> > +        s->id = le32toh(s->id);
> >          if (!active_session_conn) {
> >              send_file_xfer_status(vport,
> >                 "Could not find an agent connnection belonging to
> > the " @@ -295,12 +314,16 @@ static void do_client_file_xfer(struct
> > vdagent_virtio_port *vport, }
> >      case VD_AGENT_FILE_XFER_STATUS: {
> >          VDAgentFileXferStatusMessage *s =
> > (VDAgentFileXferStatusMessage *)data;
> > +        s->id = le32toh(s->id);
> > +        s->result = le64toh(s->result);
> >          msg_type = VDAGENTD_FILE_XFER_STATUS;
> >          id = s->id;
> >          break;
> >      }
> >      case VD_AGENT_FILE_XFER_DATA: {
> >          VDAgentFileXferDataMessage *d =
> > (VDAgentFileXferDataMessage *)data;
> > +        d->id = le32toh(d->id);
> > +        d->size = le64toh(d->size);
> >          msg_type = VDAGENTD_FILE_XFER_DATA;
> >          id = d->id;
> >          break;
> > @@ -399,15 +422,18 @@ static int virtio_port_read_complete(
> >          if (message_header->size != sizeof(VDAgentMaxClipboard))
> >              goto size_error;
> >          VDAgentMaxClipboard *msg = (VDAgentMaxClipboard *)data;  
> 
> VDAgentMaxClipboard *msg = payload_to_vdagent_msg (data,
> VD_AGENT_MAX_CLIPBOARD); and handle the byte swapping there for each
> message.

hm, many of the clipboard messages are handled similarly so there is
probably some room for unification there.

> 
> I'll try to test it tomorrow.
> Sorry for taking some time to reply back and many thanks for the
> patches :)
> 
>   toso
> 

Thanks for the review

Michal
On Wed, Dec 14, 2016 at 04:43:11PM +0100, Michal Suchánek wrote:
> On Wed, 14 Dec 2016 15:32:11 +0100
> Christophe Fergeau <cfergeau@redhat.com> wrote:
> 
> > Hey,
> > 
> > On Mon, Nov 28, 2016 at 03:08:34PM +0100, Michal Suchanek wrote:
> > > This allows running big endian and little endian guest side by side
> > > using cut&paste between them.
> > > 
> > > There is some general design idea that swapping should come as
> > > cloce to virtio_read/virtio_write as possible. In particular, the
> > > protocol between vdagent and vdagentd is guest-specific and in
> > > native endian. With muliple layers of headers this is a bit tricky.
> > > A few message types have to be swapped fully before passing through
> > > vdagentd.
> > > 
> > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > ---
> > >  src/vdagentd/uinput.c      |  4 +++
> > >  src/vdagentd/vdagentd.c    | 68
> > > ++++++++++++++++++++++++++++++++++------------
> > > src/vdagentd/virtio-port.c | 35 +++++++++++++++--------- 3 files
> > > changed, 76 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/src/vdagentd/uinput.c b/src/vdagentd/uinput.c
> > > index e2966c4..21292cb 100644
> > > --- a/src/vdagentd/uinput.c
> > > +++ b/src/vdagentd/uinput.c
> > > @@ -200,6 +200,10 @@ void vdagentd_uinput_do_mouse(struct
> > > vdagentd_uinput **uinputp, };
> > >      int i, down;
> > >  
> > > +    mouse->x = le32toh(mouse->x);
> > > +    mouse->y = le32toh(mouse->y);
> > > +    mouse->buttons = le32toh(mouse->buttons);
> > > +
> > >      if (*uinputp) {
> > >          if (mouse->display_id >= uinput->screen_count) {  
> > 
> > No swapping of mouse->display_id?
> 
> /usr/include/spice-1/spice/vd_agent.h:    uint8_t display_id;

> > and would be missing a
> > vdata->nchannels byteswap if I'm not mistaken.
> 
> /usr/include/spice-1/spice/vd_agent.h:    uint8_t nchannels;
> 

Ah, my bad, sorry for the noise!

Christophe