[libvirt] [PATCH 1/7] Rewrite keycode map to avoid a struct

[Eric Blake]

On 04/03/2013 09:06 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> Playing games with field offsets in a struct causes all sorts
> of alignment warnings on ARM platforms
> 
> util/virkeycode.c: In function '__virKeycodeValueFromString':
> util/virkeycode.c:26:7: warning: cast increases required alignment of target type [-Wcast-align]
>      (*(typeof(field_type) *)((char *)(object) + field_offset))

Wow.  The end result is still properly aligned if object was aligned to
begin with, but the warning is quite appropriate, as the code is hard to
follow.

> 
> There is no compelling reason to use a struct for the keycode
> tables. It can easily just use an array of arrays instead,
> avoiding all alignment problems
> 
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>  src/util/virkeycode-mapgen.py |  78 ++++++++++++++++++-------
>  src/util/virkeycode.c         | 130 ++++++++++++++++++------------------------
>  2 files changed, 110 insertions(+), 98 deletions(-)

[Fair warning - my python is weak, so I cheated and looked at the
generated C code virkeymaps.h file pre- and post-patch instead - you
basically went from a single table of structs:

-struct keycode virKeycodes[] = {

-    { "KEY_RESERVED",NULL,NULL,0,0,0,0,0,0,0,0,0,0},

to multiple tables of one piece of information per table:

+const char *virKeymapNames_linux[] = {
+  "KEY_RESERVED",
...
+unsigned short virKeymapValues_linux[] = {
+  0,
]

> +++ b/src/util/virkeycode.c
> @@ -22,51 +22,57 @@
>  #include <string.h>
>  #include <stddef.h>
>  
> -#define getfield(object, field_type, field_offset) \
> -    (*(typeof(field_type) *)((char *)(object) + field_offset))
> -
> -struct keycode {
> -    const char *linux_name;
> -    const char *os_x_name;
> -    const char *win32_name;
> -    unsigned short linux_keycode;
> -    unsigned short os_x;
> -    unsigned short atset1;
> -    unsigned short atset2;
> -    unsigned short atset3;
> -    unsigned short xt;
> -    unsigned short xt_kbd;
> -    unsigned short usb;
> -    unsigned short win32;
> -    unsigned short rfb;
> -};

This maps up with dropping the struct...

>  
>  #define VIRT_KEY_INTERNAL
>  #include "virkeymaps.h"
>  
> -static unsigned int codeOffset[] = {
> +static const char **virKeymapNames[] = {
>      [VIR_KEYCODE_SET_LINUX] =
> -        offsetof(struct keycode, linux_keycode),
> +      virKeymapNames_linux,

...and this maps up with pointing to the individual table, rather than a
funky offset within the struct at element 0 of the big array.

ACK (once you clean up the nit that Michal pointed out about 'make
syntax-check')

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130405/5396778c/attachment-0001.sig>