6:07 p.m.
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
6:07 p.m.
New subject: [libvirt] [PATCH 1/3] util: add virendian.h macros
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
6:56 p.m.
New subject: [libvirt] [PATCH 1/3] util: add virendian.h macros
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
10:17 a.m.
New subject: [libvirt] [PATCH 1/3] util: add virendian.h macros
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
6:07 p.m.
New subject: [libvirt] [PATCH 2/3] util: use new virendian.h macros
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
7:02 p.m.
New subject: [libvirt] [PATCH 2/3] util: use new virendian.h macros
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
6:07 p.m.
New subject: [libvirt] [PATCH 3/3] xen: clean up the mess with cpumap
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
7:06 p.m.
New subject: [libvirt] [PATCH 3/3] xen: clean up the mess with cpumap
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
4304
days inactive
4304
days old
7 comments
2 participants
participants (2)
-
Eric Blake -
John Ferlan