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

Michal Suchánek msuchanek at suse.de
Wed Dec 14 15:43:11 UTC 2016


On Wed, 14 Dec 2016 15:32:11 +0100
Christophe Fergeau <cfergeau at 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 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) {  
> 
> 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 at 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 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..

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20161214/6be68f82/attachment.sig>


More information about the Spice-devel mailing list