[PATCH 0/5] virsysinfo: Be more forgiving when decoding OEM strings
See 4/5 for explanation. Michal Prívozník (5): vircommand: Initialize dryRunStatus to portable EXIT_SUCCESS instead of 0 virsysinfo: Trim newline when decoding OEM strings tests: Add HPE Apollo test case to sysinfotest virsysinfo: Be more forgiving when decoding OEM strings virsysinfo: Calculate OEM string index better src/util/vircommand.c | 2 +- src/util/virsysinfo.c | 36 +++- .../aarch64-hpe-apollosysinfo.data | 162 ++++++++++++++++++ .../aarch64-hpe-apollosysinfo.expect | 88 ++++++++++ tests/sysinfotest.c | 9 +- 5 files changed, 285 insertions(+), 12 deletions(-) create mode 100644 tests/sysinfodata/aarch64-hpe-apollosysinfo.data create mode 100644 tests/sysinfodata/aarch64-hpe-apollosysinfo.expect -- 2.44.2
If dry run of a command was requested (virCommandSetDryRun()) then a specified callback is called instead of running actual command. This is meant to be used in tests. To mimic running the command as closely as possible the callback can also set exit status of the command it's implementing. To save some lines though, the exit status is initialized to 0 so that callback has to set it only on failures. Now, 0 is not exactly portable value - that's why stdlib.h has EXIT_SUCCESS (and EXIT_FAILURE) values. Initialize the exit status (held in dryRunStatus) to EXIT_SUCCESS then. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircommand.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 07bee9b12e..a03fcc37ae 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -2630,7 +2630,7 @@ virCommandRunAsync(virCommand *cmd, pid_t *pid) if (dryRunBuffer || dryRunCallback) { g_autofree char *cmdstr = NULL; - dryRunStatus = 0; + dryRunStatus = EXIT_SUCCESS; if (!(cmdstr = virCommandToStringFull(cmd, dryRunBufferArgLinebreaks, dryRunBufferCommandStripPath))) -- 2.44.2
dmidecode always puts a newline character at the end of each OEM string it prints. It's the dmi_oem_strings() function [1] that iterates over strings and calls pr_attr() over each one which puts "\n" at the end, unconditionally [2[. Since it's not part of the string though, trim it. 1: https://git.savannah.nongnu.org/cgit/dmidecode.git/tree/dmidecode.c#n2431 2: https://git.savannah.nongnu.org/cgit/dmidecode.git/tree/dmioutput.c#n63 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virsysinfo.c | 2 ++ tests/sysinfotest.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index 0d9083c891..cdc2a7d87b 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -915,6 +915,8 @@ virSysinfoDMIDecodeOEMString(size_t i, if (err && *err != '\0') return -1; + virStringTrimOptionalNewline(*str); + return 0; } diff --git a/tests/sysinfotest.c b/tests/sysinfotest.c index 287c40c6f9..09cfe6fa45 100644 --- a/tests/sysinfotest.c +++ b/tests/sysinfotest.c @@ -66,7 +66,7 @@ testDMIDecodeDryRun(const char *const*args G_GNUC_UNUSED, if (STREQ(args[3], "3")) { *output = g_strdup("Ha ha ha try parsing\\n\n" " String 3: this correctly\n" - " String 4:then"); + " String 4:then\n"); } else { *error = g_strdup_printf("No OEM string number %s", args[3]); *status = EXIT_FAILURE; -- 2.44.2
Introduce a test case for sysinfotest. The data was obtained by running dmidecode as libvirt would run it: dmidecode -q -t 0,1,2,3,4,11,17 Now, the expected output fits almost perfectly, except for OEM strings where the third string looks nothing like in the dmidecode output. This is because of testDMIDecodeDryRun() which overwrites the third OEM string (see v6.5.0-rc1~214 for more info). But that's okay for now. Speaking of OEM strings, it's worth noticing two 'OEM Strings' sections in the dmidecode output. This is causing some troubles and will be fixed in next commit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- .../aarch64-hpe-apollosysinfo.data | 162 ++++++++++++++++++ .../aarch64-hpe-apollosysinfo.expect | 90 ++++++++++ tests/sysinfotest.c | 1 + 3 files changed, 253 insertions(+) create mode 100644 tests/sysinfodata/aarch64-hpe-apollosysinfo.data create mode 100644 tests/sysinfodata/aarch64-hpe-apollosysinfo.expect diff --git a/tests/sysinfodata/aarch64-hpe-apollosysinfo.data b/tests/sysinfodata/aarch64-hpe-apollosysinfo.data new file mode 100644 index 0000000000..769373f2ce --- /dev/null +++ b/tests/sysinfodata/aarch64-hpe-apollosysinfo.data @@ -0,0 +1,162 @@ +BIOS Information + Vendor: EDK II + Version: L50_5.13_1.11 + Release Date: 06/18/2019 + Address: 0xF0000 + Runtime Size: 64 kB + ROM Size: 64 MB + Characteristics: + PCI is supported + BIOS is upgradeable + BIOS shadowing is allowed + Boot from CD is supported + Selectable boot is supported + BIOS ROM is socketed + ACPI is supported + BIOS boot specification is supported + Targeted content distribution is supported + UEFI is supported + BIOS Revision: 7.2 + +System Information + Manufacturer: HPE + Product Name: Apollo 70 + Version: X1 + Serial Number: 0000000000 + UUID: 69d9c240-bfde-1000-03ca-ea3db8730904 + Wake-up Type: Power Switch + SKU Number: P00367-B21 + Family: CN99XX + +Base Board Information + Manufacturer: HPE + Product Name: C01_APACHE_MB + Version: 876453-001 + Serial Number: PWJBD0AR9BI016 + Asset Tag: PWJBD0AR9BI016 + Features: + Board is a hosting board + Board is replaceable + Location In Chassis: Default string + Type: Motherboard + +Chassis Information + Manufacturer: HPE + Type: Rack Mount Chassis + Lock: Not Present + Version: P00367-B21 + Serial Number: 0000000000 + Asset Tag: 0000000000 + Boot-up State: Safe + Power Supply State: Safe + Thermal State: Safe + Security Status: None + OEM Information: 0x00000000 + Height: Unspecified + Number Of Power Cords: 1 + Contained Elements: 0 + SKU Number: Default string + +OEM Strings + String 1: Default string + +OEM Strings + String 1: ThunderX2 System + String 2: cavium.com + String 3: Comanche + +Processor Information + Socket Designation: Socket 0 + Type: Central Processor + Family: ARM + Manufacturer: Cavium Inc. + Signature: Implementor 0x43, Variant 0x1, Architecture 15, Part 0x0af, Revision 1 + Version: Cavium ThunderX2(R) CPU CN9975 v2.1 @ 2.0GHz + Voltage: 0.8 V + External Clock: 33 MHz + Max Speed: 2500 MHz + Current Speed: 2000 MHz + Status: Populated, Enabled + Upgrade: Other + Serial Number: 0000115D-1AEA3248 + Asset Tag: Not Specified + Part Number: CN9975-2000LG4077-Y21-G + Core Count: 28 + Core Enabled: 28 + Thread Count: 112 + Characteristics: + 64-bit capable + Multi-Core + Hardware Thread + Execute Protection + Enhanced Virtualization + Power/Performance Control + +Memory Device + Total Width: 72 bits + Data Width: 64 bits + Size: 16 GB + Form Factor: DIMM + Set: None + Locator: DIMM_A0 + Bank Locator: N0 + Type: DDR4 + Type Detail: Registered (Buffered) + Speed: 2666 MT/s + Manufacturer: Hyundai Electronics + Serial Number: 1112731123 + Asset Tag: Not Specified + Part Number: HMA82GR7CJR8N-VK + Rank: 2 + Configured Memory Speed: 2666 MT/s + Minimum Voltage: 1.2 V + Maximum Voltage: 1.2 V + Configured Voltage: 1.2 V + +Processor Information + Socket Designation: Socket 1 + Type: Central Processor + Family: ARM + Manufacturer: Cavium Inc. + Signature: Implementor 0x43, Variant 0x1, Architecture 15, Part 0x0af, Revision 1 + Version: Cavium ThunderX2(R) CPU CN9975 v2.1 @ 2.0GHz + Voltage: 0.8 V + External Clock: 33 MHz + Max Speed: 2500 MHz + Current Speed: 2000 MHz + Status: Populated, Enabled + Upgrade: Other + Serial Number: 0000115D-2AEA322B + Asset Tag: Not Specified + Part Number: CN9975-2000LG4077-Y21-G + Core Count: 28 + Core Enabled: 28 + Thread Count: 112 + Characteristics: + 64-bit capable + Multi-Core + Hardware Thread + Execute Protection + Enhanced Virtualization + Power/Performance Control + +Memory Device + Total Width: 72 bits + Data Width: 64 bits + Size: 16 GB + Form Factor: DIMM + Set: None + Locator: DIMM_I0 + Bank Locator: N1 + Type: DDR4 + Type Detail: Registered (Buffered) + Speed: 2666 MT/s + Manufacturer: Hyundai Electronics + Serial Number: 1112731032 + Asset Tag: Not Specified + Part Number: HMA82GR7CJR8N-VK + Rank: 2 + Configured Memory Speed: 2666 MT/s + Minimum Voltage: 1.2 V + Maximum Voltage: 1.2 V + Configured Voltage: 1.2 V diff --git a/tests/sysinfodata/aarch64-hpe-apollosysinfo.expect b/tests/sysinfodata/aarch64-hpe-apollosysinfo.expect new file mode 100644 index 0000000000..331454834d --- /dev/null +++ b/tests/sysinfodata/aarch64-hpe-apollosysinfo.expect @@ -0,0 +1,90 @@ +<sysinfo type='smbios'> + <bios> + <entry name='vendor'>EDK II</entry> + <entry name='version'>L50_5.13_1.11</entry> + <entry name='date'>06/18/2019</entry> + <entry name='release'>7.2</entry> + </bios> + <system> + <entry name='manufacturer'>HPE</entry> + <entry name='product'>Apollo 70</entry> + <entry name='version'>X1</entry> + <entry name='serial'>0000000000</entry> + <entry name='uuid'>69d9c240-bfde-1000-03ca-ea3db8730904</entry> + <entry name='sku'>P00367-B21</entry> + <entry name='family'>CN99XX</entry> + </system> + <baseBoard> + <entry name='manufacturer'>HPE</entry> + <entry name='product'>C01_APACHE_MB</entry> + <entry name='version'>876453-001</entry> + <entry name='serial'>PWJBD0AR9BI016</entry> + <entry name='asset'>PWJBD0AR9BI016</entry> + <entry name='location'>Default string</entry> + </baseBoard> + <chassis> + <entry name='manufacturer'>HPE</entry> + <entry name='version'>P00367-B21</entry> + <entry name='serial'>0000000000</entry> + <entry name='asset'>0000000000</entry> + <entry name='sku'>Default string</entry> + </chassis> + <processor> + <entry name='socket_destination'>Socket 0</entry> + <entry name='type'>Central Processor</entry> + <entry name='family'>ARM</entry> + <entry name='manufacturer'>Cavium Inc.</entry> + <entry name='signature'>Implementor 0x43, Variant 0x1, Architecture 15, Part 0x0af, Revision 1</entry> + <entry name='version'>Cavium ThunderX2(R) CPU CN9975 v2.1 @ 2.0GHz</entry> + <entry name='external_clock'>33 MHz</entry> + <entry name='max_speed'>2500 MHz</entry> + <entry name='status'>Populated, Enabled</entry> + <entry name='serial_number'>0000115D-1AEA3248</entry> + <entry name='part_number'>CN9975-2000LG4077-Y21-G</entry> + </processor> + <processor> + <entry name='socket_destination'>Socket 1</entry> + <entry name='type'>Central Processor</entry> + <entry name='family'>ARM</entry> + <entry name='manufacturer'>Cavium Inc.</entry> + <entry name='signature'>Implementor 0x43, Variant 0x1, Architecture 15, Part 0x0af, Revision 1</entry> + <entry name='version'>Cavium ThunderX2(R) CPU CN9975 v2.1 @ 2.0GHz</entry> + <entry name='external_clock'>33 MHz</entry> + <entry name='max_speed'>2500 MHz</entry> + <entry name='status'>Populated, Enabled</entry> + <entry name='serial_number'>0000115D-2AEA322B</entry> + <entry name='part_number'>CN9975-2000LG4077-Y21-G</entry> + </processor> + <memory_device> + <entry name='size'>16 GB</entry> + <entry name='form_factor'>DIMM</entry> + <entry name='locator'>DIMM_A0</entry> + <entry name='bank_locator'>N0</entry> + <entry name='type'>DDR4</entry> + <entry name='type_detail'>Registered (Buffered)</entry> + <entry name='speed'>2666 MT/s</entry> + <entry name='manufacturer'>Hyundai Electronics</entry> + <entry name='serial_number'>1112731123</entry> + <entry name='part_number'>HMA82GR7CJR8N-VK</entry> + </memory_device> + <memory_device> + <entry name='size'>16 GB</entry> + <entry name='form_factor'>DIMM</entry> + <entry name='locator'>DIMM_I0</entry> + <entry name='bank_locator'>N1</entry> + <entry name='type'>DDR4</entry> + <entry name='type_detail'>Registered (Buffered)</entry> + <entry name='speed'>2666 MT/s</entry> + <entry name='manufacturer'>Hyundai Electronics</entry> + <entry name='serial_number'>1112731032</entry> + <entry name='part_number'>HMA82GR7CJR8N-VK</entry> + </memory_device> + <oemStrings> + <entry>Default string</entry> + <entry>ThunderX2 System</entry> + <entry>Ha ha ha try parsing\n + String 3: this correctly + String 4:then</entry> + <entry>Comanche</entry> + </oemStrings> +</sysinfo> diff --git a/tests/sysinfotest.c b/tests/sysinfotest.c index 09cfe6fa45..d8bd1f38ba 100644 --- a/tests/sysinfotest.c +++ b/tests/sysinfotest.c @@ -141,6 +141,7 @@ mymain(void) TEST("aarch64", virSysinfoReadARM); TEST("aarch64-moonshot", virSysinfoReadARM); TEST("aarch64-gigabyte", virSysinfoReadARM); + TEST("aarch64-hpe-apollo", virSysinfoReadARM); return ret; } -- 2.44.2
On some systems, there are two or even more 'OEM Strings' sections in DMI table. Here's an example of dmidecode output on such system: # dmidecode -q -t 11 OEM Strings String 1: Default string OEM Strings String 1: ThunderX2 System String 2: cavium.com String 3: Comanche Now, this poses a problem, because when one tries to obtain individual strings, they get: # dmidecode -q --oem-string 1 Default string ThunderX2 System # dmidecode -q --oem-string 2 No OEM string number 2 cavium.com NB, the "No OEM string number 2" is printed onto stderr and everything else onto stdout. Oh, and trying to get OEM strings from just one section doesn't fly: # dmidecode -q -H 0x1d --oem-string 2 Options --string, --type, --handle and --dump-bin are mutually exclusive This means two things: 1) we have no way of distinguishing OEM strings at the same index but in different sections, 2) because of how virSysinfoDMIDecodeOEMString() is written, we fail in querying OEM string that exists in one section but not in the others (for instance string #2 from example above). While there's not much we can do about 1), there is something that can be done about 2) - refine the error condition and make the function return an error iff there's nothing on stdout and there's something on stderr. Resolves: https://issues.redhat.com/browse/RHEL-45952 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virsysinfo.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index cdc2a7d87b..1fd8261dc1 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -910,9 +910,20 @@ virSysinfoDMIDecodeOEMString(size_t i, /* Unfortunately, dmidecode returns 0 even if OEM String index is out * of bounds, but it prints an error message in that case. Check stderr - * and return success/failure accordingly. */ + * and return success/failure accordingly. + * To make matters worse, if there are two or more 'OEM String' + * sections then: + * + * a) we have no way of distinguishing them as dmidecode prints + * strings from all sections, + * b) if one section contains a valid string, but the other doesn't, + * then stdout contains the valid string and stderr contains the + * error "No OEM string number X*. + * + * Let's just hope there is not many systems like that. + */ - if (err && *err != '\0') + if (!(*str && **str != '\0') && err && *err != '\0') return -1; virStringTrimOptionalNewline(*str); -- 2.44.2
On Thu, Jul 18, 2024 at 16:08:06 +0200, Michal Privoznik wrote: ...
diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index cdc2a7d87b..1fd8261dc1 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -910,9 +910,20 @@ virSysinfoDMIDecodeOEMString(size_t i,
/* Unfortunately, dmidecode returns 0 even if OEM String index is out * of bounds, but it prints an error message in that case. Check stderr - * and return success/failure accordingly. */ + * and return success/failure accordingly. + * To make matters worse, if there are two or more 'OEM String' + * sections then: + * + * a) we have no way of distinguishing them as dmidecode prints + * strings from all sections, + * b) if one section contains a valid string, but the other doesn't, + * then stdout contains the valid string and stderr contains the + * error "No OEM string number X*. + * + * Let's just hope there is not many systems like that. + */
- if (err && *err != '\0') + if (!(*str && **str != '\0') && err && *err != '\0')
I think if ((!*str || **str == '\0') && err && *err != '\0') might be a bit easier for humans to parse. At least for me it is easier :-) Jirka
As can be seen in earlier commits, there can be two OEM strings with the same index. But since our parser (virSysinfoParseOEMStrings()) doesn't expect that, it increments index in each run and thus skips over these strings. Fortunately, we have the right index at hand - we're just skipping over it in a loop. Just reconstruct the index back inside the loop. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virsysinfo.c | 19 ++++++++++++------- .../aarch64-hpe-apollosysinfo.expect | 4 +--- tests/sysinfotest.c | 6 +++++- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index 1fd8261dc1..8823af0469 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -895,13 +895,13 @@ virSysinfoParseX86Chassis(const char *base, static int -virSysinfoDMIDecodeOEMString(size_t i, +virSysinfoDMIDecodeOEMString(unsigned int idx, char **str) { g_autofree char *err = NULL; g_autoptr(virCommand) cmd = virCommandNewArgList(DMIDECODE, "--dump", "--oem-string", NULL); - virCommandAddArgFormat(cmd, "%zu", i); + virCommandAddArgFormat(cmd, "%u", idx); virCommandSetOutputBuffer(cmd, str); virCommandSetErrorBuffer(cmd, &err); @@ -937,7 +937,6 @@ virSysinfoParseOEMStrings(const char *base, virSysinfoOEMStringsDef **stringsRet) { virSysinfoOEMStringsDef *strings = NULL; - size_t i = 1; int ret = -1; const char *cur; @@ -947,6 +946,8 @@ virSysinfoParseOEMStrings(const char *base, strings = g_new0(virSysinfoOEMStringsDef, 1); while ((cur = strstr(cur, "String "))) { + char *collon = NULL; + unsigned int idx = 0; char *eol; cur += 7; @@ -957,8 +958,13 @@ virSysinfoParseOEMStrings(const char *base, goto cleanup; } - while (g_ascii_isdigit(*cur)) - cur++; + if (virStrToLong_ui(cur, &collon, 10, &idx) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Malformed output of dmidecode")); + goto cleanup; + } + + cur = collon; if (*cur != ':') { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -981,7 +987,7 @@ virSysinfoParseOEMStrings(const char *base, if (memchr(cur, '.', eol - cur)) { char *str; - if (virSysinfoDMIDecodeOEMString(i, &str) < 0) + if (virSysinfoDMIDecodeOEMString(idx, &str) < 0) goto cleanup; strings->values[strings->nvalues - 1] = g_steal_pointer(&str); @@ -989,7 +995,6 @@ virSysinfoParseOEMStrings(const char *base, strings->values[strings->nvalues - 1] = g_strndup(cur, eol - cur); } - i++; cur = eol; } diff --git a/tests/sysinfodata/aarch64-hpe-apollosysinfo.expect b/tests/sysinfodata/aarch64-hpe-apollosysinfo.expect index 331454834d..2b886f74fd 100644 --- a/tests/sysinfodata/aarch64-hpe-apollosysinfo.expect +++ b/tests/sysinfodata/aarch64-hpe-apollosysinfo.expect @@ -82,9 +82,7 @@ <oemStrings> <entry>Default string</entry> <entry>ThunderX2 System</entry> - <entry>Ha ha ha try parsing\n - String 3: this correctly - String 4:then</entry> + <entry>Some valid OEM string</entry> <entry>Comanche</entry> </oemStrings> </sysinfo> diff --git a/tests/sysinfotest.c b/tests/sysinfotest.c index d8bd1f38ba..b666847379 100644 --- a/tests/sysinfotest.c +++ b/tests/sysinfotest.c @@ -63,7 +63,11 @@ testDMIDecodeDryRun(const char *const*args G_GNUC_UNUSED, return; } - if (STREQ(args[3], "3")) { + if (STREQ(args[3], "2")) { + *output = g_strdup("Some valid OEM string\n"); + *error = g_strdup_printf("No OEM string number %s", args[3]); + return; + } else if (STREQ(args[3], "3")) { *output = g_strdup("Ha ha ha try parsing\\n\n" " String 3: this correctly\n" " String 4:then\n"); -- 2.44.2
On Thu, Jul 18, 2024 at 16:08:02 +0200, Michal Privoznik wrote:
See 4/5 for explanation.
Michal Prívozník (5): vircommand: Initialize dryRunStatus to portable EXIT_SUCCESS instead of 0 virsysinfo: Trim newline when decoding OEM strings tests: Add HPE Apollo test case to sysinfotest virsysinfo: Be more forgiving when decoding OEM strings virsysinfo: Calculate OEM string index better
src/util/vircommand.c | 2 +- src/util/virsysinfo.c | 36 +++- .../aarch64-hpe-apollosysinfo.data | 162 ++++++++++++++++++ .../aarch64-hpe-apollosysinfo.expect | 88 ++++++++++ tests/sysinfotest.c | 9 +- 5 files changed, 285 insertions(+), 12 deletions(-) create mode 100644 tests/sysinfodata/aarch64-hpe-apollosysinfo.data create mode 100644 tests/sysinfodata/aarch64-hpe-apollosysinfo.expect
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
participants (2)
-
Jiri Denemark -
Michal Privoznik