[libvirt] [PATCHv3 0/3] sysinfo: s390 bug fix and tests
Primarily this fixes a memory issue in virSysinfoRead on s390. As a side effect it became necessary to work around a GCC bug. Finally I have followed Daniel Berrange's suggestion to provide tests for sysinfo. V2: Turned single patch into series V3: Fixed -Werror induced build break with current GCC Viktor Mihajlovski (3): build: Check for broken GCC -Wlogical-op in configure S390: Fix virSysinfoRead memory corruption tests: Add tests for sysinfo cfg.mk | 2 +- configure.ac | 23 ++++ src/libvirt_private.syms | 1 + src/util/sysinfo.c | 195 +++++++++++++++++---------------- tests/Makefile.am | 6 + tests/sysinfodata/dmidecode.sh | 3 + tests/sysinfodata/s390cpuinfo.data | 12 ++ tests/sysinfodata/s390sysinfo.data | 114 +++++++++++++++++++ tests/sysinfodata/s390sysinfo.expect | 25 +++++ tests/sysinfodata/x86sysinfo.data | 83 ++++++++++++++ tests/sysinfodata/x86sysinfo.expect | 53 +++++++++ tests/sysinfotest.c | 200 ++++++++++++++++++++++++++++++++++ 12 files changed, 622 insertions(+), 95 deletions(-) create mode 100755 tests/sysinfodata/dmidecode.sh create mode 100644 tests/sysinfodata/s390cpuinfo.data create mode 100644 tests/sysinfodata/s390sysinfo.data create mode 100644 tests/sysinfodata/s390sysinfo.expect create mode 100644 tests/sysinfodata/x86sysinfo.data create mode 100644 tests/sysinfodata/x86sysinfo.expect create mode 100644 tests/sysinfotest.c -- 1.7.9.5
Some older versions of GCC report a false positive on code like char * haystack, needle; strchr(haystack, needle); Added an extra check in configure.ac which will #define BROKEN_GCC_WLOGICALOP 1 in this case, allowing to special handle "offending" code. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- configure.ac | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/configure.ac b/configure.ac index bf32f95..dbcd079 100644 --- a/configure.ac +++ b/configure.ac @@ -254,6 +254,29 @@ AC_CHECK_TYPE([struct ifreq], #include <net/if.h> ]]) +dnl Check whether strchr(s, char variable) causes a bogus compile warning +dnl which is the case with a certain range of GCC versions +AC_MSG_CHECKING([whether GCC -Wlogical-op is broken]) + +save_CFLAGS="$CFLAGS" +CFLAGS="-O2 -Wlogical-op -Werror" + +AC_TRY_COMPILE([#include <string.h>], + [const char *haystack; + char needle; + return strchr(haystack, needle) == haystack;], + [gcc_false_strchr_warning=no], + [gcc_false_strchr_warning=yes]) + +CFLAGS="$save_CFLAGS" + +if test "x$gcc_false_strchr_warning" = xyes; then + AC_DEFINE_UNQUOTED([BROKEN_GCC_WLOGICALOP], 1, + [GCC -Wlogical-op is reporting false positive on strchr]) +fi + +AC_MSG_RESULT([$gcc_false_strchr_warning]) + dnl Our only use of libtasn1.h is in the testsuite, and can be skipped dnl if the header is not present. Assume -ltasn1 is present if the dnl header could be found. -- 1.7.9.5
On 12/14/2012 08:08 AM, Viktor Mihajlovski wrote:
Some older versions of GCC report a false positive on code like char * haystack, needle; strchr(haystack, needle);
Added an extra check in configure.ac which will #define BROKEN_GCC_WLOGICALOP 1 in this case, allowing to special handle "offending" code.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- configure.ac | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
I know this is already pushed, but I think it would fit better under m4/virt-compile-warnings.m4 rather than at the top-level configure.ac.
+ +if test "x$gcc_false_strchr_warning" = xyes; then + AC_DEFINE_UNQUOTED([BROKEN_GCC_WLOGICALOP], 1, + [GCC -Wlogical-op is reporting false positive on strchr]) +fi
Potential ouch. This sets BROKEN_GCC_WLOGICALOP to 1 on all non-gcc compilers. Then in patch 2/3, you blindly use this condition to request a pragma: +/* + we need to ignore warnings about strchr caused by -Wlogical-op + for some GCC versions. + Doing it via a local pragma keeps the damage smaller than + disabling it on the package level. + Unfortunately, the affected GCCs don't allow diagnostic push/pop + which would have further reduced the impact. + */ +# if BROKEN_GCC_WLOGICALOP +# pragma GCC diagnostic ignored "-Wlogical-op" +# endif We are not guaranteed that other compilers behave nicely when presented with unknown #pragma GCC. So far, our use of such pragmas has been guarded by knowing that we are using gcc; so I'd feel better if the configure-time check for setting this flag were limited to just gcc in the first place (which it would be easier to do by moving this test to m4/virt-compile-warnings.m4). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/18/2012 09:38 PM, Eric Blake wrote:
I know this is already pushed, but I think it would fit better under m4/virt-compile-warnings.m4 rather than at the top-level configure.ac.
since it is early in the release it's not too late to change this, although I think - if not in configure.ac - it would rather belong to a separate m4/workarounds.m4 or similar ...
Potential ouch. This sets BROKEN_GCC_WLOGICALOP to 1 on all non-gcc compilers. Then in patch 2/3, you blindly use this condition to request a pragma: Really? My expectation is that only a GCC compiler will trigger this condition unless there are other compilers producing a compile error with strchr(x,y). In consequence it is fair to use a GCC pragma, especially as the diagnostic pragma is applicable for the failing GCC versions (according to the GCC manuals).
-- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Wed, Dec 19, 2012 at 11:14:15AM +0100, Viktor Mihajlovski wrote:
On 12/18/2012 09:38 PM, Eric Blake wrote:
I know this is already pushed, but I think it would fit better under m4/virt-compile-warnings.m4 rather than at the top-level configure.ac.
since it is early in the release it's not too late to change this, although I think - if not in configure.ac - it would rather belong to a separate m4/workarounds.m4 or similar ...
Potential ouch. This sets BROKEN_GCC_WLOGICALOP to 1 on all non-gcc compilers. Then in patch 2/3, you blindly use this condition to request a pragma: Really? My expectation is that only a GCC compiler will trigger this condition unless there are other compilers producing a compile error with strchr(x,y). In consequence it is fair to use a GCC pragma, especially as the diagnostic pragma is applicable for the failing GCC versions (according to the GCC manuals).
I think Eric means that on non-GCC this code: # if BROKEN_GCC_WLOGICALOP # pragma GCC diagnostic ignored "-Wlogical-op" # endif will cause the compiler to complain "Unknown pragma GCC diagnostic..." since obviously non-GCC compilers don't understand GCC pragmas Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 12/19/2012 11:21 AM, Daniel P. Berrange wrote:
On Wed, Dec 19, 2012 at 11:14:15AM +0100, Viktor Mihajlovski wrote:
I think Eric means that on non-GCC this code:
# if BROKEN_GCC_WLOGICALOP # pragma GCC diagnostic ignored "-Wlogical-op" # endif
will cause the compiler to complain "Unknown pragma GCC diagnostic..." since obviously non-GCC compilers don't understand GCC pragmas
Daniel
I understood that part. My statement is that BROKEN_GCC_WLOGICALOP will not be defined if configure was run on a system with non-GCC compiler. Unless I oversaw something fundamental ... which I will never rule out. OTOH I am fine with any approach that let's me build on my back-level RHEL6 box for the time being. BTW: there's still a build/install issue for non-systemd systems, see https://www.redhat.com/archives/libvir-list/2012-December/msg01051.html -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Wed, Dec 19, 2012 at 01:05:42PM +0100, Viktor Mihajlovski wrote:
On 12/19/2012 11:21 AM, Daniel P. Berrange wrote:
On Wed, Dec 19, 2012 at 11:14:15AM +0100, Viktor Mihajlovski wrote:
I think Eric means that on non-GCC this code:
# if BROKEN_GCC_WLOGICALOP # pragma GCC diagnostic ignored "-Wlogical-op" # endif
will cause the compiler to complain "Unknown pragma GCC diagnostic..." since obviously non-GCC compilers don't understand GCC pragmas
Daniel
I understood that part. My statement is that BROKEN_GCC_WLOGICALOP will not be defined if configure was run on a system with non-GCC compiler. Unless I oversaw something fundamental ... which I will never rule out.
I think it depends on what the compiler does with unknown -W flags. In the configure test you created, there are 2 possible reasons why the TRY_COMPILE can fail - Using GCC with a broken -Wlogical-op impl - Compiler which doesn't understand -Wlogical-op at all and treats this is a fatal error I think the solution here is to move the check into virt-compiler-warnings.m4 and only look for a broken GCC, if we have already validated that '-Wlogical-op' is supported by the compiler Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 12/19/2012 01:29 PM, Daniel P. Berrange wrote:
I think it depends on what the compiler does with unknown -W flags.
indeed ... I was lead to believe that unknown -W options would be gracefully tolerated with a warning. But even for GCC this is only the case for options like -Wno-clue, i.e. turning off warnings. I've send a patch for the patch that should prevent a false positive. My apologies... -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
There was a double free issue caused by virSysinfoRead on s390, as the same manufacturer string instance was assigned to more than one processor record. Cleaned up other potential memory issues and restructured the sysinfo parsing code by moving repeating patterns into a helper function. The restructuring made it necessary to conditionally disable -Wlogical-op for some older GCC versions, using pragma GCC diagnostic. This is a GCC specific pragma, which is acceptable, since we're using it to work around a GCC specific bug. Finally, added a function virSysinfoSetup to configure the sysinfo data source files/script during run time, to facilitate writing test programs. This function is not published in sysinfo.h and only there for testing. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- V3: added missing prototype for virSysinfoSetup src/libvirt_private.syms | 1 + src/util/sysinfo.c | 195 ++++++++++++++++++++++++---------------------- 2 files changed, 102 insertions(+), 94 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9288ad3..5907d1d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1186,6 +1186,7 @@ virStorageFileResize; virSysinfoDefFree; virSysinfoFormat; virSysinfoRead; +virSysinfoSetup; # threadpool.h diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c index bac4b23..f63ecec 100644 --- a/src/util/sysinfo.c +++ b/src/util/sysinfo.c @@ -39,13 +39,30 @@ #define VIR_FROM_THIS VIR_FROM_SYSINFO -#define SYSINFO_SMBIOS_DECODER "dmidecode" -#define SYSINFO "/proc/sysinfo" -#define CPUINFO "/proc/cpuinfo" VIR_ENUM_IMPL(virSysinfo, VIR_SYSINFO_LAST, "smbios"); +static const char *sysinfoDmidecode = "dmidecode"; +static const char *sysinfoSysinfo = "/proc/sysinfo"; +static const char *sysinfoCpuinfo = "/proc/cpuinfo"; + +#define SYSINFO_SMBIOS_DECODER sysinfoDmidecode +#define SYSINFO sysinfoSysinfo +#define CPUINFO sysinfoCpuinfo + +/* only to be used test programs, therefore not in sysinfo.h */ +extern void virSysinfoSetup(const char *dmidecode, const char *sysinfo, + const char *cpuinfo); + +void virSysinfoSetup(const char *dmidecode, const char *sysinfo, + const char *cpuinfo) +{ + sysinfoDmidecode = dmidecode; + sysinfoSysinfo = sysinfo; + sysinfoCpuinfo = cpuinfo; +} + /** * virSysinfoDefFree: * @def: a sysinfo structure @@ -240,114 +257,103 @@ no_memory: #elif defined(__s390__) || defined(__s390x__) -static int -virSysinfoParseSystem(const char *base, virSysinfoDefPtr ret) -{ - char *cur, *eol = NULL; - const char *property; - - /* Return if Manufacturer field is not found */ - if ((cur = strstr(base, "Manufacturer")) == NULL) - return 0; +/* + we need to ignore warnings about strchr caused by -Wlogical-op + for some GCC versions. + Doing it via a local pragma keeps the damage smaller than + disabling it on the package level. + Unfortunately, the affected GCCs don't allow diagnostic push/pop + which would have further reduced the impact. + */ +# if BROKEN_GCC_WLOGICALOP +# pragma GCC diagnostic ignored "-Wlogical-op" +# endif - base = cur; - if ((cur = strstr(base, "Manufacturer")) != NULL) { - cur = strchr(cur, ':') + 1; - eol = strchr(cur, '\n'); - virSkipSpacesBackwards(cur, &eol); - if ((eol) && ((property = strndup(cur, eol - cur)) == NULL)) - goto no_memory; - virSkipSpaces(&property); - ret->system_manufacturer = (char *) property; - } - if ((cur = strstr(base, "Type")) != NULL) { - cur = strchr(cur, ':') + 1; - eol = strchr(cur, '\n'); - virSkipSpacesBackwards(cur, &eol); - if ((eol) && ((property = strndup(cur, eol - cur)) == NULL)) - goto no_memory; - virSkipSpaces(&property); - ret->system_family = (char *) property; - } - if ((cur = strstr(base, "Sequence Code")) != NULL) { - cur = strchr(cur, ':') + 1; - eol = strchr(cur, '\n'); - virSkipSpacesBackwards(cur, &eol); - if ((eol) && ((property = strndup(cur, eol - cur)) == NULL)) - goto no_memory; - virSkipSpaces(&property); - ret->system_serial = (char *) property; +static char * +virSysinfoParseDelimited(const char *base, const char *name, char **value, + char delim1, char delim2) +{ + const char *start; + char *end; + + if (delim1 != delim2 && + (start = strstr(base, name)) && + (start = strchr(start, delim1))) { + start += 1; + end = strchrnul(start, delim2); + virSkipSpaces(&start); + if (!((*value) = strndup(start, end - start))) { + virReportOOMError(); + goto error; + } + virTrimSpaces(*value, NULL); + return end; } - return 0; +error: + return NULL; +} -no_memory: - return -1; +static char * +virSysinfoParseLine(const char *base, const char *name, char **value) +{ + return virSysinfoParseDelimited(base, name, value, ':', '\n'); +} + +static int +virSysinfoParseSystem(const char *base, virSysinfoDefPtr ret) +{ + if (virSysinfoParseLine(base, "Manufacturer", + &ret->system_manufacturer) && + virSysinfoParseLine(base, "Type", + &ret->system_family) && + virSysinfoParseLine(base, "Sequence Code", + &ret->system_serial)) + return 0; + else + return -1; } static int virSysinfoParseProcessor(const char *base, virSysinfoDefPtr ret) { - char *cur, *eol, *tmp_base; - char *manufacturer; - const char *tmp; + char *tmp_base; + char *manufacturer = NULL; + char *procline = NULL; + int result = -1; virSysinfoProcessorDefPtr processor; - if ((cur = strstr(base, "vendor_id")) != NULL) { - cur = strchr(cur, ':') + 1; - eol = strchr(cur, '\n'); - virSkipSpacesBackwards(cur, &eol); - if ((eol) && ((tmp = strndup(cur, eol - cur)) == NULL)) - goto no_memory; - virSkipSpaces(&tmp); - manufacturer = (char *) tmp; - } - - /* Find processor N: line and gather the processor manufacturer, version, - * serial number, and family */ - while ((tmp_base = strstr(base, "processor ")) != NULL) { - base = tmp_base; - eol = strchr(base, '\n'); - cur = strchr(base, ':') + 1; + if (!(tmp_base=virSysinfoParseLine(base, "vendor_id", &manufacturer))) + goto cleanup; + /* Find processor N: line and gather the processor manufacturer, + version, serial number, and family */ + while ((tmp_base = strstr(tmp_base, "processor ")) + && (tmp_base = virSysinfoParseLine(tmp_base, "processor ", + &procline))) { if (VIR_EXPAND_N(ret->processor, ret->nprocessor, 1) < 0) { - goto no_memory; + virReportOOMError(); + goto cleanup; } - processor = &ret->processor[ret->nprocessor - 1]; - - /* Set the processor manufacturer */ - processor->processor_manufacturer = manufacturer; - - if ((cur = strstr(base, "version =")) != NULL) { - cur += sizeof("version ="); - eol = strchr(cur, ','); - if ((eol) && - ((processor->processor_version = strndup(cur, eol - cur)) == NULL)) - goto no_memory; - } - if ((cur = strstr(base, "identification =")) != NULL) { - cur += sizeof("identification ="); - eol = strchr(cur, ','); - if ((eol) && - ((processor->processor_serial_number = strndup(cur, eol - cur)) == NULL)) - goto no_memory; - } - if ((cur = strstr(base, "machine =")) != NULL) { - cur += sizeof("machine ="); - eol = strchr(cur, '\n'); - if ((eol) && - ((processor->processor_family = strndup(cur, eol - cur)) == NULL)) - goto no_memory; - } - - base = cur; + processor->processor_manufacturer = strdup(manufacturer); + if (!virSysinfoParseDelimited(procline, "version", + &processor->processor_version, + '=', ',') || + !virSysinfoParseDelimited(procline, "identification", + &processor->processor_serial_number, + '=', ',') || + !virSysinfoParseDelimited(procline, "machine", + &processor->processor_family, + '=', '\n')) + goto cleanup; } + result = 0; - return 0; - -no_memory: - return -1; +cleanup: + VIR_FREE(manufacturer); + VIR_FREE(procline); + return result; } /* virSysinfoRead for s390x @@ -388,6 +394,7 @@ virSysinfoRead(void) { return ret; no_memory: + virSysinfoDefFree(ret); VIR_FREE(outbuf); return NULL; } -- 1.7.9.5
On Fri, Dec 14, 2012 at 04:08:24PM +0100, Viktor Mihajlovski wrote:
There was a double free issue caused by virSysinfoRead on s390, as the same manufacturer string instance was assigned to more than one processor record. Cleaned up other potential memory issues and restructured the sysinfo parsing code by moving repeating patterns into a helper function.
The restructuring made it necessary to conditionally disable -Wlogical-op for some older GCC versions, using pragma GCC diagnostic. This is a GCC specific pragma, which is acceptable, since we're using it to work around a GCC specific bug.
Finally, added a function virSysinfoSetup to configure the sysinfo data source files/script during run time, to facilitate writing test programs. This function is not published in sysinfo.h and only there for testing.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- V3: added missing prototype for virSysinfoSetup
src/libvirt_private.syms | 1 + src/util/sysinfo.c | 195 ++++++++++++++++++++++++---------------------- 2 files changed, 102 insertions(+), 94 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 12/14/2012 08:08 AM, Viktor Mihajlovski wrote:
There was a double free issue caused by virSysinfoRead on s390, as the same manufacturer string instance was assigned to more than one processor record. Cleaned up other potential memory issues and restructured the sysinfo parsing code by moving repeating patterns into a helper function.
+# if BROKEN_GCC_WLOGICALOP +# pragma GCC diagnostic ignored "-Wlogical-op" +# endif
Do you have cppi installed? If so, I'm surprised this passed 'make syntax-check'. The middle line should be indented further as: # if ... # pragma ... # endif to keep cppi happy. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Dec 18, 2012 at 01:40:45PM -0700, Eric Blake wrote:
On 12/14/2012 08:08 AM, Viktor Mihajlovski wrote:
There was a double free issue caused by virSysinfoRead on s390, as the same manufacturer string instance was assigned to more than one processor record. Cleaned up other potential memory issues and restructured the sysinfo parsing code by moving repeating patterns into a helper function.
+# if BROKEN_GCC_WLOGICALOP +# pragma GCC diagnostic ignored "-Wlogical-op" +# endif
Do you have cppi installed? If so, I'm surprised this passed 'make syntax-check'. The middle line should be indented further as:
# if ... # pragma ... # endif
to keep cppi happy.
Indeed, I fixed this when I pushed the patch Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Test cases for virSysinfoRead. Initially, there are tests for x86 (DMI based) and s390 (/proc/... based). In lack of PPC data, I have stubbed out the test for it, but it can be added with a minimal effort. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- cfg.mk | 2 +- tests/Makefile.am | 6 + tests/sysinfodata/dmidecode.sh | 3 + tests/sysinfodata/s390cpuinfo.data | 12 ++ tests/sysinfodata/s390sysinfo.data | 114 +++++++++++++++++++ tests/sysinfodata/s390sysinfo.expect | 25 +++++ tests/sysinfodata/x86sysinfo.data | 83 ++++++++++++++ tests/sysinfodata/x86sysinfo.expect | 53 +++++++++ tests/sysinfotest.c | 200 ++++++++++++++++++++++++++++++++++ 9 files changed, 497 insertions(+), 1 deletion(-) create mode 100755 tests/sysinfodata/dmidecode.sh create mode 100644 tests/sysinfodata/s390cpuinfo.data create mode 100644 tests/sysinfodata/s390sysinfo.data create mode 100644 tests/sysinfodata/s390sysinfo.expect create mode 100644 tests/sysinfodata/x86sysinfo.data create mode 100644 tests/sysinfodata/x86sysinfo.expect create mode 100644 tests/sysinfotest.c diff --git a/cfg.mk b/cfg.mk index 1fe007e..84cc942 100644 --- a/cfg.mk +++ b/cfg.mk @@ -821,7 +821,7 @@ exclude_file_name_regexp--sc_require_config_h = ^(examples/|tools/virsh-$(_virsh exclude_file_name_regexp--sc_require_config_h_first = ^(examples/|tools/virsh-$(_virsh_includes)\.c$$) exclude_file_name_regexp--sc_trailing_blank = \ - (/qemuhelpdata/|\.(fig|gif|ico|png)$$) + (/qemuhelpdata/|/sysinfodata/.*\.data|\.(fig|gif|ico|png)$$) exclude_file_name_regexp--sc_unmarked_diagnostics = \ ^(docs/apibuild.py|tests/virt-aa-helper-test)$$ diff --git a/tests/Makefile.am b/tests/Makefile.am index 5fb26ad..b603ea3 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -76,6 +76,7 @@ EXTRA_DIST = \ storagevolschematest \ storagevolxml2xmlin \ storagevolxml2xmlout \ + sysinfodata \ test-lib.sh \ vmx2xmldata \ xencapsdata \ @@ -96,6 +97,7 @@ test_programs = virshtest sockettest \ virbitmaptest \ virlockspacetest \ virstringtest \ + sysinfotest \ $(NULL) if WITH_SECDRIVER_SELINUX @@ -637,6 +639,10 @@ shunloadtest_SOURCES = \ shunloadtest_LDADD = $(LIB_PTHREAD) shunloadtest_DEPENDENCIES = libshunload.la +sysinfotest_SOURCES = \ + sysinfotest.c testutils.h testutils.c +sysinfotest_LDADD = $(LDADDS) + if WITH_CIL CILOPTFLAGS = CILOPTINCS = diff --git a/tests/sysinfodata/dmidecode.sh b/tests/sysinfodata/dmidecode.sh new file mode 100755 index 0000000..28aed61 --- /dev/null +++ b/tests/sysinfodata/dmidecode.sh @@ -0,0 +1,3 @@ +#!/bin/sh +DATAFILE=`dirname $0`/x86sysinfo.data +cat $DATAFILE diff --git a/tests/sysinfodata/s390cpuinfo.data b/tests/sysinfodata/s390cpuinfo.data new file mode 100644 index 0000000..c01d146 --- /dev/null +++ b/tests/sysinfodata/s390cpuinfo.data @@ -0,0 +1,12 @@ +vendor_id : IBM/S390 +# processors : 3 +bogomips per cpu: 14367.00 +features : esan3 zarch stfle msa ldisp eimm dfp etf3eh highgprs +cache0 : level=1 type=Data scope=Private size=128K line_size=256 associativity=8 +cache1 : level=1 type=Instruction scope=Private size=64K line_size=256 associativity=4 +cache2 : level=2 type=Unified scope=Private size=1536K line_size=256 associativity=12 +cache3 : level=3 type=Unified scope=Shared size=24576K line_size=256 associativity=12 +cache4 : level=4 type=Unified scope=Shared size=196608K line_size=256 associativity=24 +processor 0: version = FF, identification = 000123, machine = 2817 +processor 1: version = FF, identification = 100123, machine = 2817 +processor 2: version = FF, identification = 200123, machine = 2817 diff --git a/tests/sysinfodata/s390sysinfo.data b/tests/sysinfodata/s390sysinfo.data new file mode 100644 index 0000000..954b51d --- /dev/null +++ b/tests/sysinfodata/s390sysinfo.data @@ -0,0 +1,114 @@ +Manufacturer: IBM +Type: 2817 +Model: 703 M66 +Sequence Code: 0000000000012345 +Plant: 02 +Model Capacity: 703 00000408 +Model Perm. Capacity: 703 00000408 +Model Temp. Capacity: 703 00000408 +Nominal Cap. Rating: 00000408 +Nominal Perm. Rating: 00000408 +Nominal Temp. Rating: 00000408 +Capacity Adj. Ind.: 100 +Capacity Ch. Reason: 0 +Capacity Transient: 0 +Type 1 Percentage: 0 +Type 2 Percentage: 0 +Type 3 Percentage: 0 +Type 4 Percentage: 0 +Type 5 Percentage: 0 + +CPUs Total: 69 +CPUs Configured: 3 +CPUs Standby: 0 +CPUs Reserved: 66 +Capability: 696 +Nominal Capability: 696 +Secondary Capability: 696 +Adjustment 02-way: 61900 +Adjustment 03-way: 60280 +Adjustment 04-way: 59080 +Adjustment 05-way: 57740 +Adjustment 06-way: 56900 +Adjustment 07-way: 56040 +Adjustment 08-way: 55120 +Adjustment 09-way: 53950 +Adjustment 10-way: 52980 +Adjustment 11-way: 52000 +Adjustment 12-way: 51080 +Adjustment 13-way: 50380 +Adjustment 14-way: 49600 +Adjustment 15-way: 48760 +Adjustment 16-way: 47860 +Adjustment 17-way: 47310 +Adjustment 18-way: 46860 +Adjustment 19-way: 46300 +Adjustment 20-way: 46020 +Adjustment 21-way: 45520 +Adjustment 22-way: 45260 +Adjustment 23-way: 44960 +Adjustment 24-way: 44690 +Adjustment 25-way: 44390 +Adjustment 26-way: 44140 +Adjustment 27-way: 43860 +Adjustment 28-way: 43460 +Adjustment 29-way: 43200 +Adjustment 30-way: 42900 +Adjustment 31-way: 42620 +Adjustment 32-way: 42360 +Adjustment 33-way: 42060 +Adjustment 34-way: 41800 +Adjustment 35-way: 41600 +Adjustment 36-way: 41440 +Adjustment 37-way: 41280 +Adjustment 38-way: 41040 +Adjustment 39-way: 40820 +Adjustment 40-way: 40570 +Adjustment 41-way: 40350 +Adjustment 42-way: 40260 +Adjustment 43-way: 40060 +Adjustment 44-way: 39910 +Adjustment 45-way: 39750 +Adjustment 46-way: 39540 +Adjustment 47-way: 39360 +Adjustment 48-way: 39260 +Adjustment 49-way: 39180 +Adjustment 50-way: 39000 +Adjustment 51-way: 38840 +Adjustment 52-way: 38740 +Adjustment 53-way: 38640 +Adjustment 54-way: 38520 +Adjustment 55-way: 38440 +Adjustment 56-way: 38330 +Adjustment 57-way: 38240 +Adjustment 58-way: 38160 +Adjustment 59-way: 38080 +Adjustment 60-way: 37960 +Adjustment 61-way: 37880 +Adjustment 62-way: 37780 +Adjustment 63-way: 37680 +Adjustment 64-way: 37580 +Adjustment 65-way: 37480 +Adjustment 66-way: 37360 +Adjustment 67-way: 37240 +Adjustment 68-way: 37120 +Adjustment 69-way: 36970 + +LPAR Number: 47 +LPAR Characteristics: Shared +LPAR Name: VIRLP01 +LPAR Adjustment: 253 +LPAR CPUs Total: 16 +LPAR CPUs Configured: 16 +LPAR CPUs Standby: 0 +LPAR CPUs Reserved: 0 +LPAR CPUs Dedicated: 0 +LPAR CPUs Shared: 16 + +VM00 Name: VIRVM01 +VM00 Control Program: z/VM 6.2.0 +VM00 Adjustment: 187 +VM00 CPUs Total: 3 +VM00 CPUs Configured: 3 +VM00 CPUs Standby: 0 +VM00 CPUs Reserved: 0 diff --git a/tests/sysinfodata/s390sysinfo.expect b/tests/sysinfodata/s390sysinfo.expect new file mode 100644 index 0000000..21712db --- /dev/null +++ b/tests/sysinfodata/s390sysinfo.expect @@ -0,0 +1,25 @@ +<sysinfo type='smbios'> + <system> + <entry name='manufacturer'>IBM</entry> + <entry name='serial'>0000000000012345</entry> + <entry name='family'>2817</entry> + </system> + <processor> + <entry name='family'>2817</entry> + <entry name='manufacturer'>IBM/S390</entry> + <entry name='version'>FF</entry> + <entry name='serial_number'>000123</entry> + </processor> + <processor> + <entry name='family'>2817</entry> + <entry name='manufacturer'>IBM/S390</entry> + <entry name='version'>FF</entry> + <entry name='serial_number'>100123</entry> + </processor> + <processor> + <entry name='family'>2817</entry> + <entry name='manufacturer'>IBM/S390</entry> + <entry name='version'>FF</entry> + <entry name='serial_number'>200123</entry> + </processor> +</sysinfo> diff --git a/tests/sysinfodata/x86sysinfo.data b/tests/sysinfodata/x86sysinfo.data new file mode 100644 index 0000000..b9da842 --- /dev/null +++ b/tests/sysinfodata/x86sysinfo.data @@ -0,0 +1,83 @@ +BIOS Information + Vendor: LENOVO + Version: 6DET62WW (3.12 ) + Release Date: 01/12/2010 + Address: 0xE0000 + Runtime Size: 128 kB + ROM Size: 8192 kB + Characteristics: + PCI is supported + PC Card (PCMCIA) is supported + PNP is supported + BIOS is upgradeable + BIOS shadowing is allowed + ESCD support is available + Boot from CD is supported + Selectable boot is supported + BIOS ROM is socketed + EDD is supported + ACPI is supported + USB legacy is supported + BIOS boot specification is supported + Targeted content distribution is supported + BIOS Revision: 3.18 + Firmware Revision: 1.6 + +System Information + Manufacturer: LENOVO + Product Name: 7458AU2 + Version: ThinkPad X200 + Serial Number: L3AAK6G + UUID: 4CD00C81-4A85-11CB-88EC-A5A7A8F13986 + Wake-up Type: Power Switch + SKU Number: Not Specified + Family: ThinkPad X200 + +Processor Information + Socket Designation: None + Type: Central Processor + Family: Other + Manufacturer: GenuineIntel + ID: 76 06 01 00 FF FB EB BF + Version: Intel(R) Core(TM)2 Duo CPU P8600 @ 2.40GHz + Voltage: 1.2 V + External Clock: 266 MHz + Max Speed: 2400 MHz + Current Speed: 2400 MHz + Status: Populated, Enabled + Upgrade: None + Serial Number: Not Specified + Asset Tag: Not Specified + Part Number: Not Specified + +Memory Device + Total Width: 64 bits + Data Width: 64 bits + Size: 2048 MB + Form Factor: SODIMM + Set: None + Locator: DIMM 1 + Bank Locator: Bank 0/1 + Type: <OUT OF SPEC> + Type Detail: Synchronous + Speed: 1066 MHz (0.9 ns) + Manufacturer: 80CE + Serial Number: 46614E40 + Asset Tag: 0841 + Part Number: M471B5673DZ1-CF8 + +Memory Device + Total Width: 64 bits + Data Width: 64 bits + Size: 2048 MB + Form Factor: SODIMM + Set: None + Locator: DIMM 2 + Bank Locator: Bank 2/3 + Type: <OUT OF SPEC> + Type Detail: Synchronous + Speed: 1066 MHz (0.9 ns) + Manufacturer: 8551 + Serial Number: 29057112 + Asset Tag: 0839 + Part Number: IMSH2GS13A1F1C-10F diff --git a/tests/sysinfodata/x86sysinfo.expect b/tests/sysinfodata/x86sysinfo.expect new file mode 100644 index 0000000..fcdd790 --- /dev/null +++ b/tests/sysinfodata/x86sysinfo.expect @@ -0,0 +1,53 @@ +<sysinfo type='smbios'> + <bios> + <entry name='vendor'>LENOVO</entry> + <entry name='version'>6DET62WW (3.12 )</entry> + <entry name='date'>01/12/2010</entry> + <entry name='release'>3.18</entry> + </bios> + <system> + <entry name='manufacturer'>LENOVO</entry> + <entry name='product'>7458AU2</entry> + <entry name='version'>ThinkPad X200</entry> + <entry name='serial'>L3AAK6G</entry> + <entry name='uuid'>4CD00C81-4A85-11CB-88EC-A5A7A8F13986</entry> + <entry name='sku'>Not Specified</entry> + <entry name='family'>ThinkPad X200</entry> + </system> + <processor> + <entry name='socket_destination'>None</entry> + <entry name='type'>Central Processor</entry> + <entry name='family'>Other</entry> + <entry name='manufacturer'>GenuineIntel</entry> + <entry name='version'>Intel(R) Core(TM)2 Duo CPU P8600 @ 2.40GHz</entry> + <entry name='external_clock'>266 MHz</entry> + <entry name='max_speed'>2400 MHz</entry> + <entry name='status'>Populated, Enabled</entry> + <entry name='serial_number'>Not Specified</entry> + <entry name='part_number'>Not Specified</entry> + </processor> + <memory_device> + <entry name='size'>2048 MB</entry> + <entry name='form_factor'>SODIMM</entry> + <entry name='locator'>DIMM 1</entry> + <entry name='bank_locator'>Bank 0/1</entry> + <entry name='type'><OUT OF SPEC></entry> + <entry name='type_detail'>Synchronous</entry> + <entry name='speed'>1066 MHz (0.9 ns)</entry> + <entry name='manufacturer'>80CE</entry> + <entry name='serial_number'>46614E40</entry> + <entry name='part_number'>M471B5673DZ1-CF8</entry> + </memory_device> + <memory_device> + <entry name='size'>2048 MB</entry> + <entry name='form_factor'>SODIMM</entry> + <entry name='locator'>DIMM 2</entry> + <entry name='bank_locator'>Bank 2/3</entry> + <entry name='type'><OUT OF SPEC></entry> + <entry name='type_detail'>Synchronous</entry> + <entry name='speed'>1066 MHz (0.9 ns)</entry> + <entry name='manufacturer'>8551</entry> + <entry name='serial_number'>29057112</entry> + <entry name='part_number'>IMSH2GS13A1F1C-10F</entry> + </memory_device> +</sysinfo> diff --git a/tests/sysinfotest.c b/tests/sysinfotest.c new file mode 100644 index 0000000..79ee128 --- /dev/null +++ b/tests/sysinfotest.c @@ -0,0 +1,200 @@ +/* + * sysinfotest.c: Testcase(s) for virSysinfoRead + * + * Copyright IBM Corp. 2012 + * + * 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/>. + * + * Authors: + * Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> + +#include "internal.h" + +#include "buf.h" +#include "sysinfo.h" +#include "testutils.h" +#include "util.h" +#include "virfile.h" + +#if defined (__linux__) + +/* from sysinfo.c */ +void virSysinfoSetup(const char *decoder, + const char *sysinfo, + const char *cpuinfo); + +struct testSysinfoData { + char *decoder; /* name of dmi decoder binary/script */ + char *sysinfo; /* name of /proc/sysinfo substitute file */ + char *cpuinfo; /* name of /proc/cpuinfo substitute file */ + char *expected; /* (required) file containing output of virSysinfoFormat */ +}; + +# if defined(__powerpc__) || defined(__powerpc64__) +/* TODO ppc: remove the if defined() branch + to enable the real test run after providing test data, see below. + */ +static int +testSysinfo(const void *data ATTRIBUTE_UNUSED) +{ + return EXIT_AM_SKIP; +} + +static int +sysinfotest_run(const char *test ATTRIBUTE_UNUSED, + const char *decoder ATTRIBUTE_UNUSED, + const char *sysinfo ATTRIBUTE_UNUSED, + const char *cpuinfo ATTRIBUTE_UNUSED, + const char *expected ATTRIBUTE_UNUSED) +{ + return testSysinfo(NULL); +} +# else + +static int +testSysinfo(const void *data) +{ + int result = -1; + char *sysfsExpectData = NULL; + const char *sysfsActualData; + virSysinfoDefPtr ret = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + const struct testSysinfoData *testdata = data; + + virSysinfoSetup(testdata->decoder, testdata->sysinfo, testdata->cpuinfo); + + if (!testdata->expected || + virtTestLoadFile(testdata->expected, &sysfsExpectData) < 0 || + !(ret = virSysinfoRead())) { + goto cleanup; + } + + if (virSysinfoFormat(&buf,ret) < 0) + goto cleanup; + + if (!(sysfsActualData = virBufferCurrentContent(&buf))) + goto cleanup; + + if (STRNEQ(sysfsActualData, sysfsExpectData)) { + virtTestDifference(stderr, sysfsActualData, sysfsExpectData); + goto cleanup; + } + + result = 0; + +cleanup: + VIR_FREE(sysfsExpectData); + virSysinfoDefFree(ret); + virBufferFreeAndReset(&buf); + + return result; +} + +static int +sysinfotest_run(const char *test, + const char *decoder, + const char *sysinfo, + const char *cpuinfo, + const char *expected) +{ + struct testSysinfoData testdata = { NULL }; + int ret = EXIT_FAILURE; + + if ((decoder && + virAsprintf(&testdata.decoder, "%s/%s", abs_srcdir, decoder) < 0) || + (sysinfo && + virAsprintf(&testdata.sysinfo, "%s/%s", abs_srcdir, sysinfo) < 0) || + (cpuinfo && + virAsprintf(&testdata.cpuinfo, "%s/%s", abs_srcdir, cpuinfo) < 0) || + (expected && + virAsprintf(&testdata.expected, "%s/%s", abs_srcdir, expected) < 0)) { + goto error; + } + + if (virtTestRun(test, 1, testSysinfo, &testdata) < 0) + goto error; + + ret = EXIT_SUCCESS; + +error: + VIR_FREE(testdata.decoder); + VIR_FREE(testdata.sysinfo); + VIR_FREE(testdata.cpuinfo); + VIR_FREE(testdata.expected); + return ret; +} +# endif /* defined(__powerpc__) ... */ + +# if defined(__s390__) || defined(__s390x__) +static int +test_s390(void) +{ + return sysinfotest_run("s390 sysinfo", + NULL, + "/sysinfodata/s390sysinfo.data", + "/sysinfodata/s390cpuinfo.data", + "/sysinfodata/s390sysinfo.expect"); +} + +VIRT_TEST_MAIN(test_s390) +# elif defined(__powerpc__) || defined(__powerpc64__) +/* TODO for PPC owner: provide test data + and enable the real sysinfotest_run above +*/ +static int +test_ppc(void) +{ + return sysinfotest_run("ppc sysinfo", + NULL, + NULL, + "/sysinfodata/ppccpuinfo.data", + "/sysinfodata/ppcsysinfo.expect"); +} + +VIRT_TEST_MAIN(test_ppc) +# elif defined(__i386__) || defined(__x86_64__) || defined(__amd64__) +static int +test_x86(void) +{ + return sysinfotest_run("x86 sysinfo", + "/sysinfodata/dmidecode.sh", + NULL, + NULL, + "/sysinfodata/x86sysinfo.expect"); +} + +VIRT_TEST_MAIN(test_x86) +# else +int +main(void) +{ + return EXIT_AM_SKIP; +} +# endif /* defined(__s390__) ... */ +#else +int +main(void) +{ + return EXIT_AM_SKIP; +} +#endif /* defined(__linux__) */ -- 1.7.9.5
On Fri, Dec 14, 2012 at 04:08:25PM +0100, Viktor Mihajlovski wrote:
Test cases for virSysinfoRead. Initially, there are tests for x86 (DMI based) and s390 (/proc/... based). In lack of PPC data, I have stubbed out the test for it, but it can be added with a minimal effort.
Cool, top marks for doing the x86 impl while you were looking at s390.
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- cfg.mk | 2 +- tests/Makefile.am | 6 + tests/sysinfodata/dmidecode.sh | 3 + tests/sysinfodata/s390cpuinfo.data | 12 ++ tests/sysinfodata/s390sysinfo.data | 114 +++++++++++++++++++ tests/sysinfodata/s390sysinfo.expect | 25 +++++ tests/sysinfodata/x86sysinfo.data | 83 ++++++++++++++ tests/sysinfodata/x86sysinfo.expect | 53 +++++++++ tests/sysinfotest.c | 200 ++++++++++++++++++++++++++++++++++ 9 files changed, 497 insertions(+), 1 deletion(-) create mode 100755 tests/sysinfodata/dmidecode.sh create mode 100644 tests/sysinfodata/s390cpuinfo.data create mode 100644 tests/sysinfodata/s390sysinfo.data create mode 100644 tests/sysinfodata/s390sysinfo.expect create mode 100644 tests/sysinfodata/x86sysinfo.data create mode 100644 tests/sysinfodata/x86sysinfo.expect create mode 100644 tests/sysinfotest.c
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Viktor Mihajlovski