Virtually every variable defined in the function can be freed automatically when going out of scope. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virsysinfo.c | 36 +++++++++++++----------------------- 1 file changed, 13 insertions(+), 23 deletions(-) diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index 41f4d1cff9..296a2e2cc1 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -1119,10 +1119,10 @@ virSysinfoParseX86Memory(const char *base, virSysinfoDefPtr ret) virSysinfoDefPtr virSysinfoReadDMI(void) { - char *path; - virSysinfoDefPtr ret = NULL; - char *outbuf = NULL; - virCommandPtr cmd; + g_autofree char *path = NULL; + g_auto(virSysinfoDefPtr) ret = NULL; + g_autofree char *outbuf = NULL; + g_autoptr(virCommand) cmd = NULL; path = virFindFileInPath(SYSINFO_SMBIOS_DECODER); if (path == NULL) { @@ -1133,48 +1133,38 @@ virSysinfoReadDMI(void) } cmd = virCommandNewArgList(path, "-q", "-t", "0,1,2,3,4,17", NULL); - VIR_FREE(path); virCommandSetOutputBuffer(cmd, &outbuf); if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return NULL; if (VIR_ALLOC(ret) < 0) - goto error; + return NULL; ret->type = VIR_SYSINFO_SMBIOS; if (virSysinfoParseBIOS(outbuf, &ret->bios) < 0) - goto error; + return NULL; if (virSysinfoParseX86System(outbuf, &ret->system) < 0) - goto error; + return NULL; if (virSysinfoParseX86BaseBoard(outbuf, &ret->baseBoard, &ret->nbaseBoard) < 0) - goto error; + return NULL; if (virSysinfoParseX86Chassis(outbuf, &ret->chassis) < 0) - goto error; + return NULL; ret->nprocessor = 0; ret->processor = NULL; if (virSysinfoParseX86Processor(outbuf, ret) < 0) - goto error; + return NULL; ret->nmemory = 0; ret->memory = NULL; if (virSysinfoParseX86Memory(outbuf, ret) < 0) - goto error; + return NULL; - cleanup: - VIR_FREE(outbuf); - virCommandFree(cmd); - - return ret; - - error: - virSysinfoDefFree(ret); - ret = NULL; - goto cleanup; + return g_steal_pointer(&ret); } -- 2.26.2

