[Spice-devel] [PATCH v6 5/5] Do endian swapping.
Christophe de Dinechin
dinechin at redhat.com
Mon Jan 23 14:40:57 UTC 2017
On 23/01/2017 14:53, 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 at suse.de>
> Signed-off-by: Victor Toso <me at victortoso.com>
> ---
> 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
> v3:
> - use glib byteswap macros in place of endian.h byteswap macros
> - move variable declaration out of case statement
> - reuse more of existing clipboard code
> v4:
> - also use glib byteswap for 64bit swaps
> - use file xfer message structure for swapping size
> v5:
> - rebase on top of vdagentd: early return on bad message size
> ---
> src/vdagentd/vdagentd.c | 125 +++++++++++++++++++++++++++++++++++----------
> src/vdagentd/virtio-port.c | 36 ++++++++-----
> 2 files changed, 121 insertions(+), 40 deletions(-)
>
> diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> index 7e16cd2..f8de1e7 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)
If we are converting uint32 messages, shouln't this take a uint32_t *msg
as input? See [1] below
> +{
> + uint32_t i, *msg = (uint32_t *)(_msg + offset);
> +
> + /* offset - size % 4 should be 0 */
Should this be an assert instead of a comment?
> + 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);
[1] It looks like having a uint8_t interface even forces us to
additional casts here.
>
> vdagent_virtio_port_write(vport, VDP_CLIENT_PORT,
> VD_AGENT_ANNOUNCE_CAPABILITIES, 0,
> @@ -174,8 +203,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));
> }
> @@ -278,8 +307,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)
> @@ -361,6 +390,50 @@ static gsize vdagent_message_min_size[VD_AGENT_END_MESSAGE] =
> sizeof(VDAgentAudioVolumeSync), /* VD_AGENT_AUDIO_VOLUME_SYNC */
> };
>
> +static void vdagent_message_clipboard_from_le(VDAgentMessage *message_header,
> + uint8_t *data)
> +{
> + gsize min_size = vdagent_message_min_size[message_header->type];
> + uint32_t *data_type = (uint32_t *) data;
> +
> + if (VD_AGENT_HAS_CAPABILITY(capabilities, capabilities_size,
> + VD_AGENT_CAP_CLIPBOARD_SELECTION)) {
> + min_size += 4;
Is that magical 4, or sizeof(something)?
> + data_type++;
> + }
> +
> + 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;
> + default:
> + g_warn_if_reached();
> + }
> +}
> +
> +static void vdagent_message_file_xfer_from_le(VDAgentMessage *message_header,
> + uint8_t *data)
> +{
> + uint32_t *id = (uint32_t *)data;
> + *id = GUINT32_FROM_LE(*id);
> + id++; /* status */
> +
> + switch (message_header->type) {
> + case VD_AGENT_FILE_XFER_DATA: {
> + VDAgentFileXferDataMessage *msg = (VDAgentFileXferDataMessage *)data;
> + msg->size = GUINT64_FROM_LE(msg->size);
> + break;
> + }
> + case VD_AGENT_FILE_XFER_STATUS:
> + *id = GUINT32_FROM_LE(*id); /* status */
> + break;
> + }
> +}
> +
> static gboolean vdagent_message_check_size(VDAgentMessage *message_header)
> {
> uint32_t min_size = 0;
> @@ -429,20 +502,21 @@ static int virtio_port_read_complete(
> VDAgentMessage *message_header,
> uint8_t *data)
> {
> - uint32_t min_size = 0;
> -
> if (!vdagent_message_check_size(message_header))
> return 0;
>
> switch (message_header->type) {
> case VD_AGENT_MOUSE_STATE:
> + virtio_msg_uint32_from_le(data, message_header->size, 0);
> do_client_mouse(&uinput, (VDAgentMouseState *)data);
> break;
> case VD_AGENT_MONITORS_CONFIG:
> + 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:
> + virtio_msg_uint32_from_le(data, message_header->size, 0);
> do_client_capabilities(vport, message_header,
> (VDAgentAnnounceCapabilities *)data);
> break;
> @@ -450,23 +524,13 @@ static int virtio_port_read_complete(
> case VD_AGENT_CLIPBOARD_REQUEST:
> case VD_AGENT_CLIPBOARD:
> case VD_AGENT_CLIPBOARD_RELEASE:
> - switch (message_header->type) {
> - case VD_AGENT_CLIPBOARD_GRAB:
> - min_size = sizeof(VDAgentClipboardGrab); break;
> - case VD_AGENT_CLIPBOARD_REQUEST:
> - min_size = sizeof(VDAgentClipboardRequest); break;
> - case VD_AGENT_CLIPBOARD:
> - min_size = sizeof(VDAgentClipboard); break;
> - }
> - if (VD_AGENT_HAS_CAPABILITY(capabilities, capabilities_size,
> - VD_AGENT_CAP_CLIPBOARD_SELECTION)) {
> - min_size += 4;
> - }
> + vdagent_message_clipboard_from_le(message_header, data);
> 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:
> + vdagent_message_file_xfer_from_le(message_header, data);
> do_client_file_xfer(vport, message_header, data);
> break;
> case VD_AGENT_CLIENT_DISCONNECTED:
> @@ -475,14 +539,18 @@ static int virtio_port_read_complete(
> break;
> case VD_AGENT_MAX_CLIPBOARD: {
> 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);
Why not
+ max_clipboard = GUINT32_FROM_LE(msg->max);
+ syslog(LOG_DEBUG, "Set max clipboard: %d", max_clipboard);
?
> break;
> }
> - case VD_AGENT_AUDIO_VOLUME_SYNC:
> - do_client_volume_sync(vport, port_nr, message_header,
> - (VDAgentAudioVolumeSync *)data);
> + case VD_AGENT_AUDIO_VOLUME_SYNC: {
> + 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, vdata);
> break;
> + }
> default:
> g_warn_if_reached();
> }
> @@ -491,7 +559,7 @@ static int virtio_port_read_complete(
> }
>
> 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;
>
> @@ -512,15 +580,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) {
Signed / unsigned mismatch. -1U? Any non-magic constant for that -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;
> @@ -831,8 +902,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);
I noticed we sometimes initialize fields like this, sometimes with { .id
= .result = }. Is there a rule, or is that just history? If history,
maybe we can pick one for this patch?
> 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..ef3de90 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 = GUINT64_TO_LE(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 = GUINT64_FROM_LE(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) {
That test seems wrong if you do it after byte-swapping.
> syslog(LOG_ERR, "chunk size %u too large",
> vport->chunk_header.size);
Same here, you probably need to save the original size before swapping
for test and print.
More information about the Spice-devel
mailing list