Hi, Sorry for taking some time to reply back. On Mon, Jan 09, 2017 at 01:54:30PM +0100, Michal Suchanek wrote: > This allows running big endian and little endian guest side by side > using cut & paste between them. > > There is a general design idea that swapping should come as close 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@xxxxxxx> > > --- > v2: > - introduce helper functions to swap (a portion of) a message wholesale > - pollute fewer places with swapping sometimes at the cost of slightly > more verbose code The helper is necessary but we are not quite there yet. I think the handling endianness for each message need to be more clear/explicit at least when parsing the incoming messages. So, what I suggested was to improve the current code to have a macro/function which cast to the expected struct. We can add then a function to check if we are in a big-endian machine. If we are, we will do the swapping. There is one example in top of my head, which is [0] (from non related project) which checks the numeric limits for different numeric G_TYPE* [0] https://github.com/GNOME/grilo/commit/9971e349da1f8dbe75c64a0f7a91f5cc8e6387f1#diff-c036e816fff2d44015fc86ee8ffd9b81R877 My point here is how to be clear that we deal with different endianness so when we include more messages, it is harder to forget that these messages need to be careful with big-endian machines too. If this needs a bigger refactor then I'm proposing, I'm would love to hear some ideas :) > v3: > - use glib byteswap macros in place of endian.h byteswap macros I agree that glib macros make sense but somehow I like the *h* (host) prefix/suffix in previous functions. I find it more clear :) ~ You don't need to change it, just a comment. Thanks for the patches, toso > - move variable declaration out of case statement > - reuse more of existing clipboard code > --- > src/vdagentd/vdagentd.c | 89 +++++++++++++++++++++++++++++++++++++++------- > src/vdagentd/virtio-port.c | 36 ++++++++++++------- > 2 files changed, 100 insertions(+), 25 deletions(-) > > diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c > index a1faf23..4cac473 100644 > --- a/src/vdagentd/vdagentd.c > +++ b/src/vdagentd/vdagentd.c > @@ -78,6 +78,34 @@ static int client_connected = 0; > static int max_clipboard = -1; > > /* utility functions */ > +static void virtio_msg_uint32_to_le(uint8_t *_msg, uint32_t size, uint32_t offset) > +{ > + uint32_t i, *msg = (uint32_t *)(_msg + offset); > + > + /* offset - size % 4 should be 0 */ > + for (i = 0; i < (size - offset) / 4; i++) > + msg[i] = GUINT32_TO_LE(msg[i]); > +} > + > +static void virtio_msg_uint32_from_le(uint8_t *_msg, uint32_t size, uint32_t offset) > +{ > + uint32_t i, *msg = (uint32_t *)(_msg + offset); > + > + /* offset - size % 4 should be 0 */ > + for (i = 0; i < (size - offset) / 4; i++) > + msg[i] = GUINT32_FROM_LE(msg[i]); > +} > + > +static void virtio_msg_uint16_from_le(uint8_t *_msg, uint32_t size, uint32_t offset) > +{ > + uint32_t i; > + uint16_t *msg = (uint16_t *)(_msg + offset); > + > + /* offset - size % 2 should be 0 */ > + for (i = 0; i < (size - offset) / 2; i++) > + msg[i] = GUINT16_FROM_LE(msg[i]); > +} > + > /* 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) > @@ -323,6 +352,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; > > if (message_header->protocol != VD_AGENT_PROTOCOL) { > syslog(LOG_ERR, "message with wrong protocol version ignoring"); > @@ -333,6 +364,7 @@ static int virtio_port_read_complete( > case VD_AGENT_MOUSE_STATE: > if (message_header->size != sizeof(VDAgentMouseState)) > goto size_error; > + virtio_msg_uint32_from_le(data, message_header->size, 0); > vdagentd_uinput_do_mouse(&uinput, (VDAgentMouseState *)data); > if (!uinput) { > /* Try to re-open the tablet */ > @@ -356,12 +388,14 @@ static int virtio_port_read_complete( > case VD_AGENT_MONITORS_CONFIG: > if (message_header->size < sizeof(VDAgentMonitorsConfig)) > goto size_error; > + virtio_msg_uint32_from_le(data, message_header->size, 0); > do_client_monitors(vport, port_nr, message_header, > (VDAgentMonitorsConfig *)data); > break; > case VD_AGENT_ANNOUNCE_CAPABILITIES: > if (message_header->size < sizeof(VDAgentAnnounceCapabilities)) > goto size_error; > + virtio_msg_uint32_from_le(data, message_header->size, 0); > do_client_capabilities(vport, message_header, > (VDAgentAnnounceCapabilities *)data); > break; > @@ -380,15 +414,41 @@ static int virtio_port_read_complete( > if (VD_AGENT_HAS_CAPABILITY(capabilities, capabilities_size, > VD_AGENT_CAP_CLIPBOARD_SELECTION)) { > min_size += 4; > + data_type++; > } > if (message_header->size < min_size) { > goto size_error; > } > + switch (message_header->type) { > + case VD_AGENT_CLIPBOARD_REQUEST: > + case VD_AGENT_CLIPBOARD: > + *data_type = GUINT32_FROM_LE(*data_type); > + break; > + case VD_AGENT_CLIPBOARD_GRAB: > + virtio_msg_uint32_from_le(data, message_header->size, min_size); > + break; > + } > do_client_clipboard(vport, message_header, data); > break; > case VD_AGENT_FILE_XFER_START: > case VD_AGENT_FILE_XFER_STATUS: > case VD_AGENT_FILE_XFER_DATA: > + if (message_header->size < sizeof(VDAgentFileXferStartMessage)) > + goto size_error; > + *id = GUINT32_FROM_LE(*id); > + id++; /* size/status */ > + switch (message_header->type) { > + case VD_AGENT_FILE_XFER_DATA: > + if (message_header->size < sizeof(VDAgentFileXferDataMessage)) > + goto size_error; > + *((uint64_t *)id) = le64toh(*((uint64_t *)id)); /* size */ > + break; > + case VD_AGENT_FILE_XFER_STATUS: > + if (message_header->size < sizeof(VDAgentFileXferStatusMessage)) > + goto size_error; > + *id = GUINT32_FROM_LE(*id); /* status */ > + break; > + } > do_client_file_xfer(vport, message_header, data); > break; > case VD_AGENT_CLIENT_DISCONNECTED: > @@ -399,15 +459,17 @@ 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", GUINT32_FROM_LE(msg->max)); > + max_clipboard = GUINT32_FROM_LE(msg->max); > break; > case VD_AGENT_AUDIO_VOLUME_SYNC: > if (message_header->size < sizeof(VDAgentAudioVolumeSync)) > goto size_error; > + VDAgentAudioVolumeSync *vdata = (VDAgentAudioVolumeSync *)data; > + virtio_msg_uint16_from_le((uint8_t *)vdata, message_header->size, > + offsetof(VDAgentAudioVolumeSync, volume)); > > - 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", > @@ -423,7 +485,7 @@ size_error: > } > > static void virtio_write_clipboard(uint8_t selection, uint32_t msg_type, > - uint32_t data_type, const uint8_t *data, uint32_t data_size) > + uint32_t data_type, uint8_t *data, uint32_t data_size) > { > uint32_t size = data_size; > > @@ -444,15 +506,18 @@ 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 = GUINT32_TO_LE(data_type); > vdagent_virtio_port_write_append(virtio_port, (uint8_t*)&data_type, 4); > } > > + if (msg_type == VD_AGENT_CLIPBOARD_GRAB) > + virtio_msg_uint32_to_le(data, data_size, 0); > vdagent_virtio_port_write_append(virtio_port, data, data_size); > } > > /* 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; > @@ -763,8 +828,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 = GUINT32_TO_LE(header->arg1); > + status.result = GUINT32_TO_LE(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..25838fe 100644 > --- a/src/vdagentd/virtio-port.c > +++ b/src/vdagentd/virtio-port.c > @@ -28,6 +28,7 @@ > #include <sys/select.h> > #include <sys/socket.h> > #include <sys/un.h> > +#include <glib.h> > > #include "virtio-port.h" > > @@ -216,16 +217,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 = GUINT32_TO_LE(port_nr); > + chunk_header.size = GUINT32_TO_LE(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 = GUINT32_TO_LE(VD_AGENT_PROTOCOL); > + message_header.type = GUINT32_TO_LE(message_type); > + message_header.opaque = htole64(message_opaque); > + message_header.size = GUINT32_TO_LE(data_size); > memcpy(new_wbuf->buf + new_wbuf->write_pos, &message_header, > sizeof(message_header)); > new_wbuf->write_pos += sizeof(message_header); > @@ -309,13 +310,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 = GUINT32_FROM_LE(port->message_header.protocol); > + port->message_header.type = GUINT32_FROM_LE(port->message_header.type); > + port->message_header.opaque = le64toh(port->message_header.opaque); > + port->message_header.size = GUINT32_FROM_LE(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 +428,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 = GUINT32_FROM_LE(vport->chunk_header.size); > + vport->chunk_header.port = GUINT32_FROM_LE(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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel