On Wed, 14 Dec 2016 15:32:11 +0100
Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote:
> Hey,
>
> On Mon, Nov 28, 2016 at 03:08:34PM +0100, Michal Suchanek wrote:
> > This allows running big endian and little endian guest side by side
> > using cut&paste between them.
> >
> > There is some general design idea that swapping should come as
> > cloce 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>
> > ---
> > src/vdagentd/uinput.c | 4 +++
> > src/vdagentd/vdagentd.c | 68
> > ++++++++++++++++++++++++++++++++++------------
> > src/vdagentd/virtio-port.c | 35 +++++++++++++++--------- 3 files
> > changed, 76 insertions(+), 31 deletions(-)
> >
> > diff --git a/src/vdagentd/uinput.c b/src/vdagentd/uinput.c
> > index e2966c4..21292cb 100644
> > --- a/src/vdagentd/uinput.c
> > +++ b/src/vdagentd/uinput.c
> > @@ -200,6 +200,10 @@ void vdagentd_uinput_do_mouse(struct
> > vdagentd_uinput **uinputp, };
> > int i, down;
> >
> > + mouse->x = le32toh(mouse->x);
> > + mouse->y = le32toh(mouse->y);
> > + mouse->buttons = le32toh(mouse->buttons);
> > +
> > if (*uinputp) {
> > if (mouse->display_id >= uinput->screen_count) {
>
> No swapping of mouse->display_id?
/usr/include/spice-1/spice/vd_agent.h: uint8_t display_id;
>
> > diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c
> > index a1faf23..f91434d 100644
> > --- a/src/vdagentd/vdagentd.c
> > +++ b/src/vdagentd/vdagentd.c
> > @@ -399,15 +422,18 @@ 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",
> > le32toh(msg->max));
> > + max_clipboard = le32toh(msg->max);
> > break;
> > case VD_AGENT_AUDIO_VOLUME_SYNC:
> > if (message_header->size < sizeof(VDAgentAudioVolumeSync))
> > goto size_error;
> > + VDAgentAudioVolumeSync *vdata = (VDAgentAudioVolumeSync
> > *)data;
> > + if (message_header->size < sizeof(VDAgentAudioVolumeSync) +
> > + vdata->nchannels * sizeof(vdata->volume[0]))
> > + goto size_error;
>
> This last change seems unrelated,
vdagentd now touches the data to swap it so it must ensure the data is
present.
> and would be missing a
> vdata->nchannels byteswap if I'm not mistaken.
/usr/include/spice-1/spice/vd_agent.h: uint8_t nchannels;
>
> Christophe
On Tue, 13 Dec 2016 18:33:54 +0100
Victor Toso <victortoso@xxxxxxxxxx> wrote:
> Hi,
>
> On Mon, Nov 28, 2016 at 03:08:34PM +0100, Michal Suchanek wrote:
> > This allows running big endian and little endian guest side by side
> > using cut&paste between them.
> >
> > There is some general design idea that swapping should come as
> > cloce 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>
> > ---
> > src/vdagentd/uinput.c | 4 +++
> > src/vdagentd/vdagentd.c | 68
> > ++++++++++++++++++++++++++++++++++------------
> > src/vdagentd/virtio-port.c | 35 +++++++++++++++--------- 3 files
> > changed, 76 insertions(+), 31 deletions(-)
> >
> > diff --git a/src/vdagentd/uinput.c b/src/vdagentd/uinput.c
> > index e2966c4..21292cb 100644
> > --- a/src/vdagentd/uinput.c
> > +++ b/src/vdagentd/uinput.c
> > @@ -200,6 +200,10 @@ void vdagentd_uinput_do_mouse(struct
> > vdagentd_uinput **uinputp, };
> > int i, down;
> >
> > + mouse->x = le32toh(mouse->x);
> > + mouse->y = le32toh(mouse->y);
> > + mouse->buttons = le32toh(mouse->buttons);
> > +
> > if (*uinputp) {
> > if (mouse->display_id >= uinput->screen_count) {
> > syslog(LOG_WARNING, "mouse event for unknown monitor
> > (%d >= %d)", diff --git a/src/vdagentd/vdagentd.c
> > b/src/vdagentd/vdagentd.c index a1faf23..f91434d 100644
> > --- a/src/vdagentd/vdagentd.c
> > +++ b/src/vdagentd/vdagentd.c
> > @@ -83,7 +83,7 @@ static void send_capabilities(struct
> > vdagent_virtio_port *vport, uint32_t request)
> > {
> > VDAgentAnnounceCapabilities *caps;
> > - uint32_t size;
> > + uint32_t size, i;
> >
> > size = sizeof(*caps) + VD_AGENT_CAPS_BYTES;
> > caps = calloc(1, size);
> > @@ -92,7 +92,7 @@ static void send_capabilities(struct
> > vdagent_virtio_port *vport, return;
> > }
> >
> > - caps->request = request;
> > + caps->request = htole32(request);
> > VD_AGENT_SET_CAPABILITY(caps->caps, VD_AGENT_CAP_MOUSE_STATE);
> > VD_AGENT_SET_CAPABILITY(caps->caps,
> > VD_AGENT_CAP_MONITORS_CONFIG); VD_AGENT_SET_CAPABILITY(caps->caps,
> > VD_AGENT_CAP_REPLY); @@ -102,6 +102,8 @@ 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);
> > + for (i = 0; i < VD_AGENT_CAPS_SIZE; i++)
> > + caps->caps[i] = le32toh(caps->caps[i]);
>
> hmm, I got confused here... I guess that the macros should take in
> consideration the guest/client endianness as well..
That's what the name and description says and it works on both endian
guests.
>
> >
> > vdagent_virtio_port_write(vport, VDP_CLIENT_PORT,
> > VD_AGENT_ANNOUNCE_CAPABILITIES, 0,
> > @@ -122,7 +124,10 @@ static void do_client_monitors(struct
> > vdagent_virtio_port *vport, int port_nr, VDAgentMessage
> > *message_header, VDAgentMonitorsConfig *new_monitors) {
> > VDAgentReply reply;
> > - uint32_t size;
> > + uint32_t size, i;
> > +
> > + new_monitors->num_of_monitors =
> > le32toh(new_monitors->num_of_monitors);
> > + new_monitors->flags = le32toh(new_monitors->flags);
>
> Instead handling the swapping in every message handler, i think it
> might be nicer to do a helper function to be called at
> virtio_port_read_complete()
The problem is the message structure differs. Maybe I missed some
pattern common to multiple message types but in general the message may
contain any combination of single byte and multibyte values.
If there are a few message types that are purely an arrray of 32bit
integers it might make sense to make a common function for them, true.
The other one that is swapped completely is audio and that is 16bit so
doing 32bit swap would probably swap the channels.
>
> >
> > /* Store monitor config to send to agents when they connect */
> > size = sizeof(VDAgentMonitorsConfig) +
> > @@ -132,6 +137,14 @@ static void do_client_monitors(struct
> > vdagent_virtio_port *vport, int port_nr, return;
> > }
> >
> > + for (i = 0; i < new_monitors->num_of_monitors; i++) {
> > + new_monitors->monitors[i].height =
> > le32toh(new_monitors->monitors[i].height);
> > + new_monitors->monitors[i].width =
> > le32toh(new_monitors->monitors[i].width);
> > + new_monitors->monitors[i].depth =
> > le32toh(new_monitors->monitors[i].depth);
> > + new_monitors->monitors[i].x =
> > le32toh(new_monitors->monitors[i].x);
> > + new_monitors->monitors[i].y =
> > le32toh(new_monitors->monitors[i].y);
> > + }
> > +
> > vdagentd_write_xorg_conf(new_monitors);
> >
> > if (!mon_config ||
> > @@ -151,8 +164,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 = htole32(VD_AGENT_MONITORS_CONFIG);
> > + reply.error = htole32(VD_AGENT_SUCCESS);
> > vdagent_virtio_port_write(vport, port_nr, VD_AGENT_REPLY, 0,
> > (uint8_t *)&reply, sizeof(reply));
> > }
> > @@ -161,11 +174,15 @@ static void do_client_volume_sync(struct
> > vdagent_virtio_port *vport, int port_nr VDAgentMessage
> > *message_header, VDAgentAudioVolumeSync *avs)
> > {
> > + int i;
> > if (active_session_conn == NULL) {
> > syslog(LOG_DEBUG, "No active session - Can't volume-sync");
> > return;
> > }
> >
> > + for (i = 0; i < avs->nchannels; i++)
> > + avs->volume[i] = le16toh(avs->volume[i]);
> > +
> > udscs_write(active_session_conn, VDAGENTD_AUDIO_VOLUME_SYNC,
> > 0, 0, (uint8_t *)avs, message_header->size);
> > }
> > @@ -174,7 +191,7 @@ static void do_client_capabilities(struct
> > vdagent_virtio_port *vport, VDAgentMessage *message_header,
> > VDAgentAnnounceCapabilities *caps)
> > {
> > - int new_size =
> > VD_AGENT_CAPS_SIZE_FROM_MSG_SIZE(message_header->size);
> > + int i, new_size =
> > VD_AGENT_CAPS_SIZE_FROM_MSG_SIZE(message_header->size);
> > if (capabilities_size != new_size) {
> > capabilities_size = new_size;
> > @@ -186,7 +203,8 @@ static void do_client_capabilities(struct
> > vdagent_virtio_port *vport, return;
> > }
> > }
> > - memcpy(capabilities, caps->caps, capabilities_size *
> > sizeof(uint32_t));
> > + for (i = 0; i < capabilities_size; i++)
> > + capabilities[i] = le32toh(caps->caps[i]);
> > if (caps->request) {
> > /* Report the previous client has disconneced. */
> > do_client_disconnect();
> > @@ -225,7 +243,7 @@ static void do_client_clipboard(struct
> > vdagent_virtio_port *vport, case VD_AGENT_CLIPBOARD_REQUEST: {
> > VDAgentClipboardRequest *req = (VDAgentClipboardRequest
> > *)data; msg_type = VDAGENTD_CLIPBOARD_REQUEST;
> > - data_type = req->type;
> > + data_type = le32toh(req->type);
> > data = NULL;
> > size = 0;
> > break;
> > @@ -233,7 +251,7 @@ static void do_client_clipboard(struct
> > vdagent_virtio_port *vport, case VD_AGENT_CLIPBOARD: {
> > VDAgentClipboard *clipboard = (VDAgentClipboard *)data;
> > msg_type = VDAGENTD_CLIPBOARD_DATA;
> > - data_type = clipboard->type;
> > + data_type = le32toh(clipboard->type);
> > size = size - sizeof(VDAgentClipboard);
> > data = clipboard->data;
> > break;
> > @@ -255,8 +273,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 = htole32(id),
> > + .result = htole32(xfer_status),
> > };
> > syslog(LOG_WARNING, msg, id);
> > if (vport)
> > @@ -275,6 +293,7 @@ static void do_client_file_xfer(struct
> > vdagent_virtio_port *vport, switch (message_header->type) {
> > case VD_AGENT_FILE_XFER_START: {
> > VDAgentFileXferStartMessage *s =
> > (VDAgentFileXferStartMessage *)data;
> > + s->id = le32toh(s->id);
> > if (!active_session_conn) {
> > send_file_xfer_status(vport,
> > "Could not find an agent connnection belonging to
> > the " @@ -295,12 +314,16 @@ static void do_client_file_xfer(struct
> > vdagent_virtio_port *vport, }
> > case VD_AGENT_FILE_XFER_STATUS: {
> > VDAgentFileXferStatusMessage *s =
> > (VDAgentFileXferStatusMessage *)data;
> > + s->id = le32toh(s->id);
> > + s->result = le64toh(s->result);
> > msg_type = VDAGENTD_FILE_XFER_STATUS;
> > id = s->id;
> > break;
> > }
> > case VD_AGENT_FILE_XFER_DATA: {
> > VDAgentFileXferDataMessage *d =
> > (VDAgentFileXferDataMessage *)data;
> > + d->id = le32toh(d->id);
> > + d->size = le64toh(d->size);
> > msg_type = VDAGENTD_FILE_XFER_DATA;
> > id = d->id;
> > break;
> > @@ -399,15 +422,18 @@ static int virtio_port_read_complete(
> > if (message_header->size != sizeof(VDAgentMaxClipboard))
> > goto size_error;
> > VDAgentMaxClipboard *msg = (VDAgentMaxClipboard *)data;
>
> VDAgentMaxClipboard *msg = payload_to_vdagent_msg (data,
> VD_AGENT_MAX_CLIPBOARD); and handle the byte swapping there for each
> message.
hm, many of the clipboard messages are handled similarly so there is
probably some room for unification there.
>
> I'll try to test it tomorrow.
> Sorry for taking some time to reply back and many thanks for the
> patches :)
>
> toso
>
Thanks for the review
Michal
Attachment:
pgpC6PFA6ucA5.pgp
Description: OpenPGP digital signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel