Hello, On Wed, 11 Jan 2017 17:04:10 +0100 Victor Toso <victortoso@xxxxxxxxxx> 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@xxxxxxx> > > > > --- > > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel