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

[Michal Suchánek]

Hello,

On Thu, 19 Jan 2017 16:18:27 +0100
Victor Toso <victortoso at redhat.com> wrote:

> Hi,
> 
> On Fri, Jan 13, 2017 at 07:32:12PM +0100, Michal Suchanek wrote:
 
> >  /* vdagentd <-> spice-client communication handling */
> >  static void send_capabilities(struct vdagent_virtio_port *vport,
> >      uint32_t request)
> > @@ -102,6 +130,7 @@ 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);
> > +    virtio_msg_uint32_to_le((uint8_t *)caps, size, 0);
> >
> >      vdagent_virtio_port_write(vport, VDP_CLIENT_PORT,
> >                                VD_AGENT_ANNOUNCE_CAPABILITIES, 0,
> > @@ -151,8 +180,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  = GUINT32_TO_LE(VD_AGENT_MONITORS_CONFIG);
> > +    reply.error = GUINT32_TO_LE(VD_AGENT_SUCCESS);
> >      vdagent_virtio_port_write(vport, port_nr, VD_AGENT_REPLY, 0,
> >                                (uint8_t *)&reply, sizeof(reply));
> >  }
> > @@ -255,8 +284,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 = GUINT32_TO_LE(id),
> > +        .result = GUINT32_TO_LE(xfer_status),
> >      };
> >      syslog(LOG_WARNING, msg, id);
> >      if (vport)
> > @@ -324,6 +353,8 @@ static int virtio_port_read_complete(
> >          uint8_t *data)
> >  {
> >      uint32_t min_size = 0;
> > +    uint32_t *data_type = (uint32_t *)data;
> > +    uint32_t *id = (uint32_t *)data;  
> 
> I would recommend removing this change as *data_type is only used for
> VD_AGENT_CLIPBOARD and *id for VD_AGENT_FILE_XFER_STATUS, both in
> nested switches with the solely propose of byte swap. Let's use a
> helper function for each case.

It was requested that to place the declaration at the start of the
function rather than inside the switch statement. I would personally
prefer placing the declarations inside the switch statement where they
are used. The code already uses a mix of these declaration styles
anyway. It's quire possible to use a byteswap helper for one or two
specific message types but I don't think it will particularly improve
readability of this code. It also performs size checks which are even
more convoluted than the swapping.
I prefer to do the swapping in one place for all message types.

On the other hand splitting off the uinput handling done specifically
for mouse into a helper so the mouse handling is done similarily to the
other messages improves readability and consistency at the same time. I
sent this change in a separate patch.

Thanks

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/20170120/23ca8f4a/attachment.sig>