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@xxxxxxx>
Signed-off-by: Victor Toso <me@xxxxxxxxxxxxxx>
---
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.
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel