[libvirt] [PATCH 0/3] add virendian.h, and use it to kill xen cpumap issue
Not only does this solve the compiler warning on 32-bit machines, but it completely gets rid of what looks like bogus pointer aliasing, all while reducing the number of lines in xen_hypervisor.c. Eric Blake (3): util: add virendian.h macros util: use new virendian.h macros xen: clean up the mess with cpumap .gitignore | 1 + src/Makefile.am | 1 + src/cpu/cpu_x86.c | 24 ++++------ src/util/virendian.h | 93 +++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.c | 109 ++++++++++------------------------------------ src/xen/xen_hypervisor.c | 16 +++---- tests/Makefile.am | 8 +++- tests/virendiantest.c | 102 +++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 239 insertions(+), 115 deletions(-) create mode 100644 src/util/virendian.h create mode 100644 tests/virendiantest.c -- 1.8.1.2
We have several cases where we need to read endian-dependent data regardless of host endianness; rather than open-coding these call sites, it will be nicer to funnel things through a macro. The virendian.h file can be expanded to add writer functions, and/or 16-bit access patterns, if needed. Also, if we need to turn things into a function to avoid multiple evaluations of buf, that can be done later. But for now, a macro worked. * src/util/virendian.h: New file. * src/Makefile.am (UTIL_SOURCES): Ship it. * tests/virendiantest.c: New test. * tests/Makefile.am (test_programs, virendiantest_SOURCES): Run the test. * .gitignore: Ignore built file. --- .gitignore | 1 + src/Makefile.am | 1 + src/util/virendian.h | 93 +++++++++++++++++++++++++++++++++++++++++++++ tests/Makefile.am | 8 +++- tests/virendiantest.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 203 insertions(+), 2 deletions(-) create mode 100644 src/util/virendian.h create mode 100644 tests/virendiantest.c diff --git a/.gitignore b/.gitignore index 1670637..8afbf33 100644 --- a/.gitignore +++ b/.gitignore @@ -177,6 +177,7 @@ /tests/virbitmaptest /tests/virbuftest /tests/virdrivermoduletest +/tests/virendiantest /tests/virhashtest /tests/virkeyfiletest /tests/virlockspacetest diff --git a/src/Makefile.am b/src/Makefile.am index f6162df..d554aa1 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -67,6 +67,7 @@ UTIL_SOURCES = \ util/virdbus.c util/virdbus.h \ util/virdnsmasq.c util/virdnsmasq.h \ util/virebtables.c util/virebtables.h \ + util/virendian.h \ util/virerror.c util/virerror.h \ util/virevent.c util/virevent.h \ util/vireventpoll.c util/vireventpoll.h \ diff --git a/src/util/virendian.h b/src/util/virendian.h new file mode 100644 index 0000000..eefe48c --- /dev/null +++ b/src/util/virendian.h @@ -0,0 +1,93 @@ +/* + * virendian.h: aid for reading endian-specific data + * + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#ifndef __VIR_ENDIAN_H__ +# define __VIR_ENDIAN_H__ + +# include "internal.h" + +/* The interfaces in this file are provided as macros for speed. */ + +/** + * virReadBufInt64BE: + * @buf: byte to start reading at (can be 'char*' or 'unsigned char*'); + * evaluating buf must not have any side effects + * + * Read 8 bytes at BUF as a big-endian 64-bit number. Caller is + * responsible to avoid reading beyond array bounds. + */ +# define virReadBufInt64BE(buf) \ + (((uint64_t)(uint8_t)((buf)[0]) << 56) | \ + ((uint64_t)(uint8_t)((buf)[1]) << 48) | \ + ((uint64_t)(uint8_t)((buf)[2]) << 40) | \ + ((uint64_t)(uint8_t)((buf)[3]) << 32) | \ + ((uint64_t)(uint8_t)((buf)[4]) << 24) | \ + ((uint64_t)(uint8_t)((buf)[5]) << 16) | \ + ((uint64_t)(uint8_t)((buf)[6]) << 8) | \ + (uint64_t)(uint8_t)((buf)[7])) + +/** + * virReadBufInt64LE: + * @buf: byte to start reading at (can be 'char*' or 'unsigned char*'); + * evaluating buf must not have any side effects + * + * Read 8 bytes at BUF as a little-endian 64-bit number. Caller is + * responsible to avoid reading beyond array bounds. + */ +# define virReadBufInt64LE(buf) \ + ((uint64_t)(uint8_t)((buf)[0]) | \ + ((uint64_t)(uint8_t)((buf)[1]) << 8) | \ + ((uint64_t)(uint8_t)((buf)[2]) << 16) | \ + ((uint64_t)(uint8_t)((buf)[3]) << 24) | \ + ((uint64_t)(uint8_t)((buf)[4]) << 32) | \ + ((uint64_t)(uint8_t)((buf)[5]) << 40) | \ + ((uint64_t)(uint8_t)((buf)[6]) << 48) | \ + ((uint64_t)(uint8_t)((buf)[7]) << 56)) + +/** + * virReadBufInt32BE: + * @buf: byte to start reading at (can be 'char*' or 'unsigned char*'); + * evaluating buf must not have any side effects + * + * Read 4 bytes at BUF as a big-endian 32-bit number. Caller is + * responsible to avoid reading beyond array bounds. + */ +# define virReadBufInt32BE(buf) \ + (((uint32_t)(uint8_t)((buf)[0]) << 24) | \ + ((uint32_t)(uint8_t)((buf)[1]) << 16) | \ + ((uint32_t)(uint8_t)((buf)[2]) << 8) | \ + (uint32_t)(uint8_t)((buf)[3])) + +/** + * virReadBufInt32LE: + * @buf: byte to start reading at (can be 'char*' or 'unsigned char*'); + * evaluating buf must not have any side effects + * + * Read 4 bytes at BUF as a little-endian 32-bit number. Caller is + * responsible to avoid reading beyond array bounds. + */ +# define virReadBufInt32LE(buf) \ + ((uint32_t)(uint8_t)((buf)[0]) | \ + ((uint32_t)(uint8_t)((buf)[1]) << 8) | \ + ((uint32_t)(uint8_t)((buf)[2]) << 16) | \ + ((uint32_t)(uint8_t)((buf)[3]) << 24)) + +#endif /* __VIR_ENDIAN_H__ */ diff --git a/tests/Makefile.am b/tests/Makefile.am index 0194db2..7d0a88e 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1,6 +1,6 @@ ## Process this file with automake to produce Makefile.in -## Copyright (C) 2005-2012 Red Hat, Inc. +## Copyright (C) 2005-2013 Red Hat, Inc. ## See COPYING.LIB for the License of this software SHELL = $(PREFERABLY_POSIX_SHELL) @@ -95,7 +95,7 @@ test_programs = virshtest sockettest \ utiltest shunloadtest \ virtimetest viruritest virkeyfiletest \ virauthconfigtest \ - virbitmaptest \ + virbitmaptest virendiantest \ virlockspacetest \ virstringtest \ virportallocatortest \ @@ -649,6 +649,10 @@ virbitmaptest_SOURCES = \ virbitmaptest.c testutils.h testutils.c virbitmaptest_LDADD = $(LDADDS) +virendiantest_SOURCES = \ + virendiantest.c testutils.h testutils.c +virendiantest_LDADD = $(LDADDS) + jsontest_SOURCES = \ jsontest.c testutils.h testutils.c jsontest_LDADD = $(LDADDS) diff --git a/tests/virendiantest.c b/tests/virendiantest.c new file mode 100644 index 0000000..386ef33 --- /dev/null +++ b/tests/virendiantest.c @@ -0,0 +1,102 @@ +/* + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> + +#include "testutils.h" + +#include "virendian.h" + +static int +test1(const void *data ATTRIBUTE_UNUSED) +{ + /* Regular char should work, even if signed, and even with + * unaligned access. */ + char array[] = { 1, 2, 3, 4, 5, 6, 7, 8, + 0x89, 0x8a, 0x8b, 0x8c, 0x8d }; + int ret = -1; + + if (virReadBufInt64BE(array) != 0x0102030405060708ULL) + goto cleanup; + if (virReadBufInt64BE(array + 5) != 0x060708898a8b8c8dULL) + goto cleanup; + if (virReadBufInt64LE(array) != 0x0807060504030201ULL) + goto cleanup; + if (virReadBufInt64LE(array + 5) != 0x8d8c8b8a89080706ULL) + goto cleanup; + + if (virReadBufInt32BE(array) != 0x01020304U) + goto cleanup; + if (virReadBufInt32BE(array + 8) != 0x898a8b8cU) + goto cleanup; + if (virReadBufInt32LE(array) != 0x04030201U) + goto cleanup; + if (virReadBufInt32LE(array + 8) != 0x8c8b8a89U) + goto cleanup; + + ret = 0; +cleanup: + return ret; +} + +static int +test2(const void *data ATTRIBUTE_UNUSED) +{ + /* Unsigned char should work without cast, even if unaligned access. */ + unsigned char array[] = { 1, 2, 3, 4, 5, 6, 7, 8, + 0x89, 0x8a, 0x8b, 0x8c, 0x8d }; + int ret = -1; + + if (virReadBufInt64BE(array) != 0x0102030405060708ULL) + goto cleanup; + if (virReadBufInt64BE(array + 5) != 0x060708898a8b8c8dULL) + goto cleanup; + if (virReadBufInt64LE(array) != 0x0807060504030201ULL) + goto cleanup; + if (virReadBufInt64LE(array + 5) != 0x8d8c8b8a89080706ULL) + goto cleanup; + + if (virReadBufInt32BE(array) != 0x01020304U) + goto cleanup; + if (virReadBufInt32BE(array + 8) != 0x898a8b8cU) + goto cleanup; + if (virReadBufInt32LE(array) != 0x04030201U) + goto cleanup; + if (virReadBufInt32LE(array + 8) != 0x8c8b8a89U) + goto cleanup; + + ret = 0; +cleanup: + return ret; +} + +static int +mymain(void) +{ + int ret = 0; + + if (virtTestRun("test1", 1, test1, NULL) < 0) + ret = -1; + if (virtTestRun("test2", 1, test2, NULL) < 0) + ret = -1; + + return ret; +} + +VIRT_TEST_MAIN(mymain) -- 1.8.1.2
On 02/11/2013 07:07 PM, Eric Blake wrote:
We have several cases where we need to read endian-dependent data regardless of host endianness; rather than open-coding these call sites, it will be nicer to funnel things through a macro.
The virendian.h file can be expanded to add writer functions, and/or 16-bit access patterns, if needed. Also, if we need to turn things into a function to avoid multiple evaluations of buf, that can be done later. But for now, a macro worked.
* src/util/virendian.h: New file. * src/Makefile.am (UTIL_SOURCES): Ship it. * tests/virendiantest.c: New test. * tests/Makefile.am (test_programs, virendiantest_SOURCES): Run the test. * .gitignore: Ignore built file. --- .gitignore | 1 + src/Makefile.am | 1 + src/util/virendian.h | 93 +++++++++++++++++++++++++++++++++++++++++++++ tests/Makefile.am | 8 +++- tests/virendiantest.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 203 insertions(+), 2 deletions(-) create mode 100644 src/util/virendian.h create mode 100644 tests/virendiantest.c
diff --git a/.gitignore b/.gitignore index 1670637..8afbf33 100644 --- a/.gitignore +++ b/.gitignore @@ -177,6 +177,7 @@ /tests/virbitmaptest /tests/virbuftest /tests/virdrivermoduletest +/tests/virendiantest /tests/virhashtest /tests/virkeyfiletest /tests/virlockspacetest diff --git a/src/Makefile.am b/src/Makefile.am index f6162df..d554aa1 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -67,6 +67,7 @@ UTIL_SOURCES = \ util/virdbus.c util/virdbus.h \ util/virdnsmasq.c util/virdnsmasq.h \ util/virebtables.c util/virebtables.h \ + util/virendian.h \ util/virerror.c util/virerror.h \ util/virevent.c util/virevent.h \ util/vireventpoll.c util/vireventpoll.h \ diff --git a/src/util/virendian.h b/src/util/virendian.h new file mode 100644 index 0000000..eefe48c --- /dev/null +++ b/src/util/virendian.h @@ -0,0 +1,93 @@ +/* + * virendian.h: aid for reading endian-specific data + * + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#ifndef __VIR_ENDIAN_H__ +# define __VIR_ENDIAN_H__ + +# include "internal.h" + +/* The interfaces in this file are provided as macros for speed. */ + +/** + * virReadBufInt64BE: + * @buf: byte to start reading at (can be 'char*' or 'unsigned char*'); + * evaluating buf must not have any side effects + * + * Read 8 bytes at BUF as a big-endian 64-bit number. Caller is + * responsible to avoid reading beyond array bounds. + */ +# define virReadBufInt64BE(buf) \ + (((uint64_t)(uint8_t)((buf)[0]) << 56) | \ + ((uint64_t)(uint8_t)((buf)[1]) << 48) | \ + ((uint64_t)(uint8_t)((buf)[2]) << 40) | \ + ((uint64_t)(uint8_t)((buf)[3]) << 32) | \ + ((uint64_t)(uint8_t)((buf)[4]) << 24) | \ + ((uint64_t)(uint8_t)((buf)[5]) << 16) | \ + ((uint64_t)(uint8_t)((buf)[6]) << 8) | \ + (uint64_t)(uint8_t)((buf)[7])) + +/** + * virReadBufInt64LE: + * @buf: byte to start reading at (can be 'char*' or 'unsigned char*'); + * evaluating buf must not have any side effects + * + * Read 8 bytes at BUF as a little-endian 64-bit number. Caller is + * responsible to avoid reading beyond array bounds. + */ +# define virReadBufInt64LE(buf) \ + ((uint64_t)(uint8_t)((buf)[0]) | \ + ((uint64_t)(uint8_t)((buf)[1]) << 8) | \ + ((uint64_t)(uint8_t)((buf)[2]) << 16) | \ + ((uint64_t)(uint8_t)((buf)[3]) << 24) | \ + ((uint64_t)(uint8_t)((buf)[4]) << 32) | \ + ((uint64_t)(uint8_t)((buf)[5]) << 40) | \ + ((uint64_t)(uint8_t)((buf)[6]) << 48) | \ + ((uint64_t)(uint8_t)((buf)[7]) << 56)) + +/** + * virReadBufInt32BE: + * @buf: byte to start reading at (can be 'char*' or 'unsigned char*'); + * evaluating buf must not have any side effects + * + * Read 4 bytes at BUF as a big-endian 32-bit number. Caller is + * responsible to avoid reading beyond array bounds. + */ +# define virReadBufInt32BE(buf) \ + (((uint32_t)(uint8_t)((buf)[0]) << 24) | \ + ((uint32_t)(uint8_t)((buf)[1]) << 16) | \ + ((uint32_t)(uint8_t)((buf)[2]) << 8) | \ + (uint32_t)(uint8_t)((buf)[3])) + +/** + * virReadBufInt32LE: + * @buf: byte to start reading at (can be 'char*' or 'unsigned char*'); + * evaluating buf must not have any side effects + * + * Read 4 bytes at BUF as a little-endian 32-bit number. Caller is + * responsible to avoid reading beyond array bounds. + */ +# define virReadBufInt32LE(buf) \ + ((uint32_t)(uint8_t)((buf)[0]) | \ + ((uint32_t)(uint8_t)((buf)[1]) << 8) | \ + ((uint32_t)(uint8_t)((buf)[2]) << 16) | \ + ((uint32_t)(uint8_t)((buf)[3]) << 24)) + +#endif /* __VIR_ENDIAN_H__ */
Looking at byteswap.h.in I'm reminded of what I've seen in a previous job where we had lots of byteswapping to do. The macros will "isolate" each byte to move, eg: #define bswap_32(x) ((((x) & 0x000000FF) << 24) | \ (((x) & 0x0000FF00) << 8) | \ (((x) & 0x00FF0000) >> 8) | \ (((x) & 0xFF000000) >> 24)) Not sure if it's necessary in this case, but figured I'd ask.
diff --git a/tests/Makefile.am b/tests/Makefile.am index 0194db2..7d0a88e 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1,6 +1,6 @@ ## Process this file with automake to produce Makefile.in
-## Copyright (C) 2005-2012 Red Hat, Inc. +## Copyright (C) 2005-2013 Red Hat, Inc. ## See COPYING.LIB for the License of this software
SHELL = $(PREFERABLY_POSIX_SHELL) @@ -95,7 +95,7 @@ test_programs = virshtest sockettest \ utiltest shunloadtest \ virtimetest viruritest virkeyfiletest \ virauthconfigtest \ - virbitmaptest \ + virbitmaptest virendiantest \ virlockspacetest \ virstringtest \ virportallocatortest \ @@ -649,6 +649,10 @@ virbitmaptest_SOURCES = \ virbitmaptest.c testutils.h testutils.c virbitmaptest_LDADD = $(LDADDS)
+virendiantest_SOURCES = \ + virendiantest.c testutils.h testutils.c +virendiantest_LDADD = $(LDADDS) + jsontest_SOURCES = \ jsontest.c testutils.h testutils.c jsontest_LDADD = $(LDADDS) diff --git a/tests/virendiantest.c b/tests/virendiantest.c new file mode 100644 index 0000000..386ef33 --- /dev/null +++ b/tests/virendiantest.c @@ -0,0 +1,102 @@ +/* + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> + +#include "testutils.h" + +#include "virendian.h" + +static int +test1(const void *data ATTRIBUTE_UNUSED) +{ + /* Regular char should work, even if signed, and even with + * unaligned access. */ + char array[] = { 1, 2, 3, 4, 5, 6, 7, 8, + 0x89, 0x8a, 0x8b, 0x8c, 0x8d }; + int ret = -1; + + if (virReadBufInt64BE(array) != 0x0102030405060708ULL) + goto cleanup; + if (virReadBufInt64BE(array + 5) != 0x060708898a8b8c8dULL) + goto cleanup; + if (virReadBufInt64LE(array) != 0x0807060504030201ULL) + goto cleanup; + if (virReadBufInt64LE(array + 5) != 0x8d8c8b8a89080706ULL) + goto cleanup; + + if (virReadBufInt32BE(array) != 0x01020304U) + goto cleanup; + if (virReadBufInt32BE(array + 8) != 0x898a8b8cU) + goto cleanup; + if (virReadBufInt32LE(array) != 0x04030201U) + goto cleanup; + if (virReadBufInt32LE(array + 8) != 0x8c8b8a89U) + goto cleanup; + + ret = 0; +cleanup: + return ret; +} + +static int +test2(const void *data ATTRIBUTE_UNUSED) +{ + /* Unsigned char should work without cast, even if unaligned access. */ + unsigned char array[] = { 1, 2, 3, 4, 5, 6, 7, 8, + 0x89, 0x8a, 0x8b, 0x8c, 0x8d }; + int ret = -1; + + if (virReadBufInt64BE(array) != 0x0102030405060708ULL) + goto cleanup; + if (virReadBufInt64BE(array + 5) != 0x060708898a8b8c8dULL) + goto cleanup; + if (virReadBufInt64LE(array) != 0x0807060504030201ULL) + goto cleanup; + if (virReadBufInt64LE(array + 5) != 0x8d8c8b8a89080706ULL) + goto cleanup; + + if (virReadBufInt32BE(array) != 0x01020304U) + goto cleanup; + if (virReadBufInt32BE(array + 8) != 0x898a8b8cU) + goto cleanup; + if (virReadBufInt32LE(array) != 0x04030201U) + goto cleanup; + if (virReadBufInt32LE(array + 8) != 0x8c8b8a89U) + goto cleanup; + + ret = 0; +cleanup: + return ret; +} + +static int +mymain(void) +{ + int ret = 0; + + if (virtTestRun("test1", 1, test1, NULL) < 0) + ret = -1; + if (virtTestRun("test2", 1, test2, NULL) < 0) + ret = -1; + + return ret; +} + +VIRT_TEST_MAIN(mymain)
ACK
On 02/11/2013 05:56 PM, John Ferlan wrote:
On 02/11/2013 07:07 PM, Eric Blake wrote:
We have several cases where we need to read endian-dependent data regardless of host endianness; rather than open-coding these call sites, it will be nicer to funnel things through a macro.
The virendian.h file can be expanded to add writer functions, and/or 16-bit access patterns, if needed. Also, if we need to turn things into a function to avoid multiple evaluations of buf, that can be done later. But for now, a macro worked.
+# define virReadBufInt32LE(buf) \ + ((uint32_t)(uint8_t)((buf)[0]) | \ + ((uint32_t)(uint8_t)((buf)[1]) << 8) | \ + ((uint32_t)(uint8_t)((buf)[2]) << 16) | \ + ((uint32_t)(uint8_t)((buf)[3]) << 24)) + +#endif /* __VIR_ENDIAN_H__ */
Looking at byteswap.h.in I'm reminded of what I've seen in a previous job where we had lots of byteswapping to do. The macros will "isolate" each byte to move, eg:
#define bswap_32(x) ((((x) & 0x000000FF) << 24) | \ (((x) & 0x0000FF00) << 8) | \ (((x) & 0x00FF0000) >> 8) | \ (((x) & 0xFF000000) >> 24))
Not sure if it's necessary in this case, but figured I'd ask.
In the byteswap case, you are going from an int to an int; so you do have to mask out bytes on each part of the expression. But this function is going from a char[] to an int, so we are already starting with bytes. You will also notice that my code does a double cast: first to uint8_t (to guarantee that the byte is not sign-extended) then to uint{32,64}_t (to widen to the correct size of the resulting expression); the first cast is the same as masking with 0xff.
ACK
Thanks for reviewing; and I have now pushed the series. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This makes code easier to read, by avoiding lines longer than 80 columns and removing the repetition from the callers. * src/util/virstoragefile.c (qedGetHeaderUL, qedGetHeaderULL): Delete in favor of more generic macros. (qcow2GetBackingStoreFormat, qcowXGetBackingStore) (qedGetBackingStore, virStorageFileMatchesVersion) (virStorageFileGetMetadataInternal): Use new macros. * src/cpu/cpu_x86.c (x86VendorLoad): Likewise. --- src/cpu/cpu_x86.c | 24 ++++------ src/util/virstoragefile.c | 109 ++++++++++------------------------------------ 2 files changed, 30 insertions(+), 103 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index c9a833a..c750afd 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1,7 +1,7 @@ /* * cpu_x86.c: CPU driver for CPUs with x86 compatible CPUID instruction * - * Copyright (C) 2009-2011 Red Hat, Inc. + * Copyright (C) 2009-2011, 2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -32,6 +32,7 @@ #include "cpu_map.h" #include "cpu_x86.h" #include "virbuffer.h" +#include "virendian.h" #define VIR_FROM_THIS VIR_FROM_CPU @@ -548,22 +549,13 @@ x86VendorLoad(xmlXPathContextPtr ctxt, } vendor->cpuid.function = 0; - vendor->cpuid.ebx = (string[0]) | - (string[1] << 8) | - (string[2] << 16) | - (string[3] << 24); - vendor->cpuid.edx = (string[4]) | - (string[5] << 8) | - (string[6] << 16) | - (string[7] << 24); - vendor->cpuid.ecx = (string[8]) | - (string[9] << 8) | - (string[10] << 16) | - (string[11] << 24); - - if (!map->vendors) + vendor->cpuid.ebx = virReadBufInt32LE(string); + vendor->cpuid.edx = virReadBufInt32LE(string + 4); + vendor->cpuid.ecx = virReadBufInt32LE(string + 8); + + if (!map->vendors) { map->vendors = vendor; - else { + } else { vendor->next = map->vendors; map->vendors = vendor; } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 6e2d61e..dc28539 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -42,6 +42,7 @@ #include "c-ctype.h" #include "vircommand.h" #include "virhash.h" +#include "virendian.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -255,16 +256,8 @@ qcow2GetBackingStoreFormat(int *format, */ while (offset < (buf_size-8) && offset < (extension_end-8)) { - unsigned int magic = - (buf[offset] << 24) + - (buf[offset+1] << 16) + - (buf[offset+2] << 8) + - (buf[offset+3]); - unsigned int len = - (buf[offset+4] << 24) + - (buf[offset+5] << 16) + - (buf[offset+6] << 8) + - (buf[offset+7]); + unsigned int magic = virReadBufInt32BE(buf + offset); + unsigned int len = virReadBufInt32BE(buf + offset + 4); offset += 8; @@ -312,20 +305,10 @@ qcowXGetBackingStore(char **res, if (buf_size < QCOWX_HDR_BACKING_FILE_OFFSET+8+4) return BACKING_STORE_INVALID; - offset = (((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET] << 56) - | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+1] << 48) - | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+2] << 40) - | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+3] << 32) - | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+4] << 24) - | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+5] << 16) - | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+6] << 8) - | buf[QCOWX_HDR_BACKING_FILE_OFFSET+7]); /* QCowHeader.backing_file_offset */ + offset = virReadBufInt64BE(buf + QCOWX_HDR_BACKING_FILE_OFFSET); if (offset > buf_size) return BACKING_STORE_INVALID; - size = ((buf[QCOWX_HDR_BACKING_FILE_SIZE] << 24) - | (buf[QCOWX_HDR_BACKING_FILE_SIZE+1] << 16) - | (buf[QCOWX_HDR_BACKING_FILE_SIZE+2] << 8) - | buf[QCOWX_HDR_BACKING_FILE_SIZE+3]); /* QCowHeader.backing_file_size */ + size = virReadBufInt32BE(buf + QCOWX_HDR_BACKING_FILE_SIZE); if (size == 0) { if (format) *format = VIR_STORAGE_FILE_NONE; @@ -468,28 +451,6 @@ cleanup: return ret; } -static unsigned long -qedGetHeaderUL(const unsigned char *loc) -{ - return (((unsigned long)loc[3] << 24) | - ((unsigned long)loc[2] << 16) | - ((unsigned long)loc[1] << 8) | - ((unsigned long)loc[0] << 0)); -} - -static unsigned long long -qedGetHeaderULL(const unsigned char *loc) -{ - return (((unsigned long long)loc[7] << 56) | - ((unsigned long long)loc[6] << 48) | - ((unsigned long long)loc[5] << 40) | - ((unsigned long long)loc[4] << 32) | - ((unsigned long long)loc[3] << 24) | - ((unsigned long long)loc[2] << 16) | - ((unsigned long long)loc[1] << 8) | - ((unsigned long long)loc[0] << 0)); -} - static int qedGetBackingStore(char **res, int *format, @@ -503,7 +464,7 @@ qedGetBackingStore(char **res, /* Check if this image has a backing file */ if (buf_size < QED_HDR_FEATURES_OFFSET+8) return BACKING_STORE_INVALID; - flags = qedGetHeaderULL(buf + QED_HDR_FEATURES_OFFSET); + flags = virReadBufInt64LE(buf + QED_HDR_FEATURES_OFFSET); if (!(flags & QED_F_BACKING_FILE)) { *format = VIR_STORAGE_FILE_NONE; return BACKING_STORE_OK; @@ -512,10 +473,10 @@ qedGetBackingStore(char **res, /* Parse the backing file */ if (buf_size < QED_HDR_BACKING_FILE_OFFSET+8) return BACKING_STORE_INVALID; - offset = qedGetHeaderUL(buf + QED_HDR_BACKING_FILE_OFFSET); + offset = virReadBufInt32LE(buf + QED_HDR_BACKING_FILE_OFFSET); if (offset > buf_size) return BACKING_STORE_INVALID; - size = qedGetHeaderUL(buf + QED_HDR_BACKING_FILE_SIZE); + size = virReadBufInt32LE(buf + QED_HDR_BACKING_FILE_SIZE); if (size == 0) return BACKING_STORE_OK; if (offset + size > buf_size || offset + size < offset) @@ -633,19 +594,10 @@ virStorageFileMatchesVersion(int format, if ((fileTypeInfo[format].versionOffset + 4) > buflen) return false; - if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) { - version = - (buf[fileTypeInfo[format].versionOffset+3] << 24) | - (buf[fileTypeInfo[format].versionOffset+2] << 16) | - (buf[fileTypeInfo[format].versionOffset+1] << 8) | - (buf[fileTypeInfo[format].versionOffset]); - } else { - version = - (buf[fileTypeInfo[format].versionOffset] << 24) | - (buf[fileTypeInfo[format].versionOffset+1] << 16) | - (buf[fileTypeInfo[format].versionOffset+2] << 8) | - (buf[fileTypeInfo[format].versionOffset+3]); - } + if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) + version = virReadBufInt32LE(buf + fileTypeInfo[format].versionOffset); + else + version = virReadBufInt32BE(buf + fileTypeInfo[format].versionOffset); VIR_DEBUG("Compare detected version %d vs expected version %d", version, fileTypeInfo[format].versionNumber); @@ -687,29 +639,15 @@ virStorageFileGetMetadataFromBuf(int format, if ((fileTypeInfo[format].sizeOffset + 8) > buflen) return 1; - if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) { - meta->capacity = - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+7] << 56) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+6] << 48) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+5] << 40) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+4] << 32) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+3] << 24) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+2] << 16) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+1] << 8) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset]); - } else { - meta->capacity = - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset] << 56) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+1] << 48) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+2] << 40) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+3] << 32) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+4] << 24) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+5] << 16) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+6] << 8) | - ((unsigned long long)buf[fileTypeInfo[format].sizeOffset+7]); - } + if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) + meta->capacity = virReadBufInt64LE(buf + + fileTypeInfo[format].sizeOffset); + else + meta->capacity = virReadBufInt64BE(buf + + fileTypeInfo[format].sizeOffset); /* Avoid unlikely, but theoretically possible overflow */ - if (meta->capacity > (ULLONG_MAX / fileTypeInfo[format].sizeMultiplier)) + if (meta->capacity > (ULLONG_MAX / + fileTypeInfo[format].sizeMultiplier)) return 1; meta->capacity *= fileTypeInfo[format].sizeMultiplier; } @@ -717,11 +655,8 @@ virStorageFileGetMetadataFromBuf(int format, if (fileTypeInfo[format].qcowCryptOffset != -1) { int crypt_format; - crypt_format = - (buf[fileTypeInfo[format].qcowCryptOffset] << 24) | - (buf[fileTypeInfo[format].qcowCryptOffset+1] << 16) | - (buf[fileTypeInfo[format].qcowCryptOffset+2] << 8) | - (buf[fileTypeInfo[format].qcowCryptOffset+3]); + crypt_format = virReadBufInt32BE(buf + + fileTypeInfo[format].qcowCryptOffset); meta->encrypted = crypt_format != 0; } -- 1.8.1.2
On 02/11/2013 07:07 PM, Eric Blake wrote:
This makes code easier to read, by avoiding lines longer than 80 columns and removing the repetition from the callers.
* src/util/virstoragefile.c (qedGetHeaderUL, qedGetHeaderULL): Delete in favor of more generic macros. (qcow2GetBackingStoreFormat, qcowXGetBackingStore) (qedGetBackingStore, virStorageFileMatchesVersion) (virStorageFileGetMetadataInternal): Use new macros. * src/cpu/cpu_x86.c (x86VendorLoad): Likewise. --- src/cpu/cpu_x86.c | 24 ++++------ src/util/virstoragefile.c | 109 ++++++++++------------------------------------ 2 files changed, 30 insertions(+), 103 deletions(-)
ACK
Commit 8b55992f added some Coverity comments to silence what was a real bug in the code. Since then, we've had a miserable run of trying to fix the underlying problem (commits c059cde and ba5193c), and still have a problem on 32-bit machines. This fixes the problem for once and for all, by realizing that on older xen, cpumap_t is identical to uint64_t, and using the new virendian.h to do the transformation from the API (documented to be little-endian) to the host structure. * src/xen/xen_hypervisor.c (virXen_setvcpumap): Do the conversion correctly. Finally. --- src/xen/xen_hypervisor.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/xen/xen_hypervisor.c b/src/xen/xen_hypervisor.c index 9b7dd2e..d803972 100644 --- a/src/xen/xen_hypervisor.c +++ b/src/xen/xen_hypervisor.c @@ -80,6 +80,7 @@ #include "virfile.h" #include "virnodesuspend.h" #include "virtypedparam.h" +#include "virendian.h" #define VIR_FROM_THIS VIR_FROM_XEN @@ -1773,18 +1774,13 @@ virXen_setvcpumap(int handle, ret = -1; } else { cpumap_t xen_cpumap; /* limited to 64 CPUs in old hypervisors */ - uint64_t *pm = &xen_cpumap; - int j; + char buf[8] = ""; - if ((maplen > (int)sizeof(cpumap_t)) || (sizeof(cpumap_t) & 7)) + if (maplen > sizeof(cpumap_t) || sizeof(cpumap_t) != sizeof(uint64_t)) return -1; - - memset(&xen_cpumap, 0, sizeof(cpumap_t)); - for (j = 0; j < maplen; j++) { - if ((j & 7) == 0) - pm = (uint64_t *)((uint64_t)&xen_cpumap + (j & ~0x7UL)); - *pm |= (uint64_t)cpumap[j] << (8 * (j & 7)); - } + /* Supply trailing 0s if user's input array was short */ + memcpy(buf, cpumap, maplen); + xen_cpumap = virReadBufInt64LE(buf); if (hv_versions.hypervisor == 1) { xen_op_v1 op; -- 1.8.1.2
On 02/11/2013 07:07 PM, Eric Blake wrote:
Commit 8b55992f added some Coverity comments to silence what was a real bug in the code. Since then, we've had a miserable run of trying to fix the underlying problem (commits c059cde and ba5193c), and still have a problem on 32-bit machines.
This fixes the problem for once and for all, by realizing that on older xen, cpumap_t is identical to uint64_t, and using the new virendian.h to do the transformation from the API (documented to be little-endian) to the host structure.
* src/xen/xen_hypervisor.c (virXen_setvcpumap): Do the conversion correctly. Finally. --- src/xen/xen_hypervisor.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
ACK
participants (2)
-
Eric Blake -
John Ferlan