Hello,
On Thu, 19 Jan 2017 16:18:27 +0100
Victor Toso <victortoso@xxxxxxxxxx> 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
Attachment:
pgpqef1kJ8L90.pgp
Description: OpenPGP digital signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel