Re: [Spice-devel] [PATCH v3 1/2] Do endian swapping.

[Michal Suchánek]

On Fri, 13 Jan 2017 06:52:17 -0500 (EST)
Frediano Ziglio  wrote:

> >  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
> > */  
> 
> Why not GUINT64_FROM_LE ?

Missed the 64bit functions when changing to glib macros.

> There could be portability issues if id is not 64-bit aligned here.
> Would be better to use VDAgentFileXferDataMessage structure which
> has the right attributes to support misalignment.

yes, that might be better option.

Thanks

Michal
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH v3 1/2] Do endian swapping.

[Frediano Ziglio]

>
> 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 
> 
> ---
> 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
> ---
>  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 *), 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(, (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);
>  

Re: [Spice-devel] [PATCH v3 1/2] Do endian swapping.

[Michal Suchánek]

Hello,

On Wed, 11 Jan 2017 17:04:10 +0100
Victor Toso  wrote:

> 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 
> >
> > ---
> > 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.

The virtio_port_read_complete callback is about as explicit as it gets
without an IDL. It does all the swapping on incoming messages except for
the outer headers which are swapped in
vdagent_virtio_port_do_read/vdagent_virtio_port_do_chunk. The case
switch in virtio_port_read_complete is comparable to the else if
branching in param_spec_is_equal in your example.

Also the swapping of results is now moved exclusively into functions
that already call virtio_port_write and results are not pre-swapped
earlier (unless I missed some ;-).

> 
> If this needs a bigger refactor then I'm proposing, I'm would love to
> hear some ideas :)

If you want to port vdagent to use an IDL with auto-generated code
which includes field decoding and swapping for all messages ... well,
it would be a bit more challenging with the chunks and variable
fields but I am sure vdagent is not the first software to use packets.

I would like to avoid that, personally. And I am not very familiar with
any IDL tools so I would not do the porting.

> 
> > 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.

They are both mnemonic and nice .. in a different, incompatible way ;-)

Thanks

Michal
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel