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

[Michal Suchánek]

Hello,

On Wed, 11 Jan 2017 17:04:10 +0100
Victor Toso <victortoso at redhat.com> 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 <msuchanek at suse.de>
> >
> > ---
> > 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