On Fri, Mar 27, 2015 at 05:49:30PM +0300, Denis Kirjanov wrote: > On 3/27/15, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote: > > Hey, > > > > On Thu, Mar 26, 2015 at 05:52:05PM +0300, Denis Kirjanov wrote: > >> Since the spice protocol is LE send the number of channels in > >> the proper byte order > >> > >> Signed-off-by: Denis Kirjanov <kda@xxxxxxxxxxxxxxxxx> > >> --- > >> server/reds.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/server/reds.c b/server/reds.c > >> index f61d5d3..468ac9c 100644 > >> --- a/server/reds.c > >> +++ b/server/reds.c > >> @@ -37,6 +37,8 @@ > >> #include <ctype.h> > >> #include <stdbool.h> > >> > >> +#include <glib.h> > >> + > >> #include <openssl/err.h> > >> > >> #if HAVE_SASL > >> @@ -874,7 +876,7 @@ void reds_fill_channels(SpiceMsgChannels > >> *channels_info) > >> used_channels++; > >> } > >> > >> - channels_info->num_of_channels = used_channels; > >> + channels_info->num_of_channels = GUINT32_TO_LE(used_channels); > > > > Are you sure this is needed? In other words, what concrete bug are you > > observing without that change? > > reds_fill_channels is called from main_channel_marshall_channels(), > > which then passes the filled SpiceMsgChannels instance to > > spice_marshall_msg_main_channels_list() > > > > spice_marshall_msg_* is generated from the description of the > > SPICE protocol available in spice.proto/spice1.proto. These generated > > marshallers are usually correct from an endianness point of view. > > > > This specific function is defined in > > spice-common/common/generated_server_marshallers.c and does: > > > > spice_marshaller_add_uint32(m, src->num_of_channels); > > > > spice_marshaller_add_uint32() calls spice_marshaller_add_uint32() which > > is > > #ifdef WORDS_BIGENDIAN > > #define write_uint32(ptr,v) (*((uint32_t *)(ptr)) = > > SPICE_BYTESWAP32((uint32_t)(v))) > > #else > > #define write_uint32(ptr,v) (*((uint32_t *)(ptr)) = v) > > #endif > > > > so I would say the existing code is correct, and this patch unneeded. > > The problem is that write_uint32t writes the value in the BE even if > the client is LE. Obviously it's wrong. As I explained above, it's supposed to write it on the wire as a little-endian number, so if you get a number of channels in big-endian on the wire, something is wrong with the reasoning above. In this case it would be good to understand which part of it is wrong, as a different fix may be needed. If you get a big endian value, one thing which could go wrong is WORDS_BIGENDIAN not being defined when marshaller.c is compiled on a BE machine. I'm sure more things could fail :) Christophe
Attachment:
pgpwuSwoF4cyU.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel