Re: [libvirt] [PATCH 1/7] Rewrite keycode map to avoid a struct
On 04/03/2013 09:06 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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(a)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
Attachments:
- signature.asc (application/pgp-signature — 621 bytes)