Some variables defined in the function can be freed automatically when going out of scope. This renders @result variable and cleanup label needless. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tests/sysinfotest.c | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/tests/sysinfotest.c b/tests/sysinfotest.c index a8a0d0869e..4807c5b1ba 100644 --- a/tests/sysinfotest.c +++ b/tests/sysinfotest.c @@ -47,34 +47,24 @@ struct testSysinfoData { static int testSysinfo(const void *data) { - int result = -1; const char *sysfsActualData; - virSysinfoDefPtr ret = NULL; - virBuffer buf = VIR_BUFFER_INITIALIZER; + g_auto(virSysinfoDefPtr) ret = NULL; + g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; const struct testSysinfoData *testdata = data; virSysinfoSetup(testdata->decoder, testdata->sysinfo, testdata->cpuinfo); if (!testdata->expected || !(ret = testdata->func())) - goto cleanup; + return -1; if (virSysinfoFormat(&buf, ret) < 0) - goto cleanup; + return -1; if (!(sysfsActualData = virBufferCurrentContent(&buf))) - goto cleanup; + return -1; - if (virTestCompareToFile(sysfsActualData, testdata->expected) < 0) - goto cleanup; - - result = 0; - - cleanup: - virSysinfoDefFree(ret); - virBufferFreeAndReset(&buf); - - return result; + return virTestCompareToFile(sysfsActualData, testdata->expected); } static int -- 2.26.2

Problem with custom dmidecode scripts is that they are hard to modify, especially if we will want them to act differently based on passed arguments. So far, we have two scripts which do no more than 'cat $sysinfo' where $sysinfo is saved dmidecode output. The virCommandSetDryRun() can be used to trick virSysinfoReadDMI() thinking it executed real dmidecode. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virsysinfo.c | 3 +- .../sysinfodata/aarch64-gigabytedmidecode.sh | 3 -- tests/sysinfodata/x86dmidecode.sh | 3 -- tests/sysinfotest.c | 51 +++++++++++++------ 4 files changed, 38 insertions(+), 22 deletions(-) delete mode 100755 tests/sysinfodata/aarch64-gigabytedmidecode.sh delete mode 100755 tests/sysinfodata/x86dmidecode.sh diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index 0f1210ab37..0bf80c339b 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -60,7 +60,8 @@ virSysinfoSetup(const char *dmidecode, const char *sysinfo, const char *cpuinfo) { - sysinfoDmidecode = dmidecode; + if (dmidecode) + sysinfoDmidecode = dmidecode; sysinfoSysinfo = sysinfo; sysinfoCpuinfo = cpuinfo; } diff --git a/tests/sysinfodata/aarch64-gigabytedmidecode.sh b/tests/sysinfodata/aarch64-gigabytedmidecode.sh deleted file mode 100755 index 202918103c..0000000000 --- a/tests/sysinfodata/aarch64-gigabytedmidecode.sh +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/sh -DATAFILE=`dirname $0`/aarch64-gigabytesysinfo.data -cat $DATAFILE diff --git a/tests/sysinfodata/x86dmidecode.sh b/tests/sysinfodata/x86dmidecode.sh deleted file mode 100755 index 28aed61459..0000000000 --- a/tests/sysinfodata/x86dmidecode.sh +++ /dev/null @@ -1,3 +0,0 @@ -#!/bin/sh -DATAFILE=`dirname $0`/x86sysinfo.data -cat $DATAFILE diff --git a/tests/sysinfotest.c b/tests/sysinfotest.c index 558dd60294..f080569730 100644 --- a/tests/sysinfotest.c +++ b/tests/sysinfotest.c @@ -34,14 +34,39 @@ #define LIBVIRT_VIRSYSINFOPRIV_H_ALLOW #include "virsysinfopriv.h" +#define LIBVIRT_VIRCOMMANDPRIV_H_ALLOW +#include "vircommandpriv.h" + #define VIR_FROM_THIS VIR_FROM_NONE struct testSysinfoData { const char *name; /* test name, also base name for result files */ virSysinfoDefPtr (*func)(void); /* sysinfo gathering function */ - const char *decoder; /* name of dmi decoder binary/script */ }; + +static void +testDMIDecodeDryRun(const char *const*args G_GNUC_UNUSED, + const char *const*env G_GNUC_UNUSED, + const char *input G_GNUC_UNUSED, + char **output, + char **error, + int *status, + void *opaque) +{ + const char *sysinfo = opaque; + + if (virFileReadAll(sysinfo, 10 * 1024 * 1024, output) < 0) { + *error = g_strdup(virGetLastErrorMessage()); + *status = EXIT_FAILURE; + return; + } + + *error = g_strdup(""); + *status = 0; +} + + static int testSysinfo(const void *data) { @@ -52,18 +77,19 @@ testSysinfo(const void *data) g_autofree char *sysinfo = NULL; g_autofree char *cpuinfo = NULL; g_autofree char *expected = NULL; - g_autofree char *decoder = NULL; sysinfo = g_strdup_printf("%s/sysinfodata/%ssysinfo.data", abs_srcdir, testdata->name); cpuinfo = g_strdup_printf("%s/sysinfodata/%scpuinfo.data", abs_srcdir, testdata->name); expected = g_strdup_printf("%s/sysinfodata/%ssysinfo.expect", abs_srcdir, testdata->name); - if (testdata->decoder) - decoder = g_strdup_printf("%s/%s", abs_srcdir, testdata->decoder); + virCommandSetDryRun(NULL, testDMIDecodeDryRun, sysinfo); - virSysinfoSetup(decoder, sysinfo, cpuinfo); + virSysinfoSetup(NULL, sysinfo, cpuinfo); - if (!(ret = testdata->func())) + ret = testdata->func(); + virCommandSetDryRun(NULL, NULL, NULL); + + if (!ret) return -1; if (virSysinfoFormat(&buf, ret) < 0) @@ -76,17 +102,13 @@ testSysinfo(const void *data) } -#define TEST_FULL(name, func, decoder) \ +#define TEST(name, func) \ do { \ - struct testSysinfoData data = { name, func, decoder }; \ + struct testSysinfoData data = { name, func }; \ if (virTestRun(name " sysinfo", testSysinfo, &data) < 0) \ ret = EXIT_FAILURE; \ } while (0) - -#define TEST(name, func) \ - TEST_FULL(name, func, NULL) - static int mymain(void) { @@ -95,13 +117,12 @@ mymain(void) TEST("s390", virSysinfoReadS390); TEST("s390-freq", virSysinfoReadS390); TEST("ppc", virSysinfoReadPPC); - TEST_FULL("x86", virSysinfoReadDMI, "/sysinfodata/x86dmidecode.sh"); + TEST("x86", virSysinfoReadDMI); TEST("arm", virSysinfoReadARM); TEST("arm-rpi2", virSysinfoReadARM); TEST("aarch64", virSysinfoReadARM); TEST("aarch64-moonshot", virSysinfoReadARM); - TEST_FULL("aarch64-gigabyte", virSysinfoReadARM, - "/sysinfodata/aarch64-gigabytedmidecode.sh"); + TEST("aarch64-gigabyte", virSysinfoReadARM); return ret; } -- 2.26.2

Setting OEM strings for a domain was introduced in v4.1.0-rc1~315. However, any application that wanted to use them (e.g. to point to an URL where a config file is stored) had to 'dmidecode -u --oem-string N' (where N is index of the string). Well, we can expose them under our <sysinfo/> XML and if the domain is running Libvirt inside it can be obtained using virConnectGetSysinfo() API. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virsysinfo.c | 102 +++++++++++++++++++++++++++- tests/sysinfodata/x86sysinfo.data | 6 ++ tests/sysinfodata/x86sysinfo.expect | 8 +++ tests/sysinfotest.c | 27 ++++++-- 4 files changed, 138 insertions(+), 5 deletions(-) diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index a26c27e83e..09e32df6a9 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -915,6 +915,103 @@ virSysinfoParseX86Chassis(const char *base, } +static int +virSysinfoDMIDecodeOEMString(size_t i, + char **str) +{ + g_autofree char *err = NULL; + g_autoptr(virCommand) cmd = virCommandNewArgList(DMIDECODE, "--dump", + "--oem-string", NULL); + virCommandAddArgFormat(cmd, "%zu", i); + virCommandSetOutputBuffer(cmd, str); + virCommandSetErrorBuffer(cmd, &err); + + if (virCommandRun(cmd, NULL) < 0) + return -1; + + /* 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. */ + + if (err && *err != '\0') + return -1; + + return 0; +} + + +static int +virSysinfoParseOEMStrings(const char *base, + virSysinfoOEMStringsDefPtr *stringsRet) +{ + virSysinfoOEMStringsDefPtr strings = NULL; + size_t i = 1; + int ret = -1; + const char *cur; + + if (!(cur = strstr(base, "OEM Strings"))) + return 0; + + if (VIR_ALLOC(strings) < 0) + return -1; + + while ((cur = strstr(cur, "String "))) { + char *eol; + + cur += 7; + + if (!(eol = strchr(cur, '\n'))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Malformed output of dmidecode")); + goto cleanup; + } + + while (g_ascii_isdigit(*cur)) + cur++; + + if (*cur != ':') { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Malformed output of dmidecode")); + goto cleanup; + } + + cur += 2; + + virSkipSpacesBackwards(cur, &eol); + if (!eol) + continue; + + if (VIR_EXPAND_N(strings->values, strings->nvalues, 1) < 0) + goto cleanup; + + /* If OEM String contains newline, dmidecode escapes it as a dot. + * If this is the case then run dmidecode again to get raw string. + * Unfortunately, we can't dinstinguish betwen dot an new line at + * this level. */ + if (memchr(cur, '.', eol - cur)) { + char *str; + + if (virSysinfoDMIDecodeOEMString(i, &str) < 0) + goto cleanup; + + strings->values[strings->nvalues - 1] = g_steal_pointer(&str); + } else { + strings->values[strings->nvalues - 1] = g_strndup(cur, eol - cur); + } + + i++; + cur = eol; + } + + *stringsRet = g_steal_pointer(&strings); + ret = 0; + + cleanup: + virSysinfoOEMStringsDefFree(strings); + return ret; +} + + static int virSysinfoParseX86Processor(const char *base, virSysinfoDefPtr ret) { @@ -1119,7 +1216,7 @@ virSysinfoReadDMI(void) g_autofree char *outbuf = NULL; g_autoptr(virCommand) cmd = NULL; - cmd = virCommandNewArgList(DMIDECODE, "-q", "-t", "0,1,2,3,4,17", NULL); + cmd = virCommandNewArgList(DMIDECODE, "-q", "-t", "0,1,2,3,4,11,17", NULL); virCommandSetOutputBuffer(cmd, &outbuf); if (virCommandRun(cmd, NULL) < 0) return NULL; @@ -1141,6 +1238,9 @@ virSysinfoReadDMI(void) if (virSysinfoParseX86Chassis(outbuf, &ret->chassis) < 0) return NULL; + if (virSysinfoParseOEMStrings(outbuf, &ret->oemStrings) < 0) + return NULL; + ret->nprocessor = 0; ret->processor = NULL; if (virSysinfoParseX86Processor(outbuf, ret) < 0) diff --git a/tests/sysinfodata/x86sysinfo.data b/tests/sysinfodata/x86sysinfo.data index 426261041d..3f0b654e4b 100644 --- a/tests/sysinfodata/x86sysinfo.data +++ b/tests/sysinfodata/x86sysinfo.data @@ -81,3 +81,9 @@ Memory Device Serial Number: 29057112 Asset Tag: 0839 Part Number: IMSH2GS13A1F1C-10F + +OEM Strings + String 1: Hello + String 2: World + String 3: Ha ha ha try parsing\n. String 3: this correctly. String 4:then + String 4: This is, more tricky value=escaped diff --git a/tests/sysinfodata/x86sysinfo.expect b/tests/sysinfodata/x86sysinfo.expect index fcdd790cbd..05add8f031 100644 --- a/tests/sysinfodata/x86sysinfo.expect +++ b/tests/sysinfodata/x86sysinfo.expect @@ -50,4 +50,12 @@ <entry name='serial_number'>29057112</entry> <entry name='part_number'>IMSH2GS13A1F1C-10F</entry> </memory_device> + <oemStrings> + <entry>Hello</entry> + <entry>World</entry> + <entry>Ha ha ha try parsing\n + String 3: this correctly + String 4:then</entry> + <entry>This is, more tricky value=escaped</entry> + </oemStrings> </sysinfo> diff --git a/tests/sysinfotest.c b/tests/sysinfotest.c index 10d24b823a..3b418955d0 100644 --- a/tests/sysinfotest.c +++ b/tests/sysinfotest.c @@ -56,10 +56,29 @@ testDMIDecodeDryRun(const char *const*args G_GNUC_UNUSED, { const char *sysinfo = opaque; - if (virFileReadAll(sysinfo, 10 * 1024 * 1024, output) < 0) { - *error = g_strdup(virGetLastErrorMessage()); - *status = EXIT_FAILURE; - return; + if (STREQ_NULLABLE(args[1], "--dump") && + STREQ_NULLABLE(args[2], "--oem-string")) { + if (!args[3]) { + *error = g_strdup("dmidecode: option '--oem-string' requires an argument"); + *status = EXIT_FAILURE; + return; + } + + if (STREQ(args[3], "3")) { + *output = g_strdup("Ha ha ha try parsing\\n\n" + " String 3: this correctly\n" + " String 4:then"); + } else { + *error = g_strdup_printf("No OEM string number %s", args[3]); + *status = EXIT_FAILURE; + return; + } + } else { + if (virFileReadAll(sysinfo, 10 * 1024 * 1024, output) < 0) { + *error = g_strdup(virGetLastErrorMessage()); + *status = EXIT_FAILURE; + return; + } } *error = g_strdup(""); -- 2.26.2