[libvirt] [PATCH 0/7] Fix multiple compiler warnings on ARMv7
The ARMv7 builds of libvirt generate a number of warnings, mostly about cast alignment http://arm.koji.fedoraproject.org/packages/libvirt/1.0.4/1.fc19/data/logs/ar... this patch series fixes as many as possible, and then disables warnings for the rest using a pragma
From: "Daniel P. Berrange" <berrange@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)) ^ util/virkeycode.c:91:28: note: in expansion of macro 'getfield' const char *name = getfield(virKeycodes + i, const char *, name_offset); ^ util/virkeycode.c:26:7: warning: cast increases required alignment of target type [-Wcast-align] (*(typeof(field_type) *)((char *)(object) + field_offset)) ^ util/virkeycode.c:94:20: note: in expansion of macro 'getfield' return getfield(virKeycodes + i, unsigned short, code_offset); ^ util/virkeycode.c: In function '__virKeycodeValueTranslate': util/virkeycode.c:26:7: warning: cast increases required alignment of target type [-Wcast-align] (*(typeof(field_type) *)((char *)(object) + field_offset)) ^ util/virkeycode.c:127:13: note: in expansion of macro 'getfield' if (getfield(virKeycodes + i, unsigned short, from_offset) == key_value) ^ util/virkeycode.c:26:7: warning: cast increases required alignment of target type [-Wcast-align] (*(typeof(field_type) *)((char *)(object) + field_offset)) ^ util/virkeycode.c:128:20: note: in expansion of macro 'getfield' return getfield(virKeycodes + i, unsigned short, to_offset); 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@redhat.com> --- src/util/virkeycode-mapgen.py | 78 ++++++++++++++++++------- src/util/virkeycode.c | 130 ++++++++++++++++++------------------------ 2 files changed, 110 insertions(+), 98 deletions(-) diff --git a/src/util/virkeycode-mapgen.py b/src/util/virkeycode-mapgen.py index d3d2aae..34de637 100755 --- a/src/util/virkeycode-mapgen.py +++ b/src/util/virkeycode-mapgen.py @@ -13,7 +13,22 @@ instead of keymaps.csv which is a mirror. import sys import re -namecolums = (0,2,10) +cols = ( + ["linux", True], + ["linux", False], + ["os_x", True], + ["os_x", False], + ["atset1", False], + ["atset2", False], + ["atset3", False], + ["xt", False], + ["xt_kbd", False], + ["usb", False], + ["win32", True], + ["win32", False], + ["rfb", False], +) + xtkbdkey_index = 8 def quotestring(str): @@ -28,29 +43,48 @@ print ''' # error do not use this; it is not a public header #endif -struct keycode virKeycodes[] = { ''' sys.stdin.readline() # eat the fist line. +keycodes = [] + +max = 0 + for line in sys.stdin.xreadlines(): - a = re.match("([^,]*)," * 13 + "([^,]*)$", line[0:-1]).groups() - b = "" - rfbkey = 0 - for i in namecolums: - b = b + (a[i] and quotestring(a[i]) or 'NULL') + ',' - for i in [ x for x in range(12) if not x in namecolums ]: - b = b + (a[i] or '0') + ',' - if i == xtkbdkey_index: - # RFB keycodes are XT kbd keycodes with a slightly - # different encoding of 0xe0 scan codes. RFB uses - # the high bit of the first byte, instead of the low - # bit of the second byte. - rfbkey = int(a[i] or '0') - rfbkey = (rfbkey & 0x100) >> 1 | (rfbkey & 0x7f) - - # Append RFB keycode as the last column - b = b + str(rfbkey) - print " { " + b + "}," - -print '};' + values = re.match("([^,]*)," * 13 + "([^,]*)$", line[0:-1]).groups() + + data = [] + for v in values: + data.append(v) + + # RFB keycodes are XT kbd keycodes with a slightly + # different encoding of 0xe0 scan codes. RFB uses + # the high bit of the first byte, instead of the low + # bit of the second byte. + rfbkey = int(data[xtkbdkey_index] or '0') + rfbkey = (rfbkey & 0x100) >> 1 | (rfbkey & 0x7f) + data.append(rfbkey) + + keycodes.append(data) + max = max + 1 + +print "#define VIR_KEYMAP_ENTRY_MAX " + str(max) + +for i in range(len(cols)): + col=cols[i] + name=col[0] + isname=col[1] + if isname: + print "const char *virKeymapNames_" + name + "[] = {" + else: + print "unsigned short virKeymapValues_" + name + "[] = {" + + for entry in keycodes: + if isname: + print " " + quotestring(entry[i] or "NULL") + "," + else: + print " " + (entry[i] or "0") + "," + + print "};\n" + diff --git a/src/util/virkeycode.c b/src/util/virkeycode.c index 7ce485c..9b2809d 100644 --- a/src/util/virkeycode.c +++ 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; -}; #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, [VIR_KEYCODE_SET_XT] = - offsetof(struct keycode, xt), + NULL, [VIR_KEYCODE_SET_ATSET1] = - offsetof(struct keycode, atset1), + NULL, [VIR_KEYCODE_SET_ATSET2] = - offsetof(struct keycode, atset2), + NULL, [VIR_KEYCODE_SET_ATSET3] = - offsetof(struct keycode, atset3), + NULL, [VIR_KEYCODE_SET_OSX] = - offsetof(struct keycode, os_x), + virKeymapNames_os_x, [VIR_KEYCODE_SET_XT_KBD] = - offsetof(struct keycode, xt_kbd), + NULL, [VIR_KEYCODE_SET_USB] = - offsetof(struct keycode, usb), + NULL, [VIR_KEYCODE_SET_WIN32] = - offsetof(struct keycode, win32), + virKeymapNames_win32, [VIR_KEYCODE_SET_RFB] = - offsetof(struct keycode, rfb), + NULL, }; -verify(ARRAY_CARDINALITY(codeOffset) == VIR_KEYCODE_SET_LAST); +verify(ARRAY_CARDINALITY(virKeymapNames) == VIR_KEYCODE_SET_LAST); + +static unsigned short *virKeymapValues[] = { + [VIR_KEYCODE_SET_LINUX] = + virKeymapValues_linux, + [VIR_KEYCODE_SET_XT] = + virKeymapValues_xt, + [VIR_KEYCODE_SET_ATSET1] = + virKeymapValues_atset1, + [VIR_KEYCODE_SET_ATSET2] = + virKeymapValues_atset2, + [VIR_KEYCODE_SET_ATSET3] = + virKeymapValues_atset3, + [VIR_KEYCODE_SET_OSX] = + virKeymapValues_os_x, + [VIR_KEYCODE_SET_XT_KBD] = + virKeymapValues_xt_kbd, + [VIR_KEYCODE_SET_USB] = + virKeymapValues_usb, + [VIR_KEYCODE_SET_WIN32] = + virKeymapValues_win32, + [VIR_KEYCODE_SET_RFB] = + virKeymapValues_rfb, +}; +verify(ARRAY_CARDINALITY(virKeymapValues) == VIR_KEYCODE_SET_LAST); VIR_ENUM_IMPL(virKeycodeSet, VIR_KEYCODE_SET_LAST, "linux", @@ -81,68 +87,40 @@ VIR_ENUM_IMPL(virKeycodeSet, VIR_KEYCODE_SET_LAST, "rfb", ); -static int __virKeycodeValueFromString(unsigned int name_offset, - unsigned int code_offset, - const char *keyname) +int virKeycodeValueFromString(virKeycodeSet codeset, + const char *keyname) { int i; - for (i = 0; i < ARRAY_CARDINALITY(virKeycodes); i++) { - const char *name = getfield(virKeycodes + i, const char *, name_offset); + for (i = 0; i < VIR_KEYMAP_ENTRY_MAX; i++) { + if (!virKeymapNames[codeset] || + !virKeymapValues[codeset]) + continue; - if (name && STREQ(name, keyname)) - return getfield(virKeycodes + i, unsigned short, code_offset); - } - - return -1; -} + const char *name = virKeymapNames[codeset][i]; -int virKeycodeValueFromString(virKeycodeSet codeset, const char *keyname) -{ - switch (codeset) { - case VIR_KEYCODE_SET_LINUX: - return __virKeycodeValueFromString(offsetof(struct keycode, linux_name), - offsetof(struct keycode, linux_keycode), - keyname); - case VIR_KEYCODE_SET_OSX: - return __virKeycodeValueFromString(offsetof(struct keycode, os_x_name), - offsetof(struct keycode, os_x), - keyname); - case VIR_KEYCODE_SET_WIN32: - return __virKeycodeValueFromString(offsetof(struct keycode, win32_name), - offsetof(struct keycode, win32), - keyname); - default: - return -1; - } -} - -static int __virKeycodeValueTranslate(unsigned int from_offset, - unsigned int to_offset, - int key_value) -{ - int i; - - for (i = 0; i < ARRAY_CARDINALITY(virKeycodes); i++) { - if (getfield(virKeycodes + i, unsigned short, from_offset) == key_value) - return getfield(virKeycodes + i, unsigned short, to_offset); + if (name && STREQ_NULLABLE(name, keyname)) + return virKeymapValues[codeset][i]; } return -1; } + int virKeycodeValueTranslate(virKeycodeSet from_codeset, virKeycodeSet to_codeset, int key_value) { - if (key_value <= 0) - return -1; + int i; - key_value = __virKeycodeValueTranslate(codeOffset[from_codeset], - codeOffset[to_codeset], - key_value); if (key_value <= 0) return -1; - return key_value; + + for (i = 0; i < VIR_KEYMAP_ENTRY_MAX; i++) { + if (virKeymapValues[from_codeset][i] == key_value) + return virKeymapValues[to_codeset][i]; + } + + return -1; } -- 1.8.1.4
On 03.04.2013 17:06, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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)) ^ util/virkeycode.c:91:28: note: in expansion of macro 'getfield' const char *name = getfield(virKeycodes + i, const char *, name_offset); ^ util/virkeycode.c:26:7: warning: cast increases required alignment of target type [-Wcast-align] (*(typeof(field_type) *)((char *)(object) + field_offset)) ^ util/virkeycode.c:94:20: note: in expansion of macro 'getfield' return getfield(virKeycodes + i, unsigned short, code_offset); ^ util/virkeycode.c: In function '__virKeycodeValueTranslate': util/virkeycode.c:26:7: warning: cast increases required alignment of target type [-Wcast-align] (*(typeof(field_type) *)((char *)(object) + field_offset)) ^ util/virkeycode.c:127:13: note: in expansion of macro 'getfield' if (getfield(virKeycodes + i, unsigned short, from_offset) == key_value) ^ util/virkeycode.c:26:7: warning: cast increases required alignment of target type [-Wcast-align] (*(typeof(field_type) *)((char *)(object) + field_offset)) ^ util/virkeycode.c:128:20: note: in expansion of macro 'getfield' return getfield(virKeycodes + i, unsigned short, to_offset);
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@redhat.com> --- src/util/virkeycode-mapgen.py | 78 ++++++++++++++++++------- src/util/virkeycode.c | 130 ++++++++++++++++++------------------------ 2 files changed, 110 insertions(+), 98 deletions(-)
diff --git a/src/util/virkeycode-mapgen.py b/src/util/virkeycode-mapgen.py index d3d2aae..34de637 100755 --- a/src/util/virkeycode-mapgen.py +++ b/src/util/virkeycode-mapgen.py @@ -13,7 +13,22 @@ instead of keymaps.csv which is a mirror. import sys import re
-namecolums = (0,2,10) +cols = ( + ["linux", True], + ["linux", False], + ["os_x", True], + ["os_x", False], + ["atset1", False], + ["atset2", False], + ["atset3", False], + ["xt", False], + ["xt_kbd", False], + ["usb", False], + ["win32", True], + ["win32", False], + ["rfb", False], +) + xtkbdkey_index = 8
def quotestring(str): @@ -28,29 +43,48 @@ print ''' # error do not use this; it is not a public header #endif
-struct keycode virKeycodes[] = { '''
sys.stdin.readline() # eat the fist line.
+keycodes = [] + +max = 0 + for line in sys.stdin.xreadlines(): - a = re.match("([^,]*)," * 13 + "([^,]*)$", line[0:-1]).groups() - b = "" - rfbkey = 0 - for i in namecolums: - b = b + (a[i] and quotestring(a[i]) or 'NULL') + ',' - for i in [ x for x in range(12) if not x in namecolums ]: - b = b + (a[i] or '0') + ',' - if i == xtkbdkey_index: - # RFB keycodes are XT kbd keycodes with a slightly - # different encoding of 0xe0 scan codes. RFB uses - # the high bit of the first byte, instead of the low - # bit of the second byte. - rfbkey = int(a[i] or '0') - rfbkey = (rfbkey & 0x100) >> 1 | (rfbkey & 0x7f) - - # Append RFB keycode as the last column - b = b + str(rfbkey) - print " { " + b + "}," - -print '};' + values = re.match("([^,]*)," * 13 + "([^,]*)$", line[0:-1]).groups() + + data = [] + for v in values: + data.append(v) + + # RFB keycodes are XT kbd keycodes with a slightly + # different encoding of 0xe0 scan codes. RFB uses + # the high bit of the first byte, instead of the low + # bit of the second byte. + rfbkey = int(data[xtkbdkey_index] or '0') + rfbkey = (rfbkey & 0x100) >> 1 | (rfbkey & 0x7f) + data.append(rfbkey) + + keycodes.append(data) + max = max + 1 + +print "#define VIR_KEYMAP_ENTRY_MAX " + str(max) + +for i in range(len(cols)): + col=cols[i] + name=col[0] + isname=col[1] + if isname: + print "const char *virKeymapNames_" + name + "[] = {" + else: + print "unsigned short virKeymapValues_" + name + "[] = {" + + for entry in keycodes: + if isname: + print " " + quotestring(entry[i] or "NULL") + "," + else: + print " " + (entry[i] or "0") + "," + + print "};\n" +
Empty line at EOF. Michal
On 04/03/2013 09:06 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@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@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
From: "Daniel P. Berrange" <berrange@redhat.com> The virNetlinkCommand() method takes an 'unsigned char **' parameter to be filled with the received netlink message. The callers then immediately cast this to 'struct nlmsghdr', triggering (bogus) warnings about increasing alignment requirements util/virnetdev.c: In function 'virNetDevLinkDump': util/virnetdev.c:1300:12: warning: cast increases required alignment of target type [-Wcast-align] resp = (struct nlmsghdr *)*recvbuf; ^ util/virnetdev.c: In function 'virNetDevSetVfConfig': util/virnetdev.c:1429:12: warning: cast increases required alignment of target type [-Wcast-align] resp = (struct nlmsghdr *)recvbuf; Since all callers cast to 'struct nlmsghdr' we can avoid the warning problem entirely by simply changing the signature of virNetlinkCommand to return a 'struct nlmsghdr **' instead of 'unsigned char **'. The way we do the cast inside virNetlinkCommand does not have any alignment issues. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virnetdev.c | 33 +++++++++------------------------ src/util/virnetdev.h | 1 - src/util/virnetdevmacvlan.c | 30 +++++++++++------------------- src/util/virnetdevvportprofile.c | 23 ++++++----------------- src/util/virnetlink.c | 14 ++++++++------ src/util/virnetlink.h | 9 +++++++-- 6 files changed, 41 insertions(+), 69 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 00e0f94..7ffaac1 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1226,8 +1226,6 @@ static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = { * @ifindex: The interface index; may be < 0 if ifname is given * @nlattr: pointer to a pointer of netlink attributes that will contain * the results - * @recvbuf: Pointer to the buffer holding the returned netlink response - * message; free it, once not needed anymore * @src_pid: pid used for nl_pid of the local end of the netlink message * (0 == "use getpid()") * @dst_pid: pid of destination nl_pid if the kernel @@ -1241,11 +1239,10 @@ static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = { int virNetDevLinkDump(const char *ifname, int ifindex, struct nlattr **tb, - unsigned char **recvbuf, uint32_t src_pid, uint32_t dst_pid) { int rc = -1; - struct nlmsghdr *resp; + struct nlmsghdr *resp = NULL; struct nlmsgerr *err; struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC, @@ -1254,8 +1251,6 @@ virNetDevLinkDump(const char *ifname, int ifindex, unsigned int recvbuflen; struct nl_msg *nl_msg; - *recvbuf = NULL; - if (ifname && ifindex <= 0 && virNetDevGetIndex(ifname, &ifindex) < 0) return -1; @@ -1290,15 +1285,13 @@ virNetDevLinkDump(const char *ifname, int ifindex, } # endif - if (virNetlinkCommand(nl_msg, recvbuf, &recvbuflen, + if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, src_pid, dst_pid, NETLINK_ROUTE, 0) < 0) goto cleanup; - if (recvbuflen < NLMSG_LENGTH(0) || *recvbuf == NULL) + if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL) goto malformed_resp; - resp = (struct nlmsghdr *)*recvbuf; - switch (resp->nlmsg_type) { case NLMSG_ERROR: err = (struct nlmsgerr *)NLMSG_DATA(resp); @@ -1326,9 +1319,8 @@ virNetDevLinkDump(const char *ifname, int ifindex, } rc = 0; cleanup: - if (rc < 0) - VIR_FREE(*recvbuf); nlmsg_free(nl_msg); + VIR_FREE(resp); return rc; malformed_resp: @@ -1348,9 +1340,8 @@ virNetDevSetVfConfig(const char *ifname, int ifindex, int vf, int vlanid, uint32_t (*getPidFunc)(void)) { int rc = -1; - struct nlmsghdr *resp; + struct nlmsghdr *resp = NULL; struct nlmsgerr *err; - unsigned char *recvbuf = NULL; unsigned int recvbuflen = 0; uint32_t pid = 0; struct nl_msg *nl_msg; @@ -1419,15 +1410,13 @@ virNetDevSetVfConfig(const char *ifname, int ifindex, int vf, } } - if (virNetlinkCommand(nl_msg, &recvbuf, &recvbuflen, 0, pid, + if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, pid, NETLINK_ROUTE, 0) < 0) goto cleanup; - if (recvbuflen < NLMSG_LENGTH(0) || recvbuf == NULL) + if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL) goto malformed_resp; - resp = (struct nlmsghdr *)recvbuf; - switch (resp->nlmsg_type) { case NLMSG_ERROR: err = (struct nlmsgerr *)NLMSG_DATA(resp); @@ -1453,7 +1442,7 @@ virNetDevSetVfConfig(const char *ifname, int ifindex, int vf, rc = 0; cleanup: nlmsg_free(nl_msg); - VIR_FREE(recvbuf); + VIR_FREE(resp); return rc; malformed_resp: @@ -1528,18 +1517,15 @@ virNetDevGetVfConfig(const char *ifname, int vf, virMacAddrPtr mac, int *vlanid) { int rc = -1; - unsigned char *recvbuf = NULL; struct nlattr *tb[IFLA_MAX + 1] = {NULL, }; int ifindex = -1; - rc = virNetDevLinkDump(ifname, ifindex, tb, &recvbuf, 0, 0); + rc = virNetDevLinkDump(ifname, ifindex, tb, 0, 0); if (rc < 0) return rc; rc = virNetDevParseVfConfig(tb, vf, mac, vlanid); - VIR_FREE(recvbuf); - return rc; } @@ -1689,7 +1675,6 @@ int virNetDevLinkDump(const char *ifname ATTRIBUTE_UNUSED, int ifindex ATTRIBUTE_UNUSED, struct nlattr **tb ATTRIBUTE_UNUSED, - unsigned char **recvbuf ATTRIBUTE_UNUSED, uint32_t src_pid ATTRIBUTE_UNUSED, uint32_t dst_pid ATTRIBUTE_UNUSED) { diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 06d0650..551ea22 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -111,7 +111,6 @@ int virNetDevGetVirtualFunctions(const char *pfname, int virNetDevLinkDump(const char *ifname, int ifindex, struct nlattr **tb, - unsigned char **recvbuf, uint32_t src_pid, uint32_t dst_pid) ATTRIBUTE_RETURN_CHECK; diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 2578ff0..e76e94c 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -110,11 +110,10 @@ virNetDevMacVLanCreate(const char *ifname, int *retry) { int rc = -1; - struct nlmsghdr *resp; + struct nlmsghdr *resp = NULL; struct nlmsgerr *err; struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC }; int ifindex; - unsigned char *recvbuf = NULL; unsigned int recvbuflen; struct nl_msg *nl_msg; struct nlattr *linkinfo, *info_data; @@ -163,16 +162,14 @@ virNetDevMacVLanCreate(const char *ifname, nla_nest_end(nl_msg, linkinfo); - if (virNetlinkCommand(nl_msg, &recvbuf, &recvbuflen, 0, 0, + if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0, NETLINK_ROUTE, 0) < 0) { goto cleanup; } - if (recvbuflen < NLMSG_LENGTH(0) || recvbuf == NULL) + if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL) goto malformed_resp; - resp = (struct nlmsghdr *)recvbuf; - switch (resp->nlmsg_type) { case NLMSG_ERROR: err = (struct nlmsgerr *)NLMSG_DATA(resp); @@ -206,7 +203,7 @@ virNetDevMacVLanCreate(const char *ifname, rc = 0; cleanup: nlmsg_free(nl_msg); - VIR_FREE(recvbuf); + VIR_FREE(resp); return rc; malformed_resp: @@ -232,10 +229,9 @@ buffer_too_small: int virNetDevMacVLanDelete(const char *ifname) { int rc = -1; - struct nlmsghdr *resp; + struct nlmsghdr *resp = NULL; struct nlmsgerr *err; struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC }; - unsigned char *recvbuf = NULL; unsigned int recvbuflen; struct nl_msg *nl_msg; @@ -252,16 +248,14 @@ int virNetDevMacVLanDelete(const char *ifname) if (nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0) goto buffer_too_small; - if (virNetlinkCommand(nl_msg, &recvbuf, &recvbuflen, 0, 0, + if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0, NETLINK_ROUTE, 0) < 0) { goto cleanup; } - if (recvbuflen < NLMSG_LENGTH(0) || recvbuf == NULL) + if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL) goto malformed_resp; - resp = (struct nlmsghdr *)recvbuf; - switch (resp->nlmsg_type) { case NLMSG_ERROR: err = (struct nlmsgerr *)NLMSG_DATA(resp); @@ -286,7 +280,7 @@ int virNetDevMacVLanDelete(const char *ifname) rc = 0; cleanup: nlmsg_free(nl_msg); - VIR_FREE(recvbuf); + VIR_FREE(resp); return rc; malformed_resp: @@ -477,7 +471,7 @@ static int instance2str(const unsigned char *p, char *dst, size_t size) /** * virNetDevMacVLanVPortProfileCallback: * - * @msg: The buffer containing the received netlink message + * @hdr: The buffer containing the received netlink header + payload * @length: The length of the received netlink message. * @peer: The netling sockaddr containing the peer information * @handled: Contains information if the message has been replied to yet @@ -489,8 +483,8 @@ static int instance2str(const unsigned char *p, char *dst, size_t size) */ static void -virNetDevMacVLanVPortProfileCallback(unsigned char *msg, - int length, +virNetDevMacVLanVPortProfileCallback(struct nlmsghdr *hdr, + unsigned int length, struct sockaddr_nl *peer, bool *handled, void *opaque) @@ -510,7 +504,6 @@ virNetDevMacVLanVPortProfileCallback(unsigned char *msg, *tb_vfinfo[IFLA_VF_MAX + 1], *tb_vfinfo_list; struct ifinfomsg ifinfo; - struct nlmsghdr *hdr; void *data; int rem; char *ifname; @@ -520,7 +513,6 @@ virNetDevMacVLanVPortProfileCallback(unsigned char *msg, pid_t virip_pid = 0; char macaddr[VIR_MAC_STRING_BUFLEN]; - hdr = (struct nlmsghdr *) msg; data = nlmsg_data(hdr); /* Quickly decide if we want this or not */ diff --git a/src/util/virnetdevvportprofile.c b/src/util/virnetdevvportprofile.c index bb97e3a..13ccbab 100644 --- a/src/util/virnetdevvportprofile.c +++ b/src/util/virnetdevvportprofile.c @@ -588,13 +588,12 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, uint8_t op) { int rc = -1; - struct nlmsghdr *resp; + struct nlmsghdr *resp = NULL; struct nlmsgerr *err; struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC, .ifi_index = ifindex, }; - unsigned char *recvbuf = NULL; unsigned int recvbuflen = 0; int src_pid = 0; uint32_t dst_pid = 0; @@ -711,15 +710,13 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, goto cleanup; } - if (virNetlinkCommand(nl_msg, &recvbuf, &recvbuflen, + if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, src_pid, dst_pid, NETLINK_ROUTE, 0) < 0) goto cleanup; - if (recvbuflen < NLMSG_LENGTH(0) || recvbuf == NULL) + if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL) goto malformed_resp; - resp = (struct nlmsghdr *)recvbuf; - switch (resp->nlmsg_type) { case NLMSG_ERROR: err = (struct nlmsgerr *)NLMSG_DATA(resp); @@ -744,7 +741,7 @@ virNetDevVPortProfileOpSetLink(const char *ifname, int ifindex, rc = 0; cleanup: nlmsg_free(nl_msg); - VIR_FREE(recvbuf); + VIR_FREE(resp); return rc; malformed_resp: @@ -784,7 +781,6 @@ virNetDevVPortProfileGetNthParent(const char *ifname, int ifindex, unsigned int { int rc; struct nlattr *tb[IFLA_MAX + 1] = { NULL, }; - unsigned char *recvbuf = NULL; bool end = false; unsigned int i = 0; @@ -794,7 +790,7 @@ virNetDevVPortProfileGetNthParent(const char *ifname, int ifindex, unsigned int return -1; while (!end && i <= nthParent) { - rc = virNetDevLinkDump(ifname, ifindex, tb, &recvbuf, 0, 0); + rc = virNetDevLinkDump(ifname, ifindex, tb, 0, 0); if (rc < 0) break; @@ -803,7 +799,6 @@ virNetDevVPortProfileGetNthParent(const char *ifname, int ifindex, unsigned int IFNAMSIZ)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("buffer for root interface name is too small")); - VIR_FREE(recvbuf); return -1; } *parent_ifindex = ifindex; @@ -815,8 +810,6 @@ virNetDevVPortProfileGetNthParent(const char *ifname, int ifindex, unsigned int } else end = true; - VIR_FREE(recvbuf); - i++; } @@ -843,7 +836,6 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex, int rc; int src_pid = 0; uint32_t dst_pid = 0; - unsigned char *recvbuf = NULL; struct nlattr *tb[IFLA_MAX + 1] = { NULL , }; int repeats = STATUS_POLL_TIMEOUT_USEC / STATUS_POLL_INTERVL_USEC; uint16_t status = 0; @@ -876,7 +868,7 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex, } while (--repeats >= 0) { - rc = virNetDevLinkDump(NULL, ifindex, tb, &recvbuf, src_pid, dst_pid); + rc = virNetDevLinkDump(NULL, ifindex, tb, src_pid, dst_pid); if (rc < 0) goto cleanup; @@ -899,8 +891,6 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex, } usleep(STATUS_POLL_INTERVL_USEC); - - VIR_FREE(recvbuf); } if (status == PORT_PROFILE_RESPONSE_INPROGRESS) { @@ -910,7 +900,6 @@ virNetDevVPortProfileOpCommon(const char *ifname, int ifindex, } cleanup: - VIR_FREE(recvbuf); return rc; } diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index 0b36fdc..af1985c 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -174,7 +174,7 @@ virNetlinkShutdown(void) * buffer will be returned. */ int virNetlinkCommand(struct nl_msg *nl_msg, - unsigned char **respbuf, unsigned int *respbuflen, + struct nlmsghdr **resp, unsigned int *respbuflen, uint32_t src_pid, uint32_t dst_pid, unsigned int protocol, unsigned int groups) { @@ -257,7 +257,8 @@ int virNetlinkCommand(struct nl_msg *nl_msg, goto error; } - *respbuflen = nl_recv(nlhandle, &nladdr, respbuf, NULL); + *respbuflen = nl_recv(nlhandle, &nladdr, + (unsigned char **)resp, NULL); if (*respbuflen <= 0) { virReportSystemError(errno, "%s", _("nl_recv failed")); @@ -265,8 +266,8 @@ int virNetlinkCommand(struct nl_msg *nl_msg, } error: if (rc == -1) { - VIR_FREE(*respbuf); - *respbuf = NULL; + VIR_FREE(*resp); + *resp = NULL; *respbuflen = 0; } @@ -324,13 +325,14 @@ virNetlinkEventCallback(int watch, void *opaque) { virNetlinkEventSrvPrivatePtr srv = opaque; - unsigned char *msg; + struct nlmsghdr *msg; struct sockaddr_nl peer; struct ucred *creds = NULL; int i, length; bool handled = false; - length = nl_recv(srv->netlinknh, &peer, &msg, &creds); + length = nl_recv(srv->netlinknh, &peer, + (unsigned char **)&msg, &creds); if (length == 0) return; diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 064f3d1..9a69a0b 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -41,6 +41,7 @@ struct nl_msg; struct sockaddr_nl; struct nlattr; +struct nlmsghdr; # endif /* __linux__ */ @@ -48,11 +49,15 @@ int virNetlinkStartup(void); void virNetlinkShutdown(void); int virNetlinkCommand(struct nl_msg *nl_msg, - unsigned char **respbuf, unsigned int *respbuflen, + struct nlmsghdr **resp, unsigned int *respbuflen, uint32_t src_pid, uint32_t dst_pid, unsigned int protocol, unsigned int groups); -typedef void (*virNetlinkEventHandleCallback)(unsigned char *msg, int length, struct sockaddr_nl *peer, bool *handled, void *opaque); +typedef void (*virNetlinkEventHandleCallback)(struct nlmsghdr *, + unsigned int length, + struct sockaddr_nl *peer, + bool *handled, + void *opaque); typedef void (*virNetlinkEventRemoveCallback)(int watch, const virMacAddrPtr macaddr, void *opaque); -- 1.8.1.4
On 04/03/2013 09:06 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The virNetlinkCommand() method takes an 'unsigned char **' parameter to be filled with the received netlink message. The callers then immediately cast this to 'struct nlmsghdr', triggering (bogus) warnings about increasing alignment requirements
Not bogus warnings unless all callers were really passing in something that was already aligned to struct nlmsghdr requirements. But your analysis was right - the pointer was properly aligned.
util/virnetdev.c: In function 'virNetDevLinkDump': util/virnetdev.c:1300:12: warning: cast increases required alignment of target type [-Wcast-align] resp = (struct nlmsghdr *)*recvbuf; ^
Since all callers cast to 'struct nlmsghdr' we can avoid the warning problem entirely by simply changing the signature of virNetlinkCommand to return a 'struct nlmsghdr **' instead of 'unsigned char **'. The way we do the cast inside virNetlinkCommand does not have any alignment issues.
Yeah, telling the compiler the correct type to begin with is better anyways. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> The current way virObject instances are allocated using VIR_ALLOC_N causes alignment warnings util/virobject.c: In function 'virObjectNew': util/virobject.c:195:11: error: cast increases required alignment of target type [-Werror=cast-align] Changing to use VIR_ALLOC_VAR will avoid the need todo the casts entirely. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virobject.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/virobject.c b/src/util/virobject.c index 808edc4..93e37e4 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -186,13 +186,13 @@ bool virClassIsDerivedFrom(virClassPtr klass, void *virObjectNew(virClassPtr klass) { virObjectPtr obj = NULL; - char *somebytes; - if (VIR_ALLOC_N(somebytes, klass->objectSize) < 0) { + if (VIR_ALLOC_VAR(obj, + char, + klass->objectSize - sizeof(virObject)) < 0) { virReportOOMError(); return NULL; } - obj = (virObjectPtr)somebytes; obj->magic = klass->magic; obj->klass = klass; -- 1.8.1.4
On 04/03/2013 09:06 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The current way virObject instances are allocated using VIR_ALLOC_N causes alignment warnings
util/virobject.c: In function 'virObjectNew': util/virobject.c:195:11: error: cast increases required alignment of target type [-Werror=cast-align]
Changing to use VIR_ALLOC_VAR will avoid the need todo
s/todo/to do/
the casts entirely.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virobject.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/util/virobject.c b/src/util/virobject.c index 808edc4..93e37e4 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -186,13 +186,13 @@ bool virClassIsDerivedFrom(virClassPtr klass, void *virObjectNew(virClassPtr klass) { virObjectPtr obj = NULL; - char *somebytes;
- if (VIR_ALLOC_N(somebytes, klass->objectSize) < 0) {
If klass->objectSize was not a multiple of sizeof(void*), then the compiler warning is correct. I seriously doubt any of the callers had this issue, though, since klass->objectSize was determined by sizeof() a struct that included virObject as a base class, and virObject includes a void* member (virClassPtr klass). So the warning is spurious.
+ if (VIR_ALLOC_VAR(obj, + char, + klass->objectSize - sizeof(virObject)) < 0) {
However, I agree that this formulation avoids the issue. The only concern I have is if a child class ever includes a 'long double' on a platform where the use of long double introduces stronger alignment requirements than 'void *'. But since we don't use 'long double' (except for virtTestCountAverage() in the testsuite, and even then I think that usage is overkill), it's academic. And even if we did, we could increase the alignment of virObject by having it contain a union with a long double member, even without increasing its size (although it might get tricky for how to do that without breaking compilation of existing code). Let's not worry about it for today. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> When reading the inotify FD, we get back a sequence of struct inotify_event, each with variable length data following. It is not safe to simply cast from the char *buf to the struct inotify_event struct since this may violate data alignment rules. Thus we must copy from the char *buf into the struct inotify_event instance before accessing the data. uml/uml_driver.c: In function 'umlInotifyEvent': uml/uml_driver.c:327:13: warning: cast increases required alignment of target type [-Wcast-align] e = (struct inotify_event *)tmp; Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/uml/uml_driver.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 09a777c..4d81a13 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -300,7 +300,7 @@ umlInotifyEvent(int watch, void *data) { char buf[1024]; - struct inotify_event *e; + struct inotify_event e; int got; char *tmp, *name; struct uml_driver *driver = data; @@ -321,20 +321,20 @@ reread: tmp = buf; while (got) { - if (got < sizeof(struct inotify_event)) + if (got < sizeof(e)) goto cleanup; /* bad */ - e = (struct inotify_event *)tmp; - tmp += sizeof(struct inotify_event); - got -= sizeof(struct inotify_event); + memcpy(&e, tmp, sizeof(e)); + tmp += sizeof(e); + got -= sizeof(e); - if (got < e->len) + if (got < e.len) goto cleanup; - tmp += e->len; - got -= e->len; + tmp += e.len; + got -= e.len; - name = (char *)&(e->name); + name = (char *)&(e.name); dom = virDomainObjListFindByName(driver->domains, name); @@ -342,7 +342,7 @@ reread: continue; } - if (e->mask & IN_DELETE) { + if (e.mask & IN_DELETE) { VIR_DEBUG("Got inotify domain shutdown '%s'", name); if (!virDomainObjIsActive(dom)) { virObjectUnlock(dom); @@ -359,7 +359,7 @@ reread: dom); dom = NULL; } - } else if (e->mask & (IN_CREATE | IN_MODIFY)) { + } else if (e.mask & (IN_CREATE | IN_MODIFY)) { VIR_DEBUG("Got inotify domain startup '%s'", name); if (virDomainObjIsActive(dom)) { virObjectUnlock(dom); -- 1.8.1.4
On 04/03/2013 09:06 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
When reading the inotify FD, we get back a sequence of struct inotify_event, each with variable length data following. It is not safe to simply cast from the char *buf to the struct inotify_event struct since this may violate data alignment rules. Thus we must copy from the char *buf into the struct inotify_event instance before accessing the data.
uml/uml_driver.c: In function 'umlInotifyEvent': uml/uml_driver.c:327:13: warning: cast increases required alignment of target type [-Wcast-align] e = (struct inotify_event *)tmp;
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/uml/uml_driver.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> There are a number of places which generate cast alignment warnings, which are difficult or impossible to address. Use pragmas to disable the warnings in these few places conf/nwfilter_conf.c: In function 'virNWFilterRuleDetailsParse': conf/nwfilter_conf.c:1806:16: warning: cast increases required alignment of target type [-Wcast-align] item = (nwItemDesc *)((char *)nwf + att[idx].dataIdx); conf/nwfilter_conf.c: In function 'virNWFilterRuleDefDetailsFormat': conf/nwfilter_conf.c:3238:16: warning: cast increases required alignment of target type [-Wcast-align] item = (nwItemDesc *)((char *)def + att[i].dataIdx); storage/storage_backend_mpath.c: In function 'virStorageBackendCreateVols': storage/storage_backend_mpath.c:247:17: warning: cast increases required alignment of target type [-Wcast-align] names = (struct dm_names *)(((char *)names) + next); nwfilter/nwfilter_dhcpsnoop.c: In function 'virNWFilterSnoopDHCPDecode': nwfilter/nwfilter_dhcpsnoop.c:994:15: warning: cast increases required alignment of target type [-Wcast-align] pip = (struct iphdr *) pep->eh_data; nwfilter/nwfilter_dhcpsnoop.c:1004:11: warning: cast increases required alignment of target type [-Wcast-align] pup = (struct udphdr *) ((char *) pip + (pip->ihl << 2)); nwfilter/nwfilter_learnipaddr.c: In function 'procDHCPOpts': nwfilter/nwfilter_learnipaddr.c:327:33: warning: cast increases required alignment of target type [-Wcast-align] uint32_t *tmp = (uint32_t *)&dhcpopt->value; nwfilter/nwfilter_learnipaddr.c: In function 'learnIPAddressThread': nwfilter/nwfilter_learnipaddr.c:501:43: warning: cast increases required alignment of target type [-Wcast-align] struct iphdr *iphdr = (struct iphdr*)(packet + nwfilter/nwfilter_learnipaddr.c:538:43: warning: cast increases required alignment of target type [-Wcast-align] struct iphdr *iphdr = (struct iphdr*)(packet + nwfilter/nwfilter_learnipaddr.c:544:48: warning: cast increases required alignment of target type [-Wcast-align] struct udphdr *udphdr= (struct udphdr *) Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- m4/virt-compile-warnings.m4 | 1 + src/conf/nwfilter_conf.c | 4 ++++ src/internal.h | 13 +++++++++++++ src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++++ src/nwfilter/nwfilter_learnipaddr.c | 10 ++++++++++ src/storage/storage_backend_mpath.c | 2 ++ 6 files changed, 34 insertions(+) diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index a08fcac..e054913 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -93,6 +93,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ if test $lv_cv_gcc_pragma_push_works = no; then dontwarn="$dontwarn -Wmissing-prototypes" dontwarn="$dontwarn -Wmissing-declarations" + dontwarn="$dontwarn -Wcast-align" fi dnl Check whether strchr(s, char variable) causes a bogus compile diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index e63a04b..b61010d 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -1803,7 +1803,9 @@ virNWFilterRuleDetailsParse(xmlNodePtr node, while (att[idx].name != NULL) { prop = virXMLPropString(node, att[idx].name); + VIR_WARNINGS_NO_CAST_ALIGN item = (nwItemDesc *)((char *)nwf + att[idx].dataIdx); + VIR_WARNINGS_RESET flags = &item->flags; flags_set = match_flag; @@ -3235,7 +3237,9 @@ virNWFilterRuleDefDetailsFormat(virBufferPtr buf, nwItemDesc *item; while (att[i].name) { + VIR_WARNINGS_NO_CAST_ALIGN item = (nwItemDesc *)((char *)def + att[i].dataIdx); + VIR_WARNINGS_RESET enum virNWFilterEntryItemFlags flags = item->flags; if ((flags & NWFILTER_ENTRY_ITEM_FLAG_EXISTS)) { if (!typeShown) { diff --git a/src/internal.h b/src/internal.h index 2cf4731..d819aa3 100644 --- a/src/internal.h +++ b/src/internal.h @@ -214,6 +214,19 @@ # endif # endif /* __GNUC__ */ + +# if __GNUC_PREREQ (4, 6) +# define VIR_WARNINGS_NO_CAST_ALIGN \ + _Pragma ("GCC diagnostic push") \ + _Pragma ("GCC diagnostic ignored \"-Wcast-align\"") + +# define VIR_WARNINGS_RESET \ + _Pragma ("GCC diagnostic pop") +# else +# define VIR_WARNINGS_NO_CAST_ALIGN +# define VIR_WARNINGS_RESET +# endif + /* * Use this when passing possibly-NULL strings to printf-a-likes. */ diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index 5124069..5012b14 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -991,7 +991,9 @@ virNWFilterSnoopDHCPDecode(virNWFilterSnoopReqPtr req, /* go through the protocol headers */ switch (ntohs(pep->eh_type)) { case ETHERTYPE_IP: + VIR_WARNINGS_NO_CAST_ALIGN; pip = (struct iphdr *) pep->eh_data; + VIR_WARNINGS_RESET; len -= offsetof(virNWFilterSnoopEthHdr, eh_data); break; default: @@ -1001,7 +1003,9 @@ virNWFilterSnoopDHCPDecode(virNWFilterSnoopReqPtr req, if (len < 0) return -2; + VIR_WARNINGS_NO_CAST_ALIGN pup = (struct udphdr *) ((char *) pip + (pip->ihl << 2)); + VIR_WARNINGS_RESET len -= pip->ihl << 2; if (len < 0) return -2; diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index 7a4f983..b399092 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -324,7 +324,9 @@ procDHCPOpts(struct dhcp *dhcp, int dhcp_opts_len, case DHCP_OPT_BCASTADDRESS: /* Broadcast address */ if (dhcp_opts_len >= 6) { + VIR_WARNINGS_NO_CAST_ALIGN uint32_t *tmp = (uint32_t *)&dhcpopt->value; + VIR_WARNINGS_RESET (*bcastaddr) = ntohl(*tmp); } break; @@ -498,8 +500,10 @@ learnIPAddressThread(void *arg) if (etherType == ETHERTYPE_IP && (header.len >= ethHdrSize + sizeof(struct iphdr))) { + VIR_WARNINGS_NO_CAST_ALIGN struct iphdr *iphdr = (struct iphdr*)(packet + ethHdrSize); + VIR_WARNINGS_RESET vmaddr = iphdr->saddr; /* skip mcast addresses (224.0.0.0 - 239.255.255.255), * class E (240.0.0.0 - 255.255.255.255, includes eth. @@ -514,8 +518,10 @@ learnIPAddressThread(void *arg) } else if (etherType == ETHERTYPE_ARP && (header.len >= ethHdrSize + sizeof(struct f_arphdr))) { + VIR_WARNINGS_NO_CAST_ALIGN struct f_arphdr *arphdr = (struct f_arphdr*)(packet + ethHdrSize); + VIR_WARNINGS_RESET switch (ntohs(arphdr->arphdr.ar_op)) { case ARPOP_REPLY: vmaddr = arphdr->ar_sip; @@ -535,14 +541,18 @@ learnIPAddressThread(void *arg) if (etherType == ETHERTYPE_IP && (header.len >= ethHdrSize + sizeof(struct iphdr))) { + VIR_WARNINGS_NO_CAST_ALIGN struct iphdr *iphdr = (struct iphdr*)(packet + ethHdrSize); + VIR_WARNINGS_RESET if ((iphdr->protocol == IPPROTO_UDP) && (header.len >= ethHdrSize + iphdr->ihl * 4 + sizeof(struct udphdr))) { + VIR_WARNINGS_NO_CAST_ALIGN struct udphdr *udphdr= (struct udphdr *) ((char *)iphdr + iphdr->ihl * 4); + VIR_WARNINGS_RESET if (ntohs(udphdr->source) == 67 && ntohs(udphdr->dest) == 68 && header.len >= ethHdrSize + diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index b12b81f..6049063 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -243,8 +243,10 @@ virStorageBackendCreateVols(virStoragePoolObjPtr pool, /* Given the way libdevmapper returns its data, I don't see * any way to avoid this series of casts. */ + VIR_WARNINGS_NO_CAST_ALIGN next = names->next; names = (struct dm_names *)(((char *)names) + next); + VIR_WARNINGS_RESET } while (next); -- 1.8.1.4
On 04/03/2013 09:06 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
There are a number of places which generate cast alignment warnings, which are difficult or impossible to address. Use pragmas to disable the warnings in these few places
Sheesh - this is some hairy code.
conf/nwfilter_conf.c: In function 'virNWFilterRuleDetailsParse': conf/nwfilter_conf.c:1806:16: warning: cast increases required alignment of target type [-Wcast-align] item = (nwItemDesc *)((char *)nwf + att[idx].dataIdx);
nwf is type virNWFilterRuleDefPtr, and att[idx].dataIdx is populated to be the offset of a field within virNWFilterRuleDef. I looked for all instances of dataIdx initializations, and it loos like all of those fields are indeed typed nwItemDesc, so the cast is aligned, and I can agree to the use of a pragma instead of rewriting this.
conf/nwfilter_conf.c: In function 'virNWFilterRuleDefDetailsFormat': conf/nwfilter_conf.c:3238:16: warning: cast increases required alignment of target type [-Wcast-align] item = (nwItemDesc *)((char *)def + att[i].dataIdx);
Same analysis.
storage/storage_backend_mpath.c: In function 'virStorageBackendCreateVols': storage/storage_backend_mpath.c:247:17: warning: cast increases required alignment of target type [-Wcast-align] names = (struct dm_names *)(((char *)names) + next);
names is strcut dm_names*, and libdevmapper (stupidly) set up next to be a byte offset instead of a struct offset. Serves them right if they botched alignment, but I agree that we are stuck here using the pragma.
nwfilter/nwfilter_dhcpsnoop.c: In function 'virNWFilterSnoopDHCPDecode': nwfilter/nwfilter_dhcpsnoop.c:994:15: warning: cast increases required alignment of target type [-Wcast-align] pip = (struct iphdr *) pep->eh_data;
pep is virNWFilterSnoopEthHdrPtr, which is ATTRIBUTE_PACKED. eh_data within that struct is therefore aligned only to uint16_t boundaries. 'struct iphdr' in <netinet/ip.h> requires uint32_t alignment. This is a real case of misalignment. I'd rather see us work around this with memcpy() than risk SIGBUS if we read a 32-bit entity that crosses a word boundary due to 16-bit alignment.
nwfilter/nwfilter_dhcpsnoop.c:1004:11: warning: cast increases required alignment of target type [-Wcast-align] pup = (struct udphdr *) ((char *) pip + (pip->ihl << 2));
pip is struct iphdr *, which I already said was 32-bit aligned; and the offset (pip->ihl << 2) is guaranteed to be a multiple of 32-bit alignment. Furthermore, struct udphdr appears to be only 16-bit aligned (the warning about alignment increase is due to the intermediate use of 8-bit char*). This one is safe for use of the pragma.
nwfilter/nwfilter_learnipaddr.c: In function 'procDHCPOpts': nwfilter/nwfilter_learnipaddr.c:327:33: warning: cast increases required alignment of target type [-Wcast-align] uint32_t *tmp = (uint32_t *)&dhcpopt->value;
dhcpopt is declared as struct dhcp_option, which is only 8-bit aligned (it has a pointless ATTRIBUTE_PACKED, given that none of its fields are larger than char). But dhcpopt is created as uint16_t away from the argument dhcp; and the only caller is at line 562, where dhcp was computed based on a udphdr. Sounds like it is an alignment problem after all, and I'd rather see memcpy() used to avoid the possibility of a SIGBUS.
nwfilter/nwfilter_learnipaddr.c: In function 'learnIPAddressThread': nwfilter/nwfilter_learnipaddr.c:501:43: warning: cast increases required alignment of target type [-Wcast-align] struct iphdr *iphdr = (struct iphdr*)(packet +
packet is merely a byte array with no alignment (in reality, it's probably at least uint16_t aligned, and maybe we get lucky with uint32_t alignment; but I don't know that pcap_next() guarantees that). Here,...
nwfilter/nwfilter_learnipaddr.c:538:43: warning: cast increases required alignment of target type [-Wcast-align] struct iphdr *iphdr = (struct iphdr*)(packet + nwfilter/nwfilter_learnipaddr.c:544:48: warning: cast increases required alignment of target type [-Wcast-align] struct udphdr *udphdr= (struct udphdr *)
...and in these two as well, we are progressively looking further into that raw array of packet, and trying to parse out possibly unaligned data. I have no idea how we could possibly guarantee alignment, so the pragma may be the best we can do without writing the code to access unaligned offsets a byte at a time instead of as a struct field dereference. Let's face it - trying to parse raw packets off a NIC is painful when you can't use unaligned access. Ultimately, I think this file would be better if we used accessors from virendian.h (well, we'd have to add virReadBufInt16BE/virReadBufInt16LE) to read integers from byte[]+offset, (the virendian.h macros are safe regardless of alignment) instead of trying to cast through a packed type and risking problems. But doing a rewrite like that is a much bigger task; so what you have at least shuts the compiler up, even if it doesn't fix the issues at hand.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- m4/virt-compile-warnings.m4 | 1 + src/conf/nwfilter_conf.c | 4 ++++ src/internal.h | 13 +++++++++++++ src/nwfilter/nwfilter_dhcpsnoop.c | 4 ++++ src/nwfilter/nwfilter_learnipaddr.c | 10 ++++++++++ src/storage/storage_backend_mpath.c | 2 ++ 6 files changed, 34 insertions(+)
diff --git a/m4/virt-compile-warnings.m4 b/m4/virt-compile-warnings.m4 index a08fcac..e054913 100644 --- a/m4/virt-compile-warnings.m4 +++ b/m4/virt-compile-warnings.m4 @@ -93,6 +93,7 @@ AC_DEFUN([LIBVIRT_COMPILE_WARNINGS],[ if test $lv_cv_gcc_pragma_push_works = no; then dontwarn="$dontwarn -Wmissing-prototypes" dontwarn="$dontwarn -Wmissing-declarations" + dontwarn="$dontwarn -Wcast-align" fi
Good.
dnl Check whether strchr(s, char variable) causes a bogus compile diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index e63a04b..b61010d 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -1803,7 +1803,9 @@ virNWFilterRuleDetailsParse(xmlNodePtr node, while (att[idx].name != NULL) { prop = virXMLPropString(node, att[idx].name);
+ VIR_WARNINGS_NO_CAST_ALIGN item = (nwItemDesc *)((char *)nwf + att[idx].dataIdx); + VIR_WARNINGS_RESET
Good (see my comments above).
flags = &item->flags; flags_set = match_flag;
@@ -3235,7 +3237,9 @@ virNWFilterRuleDefDetailsFormat(virBufferPtr buf, nwItemDesc *item;
while (att[i].name) { + VIR_WARNINGS_NO_CAST_ALIGN item = (nwItemDesc *)((char *)def + att[i].dataIdx); + VIR_WARNINGS_RESET
Good.
enum virNWFilterEntryItemFlags flags = item->flags; if ((flags & NWFILTER_ENTRY_ITEM_FLAG_EXISTS)) { if (!typeShown) { diff --git a/src/internal.h b/src/internal.h index 2cf4731..d819aa3 100644 --- a/src/internal.h +++ b/src/internal.h @@ -214,6 +214,19 @@ # endif # endif /* __GNUC__ */
+ +# if __GNUC_PREREQ (4, 6) +# define VIR_WARNINGS_NO_CAST_ALIGN \ + _Pragma ("GCC diagnostic push") \ + _Pragma ("GCC diagnostic ignored \"-Wcast-align\"") + +# define VIR_WARNINGS_RESET \ + _Pragma ("GCC diagnostic pop") +# else +# define VIR_WARNINGS_NO_CAST_ALIGN +# define VIR_WARNINGS_RESET +# endif +
Good.
/* * Use this when passing possibly-NULL strings to printf-a-likes. */ diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index 5124069..5012b14 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -991,7 +991,9 @@ virNWFilterSnoopDHCPDecode(virNWFilterSnoopReqPtr req, /* go through the protocol headers */ switch (ntohs(pep->eh_type)) { case ETHERTYPE_IP: + VIR_WARNINGS_NO_CAST_ALIGN; pip = (struct iphdr *) pep->eh_data; + VIR_WARNINGS_RESET;
Tolerable, although I'd prefer a memcpy or virendian.h solution for whatever we end up reading out of pip.
len -= offsetof(virNWFilterSnoopEthHdr, eh_data); break; default: @@ -1001,7 +1003,9 @@ virNWFilterSnoopDHCPDecode(virNWFilterSnoopReqPtr req, if (len < 0) return -2;
+ VIR_WARNINGS_NO_CAST_ALIGN pup = (struct udphdr *) ((char *) pip + (pip->ihl << 2)); + VIR_WARNINGS_RESET
Good.
len -= pip->ihl << 2; if (len < 0) return -2; diff --git a/src/nwfilter/nwfilter_learnipaddr.c b/src/nwfilter/nwfilter_learnipaddr.c index 7a4f983..b399092 100644 --- a/src/nwfilter/nwfilter_learnipaddr.c +++ b/src/nwfilter/nwfilter_learnipaddr.c @@ -324,7 +324,9 @@ procDHCPOpts(struct dhcp *dhcp, int dhcp_opts_len,
case DHCP_OPT_BCASTADDRESS: /* Broadcast address */ if (dhcp_opts_len >= 6) { + VIR_WARNINGS_NO_CAST_ALIGN uint32_t *tmp = (uint32_t *)&dhcpopt->value; + VIR_WARNINGS_RESET
Tolerable.
(*bcastaddr) = ntohl(*tmp); } break; @@ -498,8 +500,10 @@ learnIPAddressThread(void *arg) if (etherType == ETHERTYPE_IP && (header.len >= ethHdrSize + sizeof(struct iphdr))) { + VIR_WARNINGS_NO_CAST_ALIGN struct iphdr *iphdr = (struct iphdr*)(packet + ethHdrSize); + VIR_WARNINGS_RESET
Tolerable.
vmaddr = iphdr->saddr; /* skip mcast addresses (224.0.0.0 - 239.255.255.255), * class E (240.0.0.0 - 255.255.255.255, includes eth. @@ -514,8 +518,10 @@ learnIPAddressThread(void *arg) } else if (etherType == ETHERTYPE_ARP && (header.len >= ethHdrSize + sizeof(struct f_arphdr))) { + VIR_WARNINGS_NO_CAST_ALIGN struct f_arphdr *arphdr = (struct f_arphdr*)(packet + ethHdrSize); + VIR_WARNINGS_RESET
Tolerable.
switch (ntohs(arphdr->arphdr.ar_op)) { case ARPOP_REPLY: vmaddr = arphdr->ar_sip; @@ -535,14 +541,18 @@ learnIPAddressThread(void *arg) if (etherType == ETHERTYPE_IP && (header.len >= ethHdrSize + sizeof(struct iphdr))) { + VIR_WARNINGS_NO_CAST_ALIGN struct iphdr *iphdr = (struct iphdr*)(packet + ethHdrSize); + VIR_WARNINGS_RESET
Tolerable.
if ((iphdr->protocol == IPPROTO_UDP) && (header.len >= ethHdrSize + iphdr->ihl * 4 + sizeof(struct udphdr))) { + VIR_WARNINGS_NO_CAST_ALIGN struct udphdr *udphdr= (struct udphdr *) ((char *)iphdr + iphdr->ihl * 4); + VIR_WARNINGS_RESET
Tolerable.
if (ntohs(udphdr->source) == 67 && ntohs(udphdr->dest) == 68 && header.len >= ethHdrSize + diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index b12b81f..6049063 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -243,8 +243,10 @@ virStorageBackendCreateVols(virStoragePoolObjPtr pool,
/* Given the way libdevmapper returns its data, I don't see * any way to avoid this series of casts. */ + VIR_WARNINGS_NO_CAST_ALIGN next = names->next; names = (struct dm_names *)(((char *)names) + next); + VIR_WARNINGS_RESET
No choice in the matter; good. ACK. Although I wouldn't mind if you attempt a v2 to address some of the more questionable locations, I also won't insist on it right now. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> Certain functions in the sysinfotest.c are not used unless a whitelisted architecture is being built. Disable those functions unless required to avoid warnings about unused functions. sysinfotest.c:93:1: warning: 'sysinfotest_run' defined but not used [-Wunused-function] sysinfotest_run(const char *test, Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/sysinfotest.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/sysinfotest.c b/tests/sysinfotest.c index aee57e6..94493a3 100644 --- a/tests/sysinfotest.c +++ b/tests/sysinfotest.c @@ -38,6 +38,10 @@ #if defined (__linux__) +# if defined(__s390__) || defined(__s390x__) || \ + defined(__powerpc__) || defined(__powerpc64__) || \ + defined(__i386__) || defined(__x86_64__) || defined(__amd64__) + /* from sysinfo.c */ void virSysinfoSetup(const char *decoder, const char *sysinfo, @@ -122,6 +126,7 @@ error: VIR_FREE(testdata.expected); return ret; } +# endif # if defined(__s390__) || defined(__s390x__) static int -- 1.8.1.4
On 03.04.2013 17:06, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Certain functions in the sysinfotest.c are not used unless a whitelisted architecture is being built. Disable those functions unless required to avoid warnings about unused functions.
sysinfotest.c:93:1: warning: 'sysinfotest_run' defined but not used [-Wunused-function] sysinfotest_run(const char *test,
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/sysinfotest.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/tests/sysinfotest.c b/tests/sysinfotest.c index aee57e6..94493a3 100644 --- a/tests/sysinfotest.c +++ b/tests/sysinfotest.c @@ -38,6 +38,10 @@
#if defined (__linux__)
+# if defined(__s390__) || defined(__s390x__) || \ + defined(__powerpc__) || defined(__powerpc64__) || \ + defined(__i386__) || defined(__x86_64__) || defined(__amd64__) + /* from sysinfo.c */ void virSysinfoSetup(const char *decoder, const char *sysinfo, @@ -122,6 +126,7 @@ error: VIR_FREE(testdata.expected); return ret; } +# endif
# if defined(__s390__) || defined(__s390x__) static int
Seems like you (accidentally?) pushed this one already. Michal
On 04/03/2013 09:06 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Certain functions in the sysinfotest.c are not used unless a whitelisted architecture is being built. Disable those functions unless required to avoid warnings about unused functions.
sysinfotest.c:93:1: warning: 'sysinfotest_run' defined but not used [-Wunused-function] sysinfotest_run(const char *test,
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/sysinfotest.c | 5 +++++ 1 file changed, 5 insertions(+)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> To avoid virportallocatortest.c: In function 'bind': virportallocatortest.c:34:33: warning: cast increases required alignment of target type [-Wcast-align] struct sockaddr_in *saddr = (struct sockaddr_in *)addr; ^ Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/virportallocatortest.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/virportallocatortest.c b/tests/virportallocatortest.c index 9931e11..1935602 100644 --- a/tests/virportallocatortest.c +++ b/tests/virportallocatortest.c @@ -31,12 +31,14 @@ int bind(int sockfd ATTRIBUTE_UNUSED, const struct sockaddr *addr, socklen_t addrlen ATTRIBUTE_UNUSED) { - struct sockaddr_in *saddr = (struct sockaddr_in *)addr; + struct sockaddr_in saddr; - if (saddr->sin_port == htons(5900) || - saddr->sin_port == htons(5904) || - saddr->sin_port == htons(5905) || - saddr->sin_port == htons(5906)) { + memcpy(&saddr, addr, sizeof(saddr)); + + if (saddr.sin_port == htons(5900) || + saddr.sin_port == htons(5904) || + saddr.sin_port == htons(5905) || + saddr.sin_port == htons(5906)) { errno = EADDRINUSE; return -1; } -- 1.8.1.4
On 04/03/2013 09:06 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
To avoid
virportallocatortest.c: In function 'bind': virportallocatortest.c:34:33: warning: cast increases required alignment of target type [-Wcast-align] struct sockaddr_in *saddr = (struct sockaddr_in *)addr; ^
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tests/virportallocatortest.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/tests/virportallocatortest.c b/tests/virportallocatortest.c index 9931e11..1935602 100644 --- a/tests/virportallocatortest.c +++ b/tests/virportallocatortest.c @@ -31,12 +31,14 @@ int bind(int sockfd ATTRIBUTE_UNUSED, const struct sockaddr *addr, socklen_t addrlen ATTRIBUTE_UNUSED) { - struct sockaddr_in *saddr = (struct sockaddr_in *)addr; + struct sockaddr_in saddr;
Hmm. POSIX requires that apps use sockaddr_storage to guarantee alignment before downcasting to struct sockaddr_in or further down to struct sockaddr. But since we are providing a replacement bind(), and POSIX doesn't give us any way to specify that 'struct sockaddr' in our signature should have been aligned as struct sockaddr_storage by any sane caller, we're stuck playing realignment games, or else using a pragma.
- if (saddr->sin_port == htons(5900) || - saddr->sin_port == htons(5904) || - saddr->sin_port == htons(5905) || - saddr->sin_port == htons(5906)) { + memcpy(&saddr, addr, sizeof(saddr));
Since this is just test code, performance doesn't matter, and using memcpy() for the realignment game makes sense. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03.04.2013 17:06, Daniel P. Berrange wrote:
The ARMv7 builds of libvirt generate a number of warnings, mostly about cast alignment
http://arm.koji.fedoraproject.org/packages/libvirt/1.0.4/1.fc19/data/logs/ar...
this patch series fixes as many as possible, and then disables warnings for the rest using a pragma
ACK series - passes my compile test on my ARMv6 based machine. I've prepared something similar a while ago but never get to send it actually. Michal
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Michal Privoznik