On Mon, 23 Jan 2017 15:40:57 +0100
Christophe de Dinechin <dinechin@xxxxxxxxxx> wrote:
> 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
Technically we can. The to_le message swap will likely get the native
pointer width, unlike the from_le message swaps.
>
> > +{
> > + 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)?
It's 4 * sizeof(uint8_t) and used in this magical way throughout the
code. Search for VD_AGENT_CAP_CLIPBOARD_SELECTION. It's also part of
the protocol definition so it shall not magically change.
> > + 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);
>
> ?
Because the search and replace did not rewrite the code. It does not
really matter either way since compilers optimize.
>
> > 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?
Here gcc does not complain about the mismatch. That does not mean the
code was not broken (or relying on not well defined behavior) to start
with. The -1 is a magic value set in the caller probably to mean invalid
type/type not applicable.
> > + 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?
It's probably history. Changing initialization style is out of scope of
this patch given it's inconsistent throughout code. Changing the
initialization style will also make it harder to see what the patch is
actually doing.
>
> > 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.
No. The value must be swapped for the test to make sense.
>
> > 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.
ditto
On Mon, 23 Jan 2017 15:27:24 +0100
Christophe de Dinechin <dinechin@xxxxxxxxxx> wrote:
> On 23/01/2017 14:53, Michal Suchanek wrote:
> > Move some mouse-specific code from the start of
> > virtio_port_read_complete to a separate helper
> > as is the case with other message types.
> >
> > Signed-off-by: Michal Suchanek <msuchanek@xxxxxxx>
> > ---
> > src/vdagentd/vdagentd.c | 43
> > ++++++++++++++++++++++++------------------- 1 file changed, 24
> > insertions(+), 19 deletions(-)
> >
> > diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> > index 2b16ca4..785ce50 100644
> > --- a/src/vdagentd/vdagentd.c
> > +++ b/src/vdagentd/vdagentd.c
> > @@ -118,6 +118,29 @@ static void do_client_disconnect(void)
> > }
> > }
> >
> > +void do_client_mouse(struct vdagentd_uinput **uinputp,
> > VDAgentMouseState *mouse) +{
> > + vdagentd_uinput_do_mouse(uinputp, mouse);
> > + if (!*uinputp) {
> > + /* Try to re-open the tablet */
> > + struct agent_data *agent_data =
> > + udscs_get_user_data(active_session_conn);
> > + if (agent_data)
> > + *uinputp = vdagentd_uinput_create(uinput_device,
> > + agent_data->width,
> > + agent_data->height,
> > +
> > agent_data->screen_info,
> > +
> > agent_data->screen_count,
> > + debug > 1,
> > + uinput_fake);
> > + if (!*uinputp) {
> > + syslog(LOG_CRIT, "Fatal uinput error");
> > + retval = 1;
> > + quit = 1;
> Are retval and quit globals?
Obviously. They are not locals in either function and the code builds.
>
> > + }
> > + }
> > +}
> > +
> > static void do_client_monitors(struct vdagent_virtio_port *vport,
> > int port_nr, VDAgentMessage *message_header, VDAgentMonitorsConfig
> > *new_monitors) {
> > @@ -335,25 +358,7 @@ static int virtio_port_read_complete(
> > case VD_AGENT_MOUSE_STATE:
> > if (message_header->size != sizeof(VDAgentMouseState))
> > goto size_error;
> > - vdagentd_uinput_do_mouse(&uinput, (VDAgentMouseState
> > *)data);
> > - if (!uinput) {
> > - /* Try to re-open the tablet */
> > - struct agent_data *agent_data =
> > - udscs_get_user_data(active_session_conn);
> > - if (agent_data)
> > - uinput = vdagentd_uinput_create(uinput_device,
> > - agent_data->width,
> > - agent_data->height,
> > -
> > agent_data->screen_info,
> > -
> > agent_data->screen_count,
> > - debug > 1,
> I know it's not related to the patch, but what does debug > 1 mean
> here? Shouldn't we replace 1 with a non-magic value?
It means that uinput debug is set when debug level is > 1. It's just
random magic value. Feel free to write a followup patch to make this
less magic.
> > - uinput_fake);
> > - if (!uinput) {
> > - syslog(LOG_CRIT, "Fatal uinput error");
> > - retval = 1;
> > - quit = 1;
> > - }
> > - }
> > + do_client_mouse(&uinput, (VDAgentMouseState *)data);
> > break;
> > case VD_AGENT_MONITORS_CONFIG:
> > if (message_header->size <
> > sizeof(VDAgentMonitorsConfig))
>
Thanks
Michal
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel