[Spice-devel] [PATCH 5/7] spice-ppc: avoid casts to lessers types!

[Christophe Fergeau]

Hey,

On Tue, Aug 07, 2012 at 03:43:12PM -0300, Erlon Cruz wrote:
> From: Fabiano Fidêncio <fabiano at fidencio.org>
> 
>     It's breaking PPC's keyboard functionality, once this cast is
>     getting the first byte (from left to right) on any architecture,
>     what's wrong when we think in a PPC (we should get the last one,
>     instead).
> 
> Signed-off-by: Erlon R. Cruz <erlon.cruz at br.flextronics.com>
> Signed-off-by: Fabiano Fidêncio <Fabiano.Fidêncio at fit-tecnologia.org.br>
> Signed-off-by: Rafael F. Santos <Rafael.Santos at fit-tecnologia.org.br>
> 
> ---
>  server/inputs_channel.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/server/inputs_channel.c b/server/inputs_channel.c
> index e14e995..015f7b5 100644
> --- a/server/inputs_channel.c
> +++ b/server/inputs_channel.c
> @@ -289,7 +289,7 @@ static int inputs_channel_handle_parsed(RedChannelClient *rcc, uint32_t size, ui
>  {
>      InputsChannel *inputs_channel = (InputsChannel *)rcc->channel;
>      InputsChannelClient *icc = (InputsChannelClient *)rcc;
> -    uint8_t *buf = (uint8_t *)message;
> +    uint32_t *buf = message;

I don't think this one is needed as 'buf' will be cast to
SpiceMsgcSomething * before being used.

>  
>      spice_assert(g_inputs_channel == inputs_channel);
>      switch (type) {
> @@ -302,8 +302,8 @@ static int inputs_channel_handle_parsed(RedChannelClient *rcc, uint32_t size, ui
>      }
>      case SPICE_MSGC_INPUTS_KEY_UP: {
>          SpiceMsgcKeyDown *key_down = (SpiceMsgcKeyDown *)buf;

This should be SpiceMsgcKeyUp (but the 2 structs are the same)

> -        uint8_t *now = (uint8_t *)&key_down->code;
> -        uint8_t *end = now + sizeof(key_down->code);
> +        uint32_t *now = &key_down->code;
> +        uint32_t *end = now + sizeof(key_down->code);

Nope, this will overflow the SpiceMsgcKeyDown struct. Before 'end' was
pointing 4 bytes after 'now', after these changes, 'end' will be pointing
4 * sizeof(uint32_t) bytes after 'now' (assuming sizeof(key_down->code) is
4 bytes).

>          for (; now < end && *now; now++) {
>              kbd_push_scan(keyboard, *now);
>          }

Wow, complicated code (before your changes). It seems the reason we need
to loop is because of:
if (scancode < 0x100) {
    up.code = scancode | 0x80;
} else {
    up.code = 0x80e0 | ((scancode - 0x100) << 8);
}
in spice_inputs_key_release in spice-gtk/gtk/channel-inputs.c

SpiceMsgcKeyUp would probably be better as a uint8 codes[4] in
spice1.proto, and then we could iterate over this array without endianess
issues. This has to be carefully considered to be sure it doesn't break the
protocol though. Another way of doing it is to just change this part of the
code and to use bitshifts and masking, something like:
uint32_t code = key_down->code;

for (; code != 0; code >>= 8) {
    kbd_push_scan(keyboard, code & 0xff);
}

Though after writing this, I may be missing something, in the up.code =
0x80e0 | 0xNN00 case, I have no idea if we want to push the less
significant byte first. Hopefully someone more familiar with scancodes will
chime in ;)

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20120808/adccf5c2/attachment.pgp>