[Spice-devel] [PATCH] Do endian swapping.

Victor Toso victortoso at redhat.com
Tue Dec 13 17:33:54 UTC 2016


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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20161213/c11a1b90/attachment-0001.sig>


More information about the Spice-devel mailing list