[libvirt] [PATCHv2 0/5] smbios cleanups
No change to the first three patches (but reposting so they'll all be in the same thread). The last two patches are new. Eric Blake (5): qemu: avoid adding "" in smbios arguments smbios: support system family smbios: allow () in smbios strings uuid: require smbios uuid and domain uuid to match sysinfo: convert to virCommand docs/schemas/domain.rng | 3 +- src/conf/domain_conf.c | 31 ++++++++++++- src/qemu/qemu_conf.c | 40 ++++++++++------ src/util/sysinfo.c | 57 +++++++---------------- src/util/sysinfo.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-smbios.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-smbios.xml | 20 ++++---- 7 files changed, 85 insertions(+), 69 deletions(-) -- 1.7.3.2
The log lists things like -smbios type=1,vendor="Red Hat", which is great for shell parsing, but not so great when you realize that execve() then passes those literal "" on as part of the command line argument, such that qemu sets SMBIOS with extra literal quotes. The eventual addition of virCommand is needed before we have the API to shell-quote a string representation of a command line, so that the log can still be pasted into a shell, but without inserting extra bytes into the execve() arguments. * src/qemu/qemu_conf.c (qemuBuildSmbiosBiosStr) (qemuBuildSmbiosSystemStr): Qemu doesn't like quotes around uuid arguments, and the remaining quotes are passed literally to smbios, making <smbios mode='host'/> inaccurate. Removing the quotes makes the log harder to parse, but that can be fixed later with virCommand improvements. * tests/qemuxml2argvdata/qemuxml2argv-smbios.args: 'Fix' test; it will need fixing again once virCommand learns how to shell-quote a potential command line. --- src/qemu/qemu_conf.c | 20 ++++++++++---------- tests/qemuxml2argvdata/qemuxml2argv-smbios.args | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 925585a..8985241 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -3622,16 +3622,16 @@ static char *qemuBuildSmbiosBiosStr(virSysinfoDefPtr def) /* 0:Vendor */ if (def->bios_vendor) - virBufferVSprintf(&buf, ",vendor=\"%s\"", def->bios_vendor); + virBufferVSprintf(&buf, ",vendor=%s", def->bios_vendor); /* 0:BIOS Version */ if (def->bios_version) - virBufferVSprintf(&buf, ",version=\"%s\"", def->bios_version); + virBufferVSprintf(&buf, ",version=%s", def->bios_version); /* 0:BIOS Release Date */ if (def->bios_date) - virBufferVSprintf(&buf, ",date=\"%s\"", def->bios_date); + virBufferVSprintf(&buf, ",date=%s", def->bios_date); /* 0:System BIOS Major Release and 0:System BIOS Minor Release */ if (def->bios_release) - virBufferVSprintf(&buf, ",release=\"%s\"", def->bios_release); + virBufferVSprintf(&buf, ",release=%s", def->bios_release); if (virBufferError(&buf)) { virReportOOMError(); @@ -3658,23 +3658,23 @@ static char *qemuBuildSmbiosSystemStr(virSysinfoDefPtr def) /* 1:Manufacturer */ if (def->system_manufacturer) - virBufferVSprintf(&buf, ",manufacturer=\"%s\"", + virBufferVSprintf(&buf, ",manufacturer=%s", def->system_manufacturer); /* 1:Product Name */ if (def->system_product) - virBufferVSprintf(&buf, ",product=\"%s\"", def->system_product); + virBufferVSprintf(&buf, ",product=%s", def->system_product); /* 1:Version */ if (def->system_version) - virBufferVSprintf(&buf, ",version=\"%s\"", def->system_version); + virBufferVSprintf(&buf, ",version=%s", def->system_version); /* 1:Serial Number */ if (def->system_serial) - virBufferVSprintf(&buf, ",serial=\"%s\"", def->system_serial); + virBufferVSprintf(&buf, ",serial=%s", def->system_serial); /* 1:UUID */ if (def->system_uuid) - virBufferVSprintf(&buf, ",uuid=\"%s\"", def->system_uuid); + virBufferVSprintf(&buf, ",uuid=%s", def->system_uuid); /* 1:SKU Number */ if (def->system_sku) - virBufferVSprintf(&buf, ",sku=\"%s\"", def->system_sku); + virBufferVSprintf(&buf, ",sku=%s", def->system_sku); if (virBufferError(&buf)) { virReportOOMError(); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios.args b/tests/qemuxml2argvdata/qemuxml2argv-smbios.args index d5bd289..bd3ede4 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-smbios.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -smbios type=0,vendor="QEmu/KVM",version="0.13" -smbios type=1,manufacturer="Fedora",product="Virt-Manager",version="0.8.2-3.fc14",serial="32dfcb37-5af1-552b-357c-be8c3aa38310",uuid="c7a5fdbd-edaf-9455-926a-d65c16db1809" -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -smbios type=0,vendor=QEmu/KVM,version=0.13 -smbios type=1,manufacturer=Fedora,product=Virt-Manager,version=0.8.2-3.fc14,serial=32dfcb37-5af1-552b-357c-be8c3aa38310,uuid=c7a5fdbd-edaf-9455-926a-d65c16db1809 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -- 1.7.3.2
On Fri, Dec 03, 2010 at 02:56:14PM -0700, Eric Blake wrote:
The log lists things like -smbios type=1,vendor="Red Hat", which is great for shell parsing, but not so great when you realize that execve() then passes those literal "" on as part of the command line argument, such that qemu sets SMBIOS with extra literal quotes.
Hum, I was afraid that QEmu parsing would fail in case of spaces if there is no quote, but if you checked this, sure !
src/qemu/qemu_conf.c | 20 ++++++++++---------- tests/qemuxml2argvdata/qemuxml2argv-smbios.args | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 925585a..8985241 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -3622,16 +3622,16 @@ static char *qemuBuildSmbiosBiosStr(virSysinfoDefPtr def)
/* 0:Vendor */ if (def->bios_vendor) - virBufferVSprintf(&buf, ",vendor=\"%s\"", def->bios_vendor); + virBufferVSprintf(&buf, ",vendor=%s", def->bios_vendor); /* 0:BIOS Version */ if (def->bios_version) - virBufferVSprintf(&buf, ",version=\"%s\"", def->bios_version); + virBufferVSprintf(&buf, ",version=%s", def->bios_version); /* 0:BIOS Release Date */ if (def->bios_date) - virBufferVSprintf(&buf, ",date=\"%s\"", def->bios_date); + virBufferVSprintf(&buf, ",date=%s", def->bios_date); /* 0:System BIOS Major Release and 0:System BIOS Minor Release */ if (def->bios_release) - virBufferVSprintf(&buf, ",release=\"%s\"", def->bios_release); + virBufferVSprintf(&buf, ",release=%s", def->bios_release);
if (virBufferError(&buf)) { virReportOOMError(); @@ -3658,23 +3658,23 @@ static char *qemuBuildSmbiosSystemStr(virSysinfoDefPtr def)
/* 1:Manufacturer */ if (def->system_manufacturer) - virBufferVSprintf(&buf, ",manufacturer=\"%s\"", + virBufferVSprintf(&buf, ",manufacturer=%s", def->system_manufacturer); /* 1:Product Name */ if (def->system_product) - virBufferVSprintf(&buf, ",product=\"%s\"", def->system_product); + virBufferVSprintf(&buf, ",product=%s", def->system_product); /* 1:Version */ if (def->system_version) - virBufferVSprintf(&buf, ",version=\"%s\"", def->system_version); + virBufferVSprintf(&buf, ",version=%s", def->system_version); /* 1:Serial Number */ if (def->system_serial) - virBufferVSprintf(&buf, ",serial=\"%s\"", def->system_serial); + virBufferVSprintf(&buf, ",serial=%s", def->system_serial); /* 1:UUID */ if (def->system_uuid) - virBufferVSprintf(&buf, ",uuid=\"%s\"", def->system_uuid); + virBufferVSprintf(&buf, ",uuid=%s", def->system_uuid); /* 1:SKU Number */ if (def->system_sku) - virBufferVSprintf(&buf, ",sku=\"%s\"", def->system_sku); + virBufferVSprintf(&buf, ",sku=%s", def->system_sku);
if (virBufferError(&buf)) { virReportOOMError(); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios.args b/tests/qemuxml2argvdata/qemuxml2argv-smbios.args index d5bd289..bd3ede4 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-smbios.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -smbios type=0,vendor="QEmu/KVM",version="0.13" -smbios type=1,manufacturer="Fedora",product="Virt-Manager",version="0.8.2-3.fc14",serial="32dfcb37-5af1-552b-357c-be8c3aa38310",uuid="c7a5fdbd-edaf-9455-926a-d65c16db1809" -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -smbios type=0,vendor=QEmu/KVM,version=0.13 -smbios type=1,manufacturer=Fedora,product=Virt-Manager,version=0.8.2-3.fc14,serial=32dfcb37-5af1-552b-357c-be8c3aa38310,uuid=c7a5fdbd-edaf-9455-926a-d65c16db1809 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 12/05/2010 12:57 AM, Daniel Veillard wrote:
On Fri, Dec 03, 2010 at 02:56:14PM -0700, Eric Blake wrote:
The log lists things like -smbios type=1,vendor="Red Hat", which is great for shell parsing, but not so great when you realize that execve() then passes those literal "" on as part of the command line argument, such that qemu sets SMBIOS with extra literal quotes.
Hum, I was afraid that QEmu parsing would fail in case of spaces if there is no quote, but if you checked this, sure !
What's happening here is that we are building up execve arguments, and supplying roughly: "qemu" "-smbios" "type=0,vendor=\"Red Hat\",version=\"Fedora 14\"" instead of the intended: "qemu" "-smbios" "type=0,vendor=Red Hat,version=Fedora 14" Although qemu uses a hand-rolled loop instead of getsubopt(), it looks like qemu is using the same algorithm as getsubopt, where it parses everything between '=' and ',', including spaces, as the subopt argument. At any rate, yes, I did test this; the only thing you can't pass through qemu's -smbios is a literal comma, but that's already excluded from our domain.rng schema. :)
ACK,
Thanks; I've pushed 1 through 4; I'm waiting to push 5 until after my virCommand buffer patches have been ACK'd, so as to avoid any question of any potential NULL dereferences due to the virCommandSetOutputBuffer calls. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* docs/schemas/domain.rng (sysinfo-system-name): Also allow family. * src/util/sysinfo.h (struct _virSysinfoDef): Add system_family. * src/conf/domain_conf.c (virSysinfoParseXML) (virDomainSysinfoDefFormat): Support it. * src/util/sysinfo.c (virSysinfoDefFree, virSysinfoRead): Likewise. * src/qemu/qemu_conf.c (qemuBuildSmbiosSystemStr): Likewise. * tests/qemuxml2argvdata/qemuxml2argv-smbios.xml: Adjust test. * tests/qemuxml2argvdata/qemuxml2argv-smbios.args: Likewise. --- docs/schemas/domain.rng | 1 + src/conf/domain_conf.c | 9 ++++++++- src/qemu/qemu_conf.c | 6 +++++- src/util/sysinfo.c | 7 +++++++ src/util/sysinfo.h | 1 + tests/qemuxml2argvdata/qemuxml2argv-smbios.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-smbios.xml | 2 ++ 7 files changed, 25 insertions(+), 3 deletions(-) diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 08ebefb..3dd7aa3 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -1875,6 +1875,7 @@ <value>serial</value> <value>uuid</value> <value>sku</value> + <value>family</value> </choice> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 9516427..3dfd4ee 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3625,6 +3625,8 @@ virSysinfoParseXML(const xmlNodePtr node, virXPathString("string(system/entry[@name='uuid'])", ctxt); def->system_sku = virXPathString("string(system/entry[@name='sku'])", ctxt); + def->system_family = + virXPathString("string(system/entry[@name='family'])", ctxt); cleanup: VIR_FREE(type); @@ -6425,7 +6427,8 @@ virDomainSysinfoDefFormat(virBufferPtr buf, } if ((def->system_manufacturer != NULL) || (def->system_product != NULL) || (def->system_version != NULL) || (def->system_serial != NULL) || - (def->system_uuid != NULL) || (def->system_sku != NULL)) { + (def->system_uuid != NULL) || (def->system_sku != NULL) || + (def->system_family != NULL)) { virBufferAddLit(buf, " <system>\n"); if (def->system_manufacturer != NULL) virBufferEscapeString(buf, @@ -6451,6 +6454,10 @@ virDomainSysinfoDefFormat(virBufferPtr buf, virBufferEscapeString(buf, " <entry name='sku'>%s</entry>\n", def->system_sku); + if (def->system_family != NULL) + virBufferEscapeString(buf, + " <entry name='family'>%s</entry>\n", + def->system_family); virBufferAddLit(buf, " </system>\n"); } diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 8985241..9a9d924 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -3651,7 +3651,8 @@ static char *qemuBuildSmbiosSystemStr(virSysinfoDefPtr def) if ((def->system_manufacturer == NULL) && (def->system_sku == NULL) && (def->system_product == NULL) && (def->system_uuid == NULL) && - (def->system_version == NULL) && (def->system_serial == NULL)) + (def->system_version == NULL) && (def->system_serial == NULL) && + (def->system_family == NULL)) return(NULL); virBufferAddLit(&buf, "type=1"); @@ -3675,6 +3676,9 @@ static char *qemuBuildSmbiosSystemStr(virSysinfoDefPtr def) /* 1:SKU Number */ if (def->system_sku) virBufferVSprintf(&buf, ",sku=%s", def->system_sku); + /* 1:Family */ + if (def->system_family) + virBufferVSprintf(&buf, ",family=%s", def->system_family); if (virBufferError(&buf)) { virReportOOMError(); diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c index 8ad98fe..cf41773 100644 --- a/src/util/sysinfo.c +++ b/src/util/sysinfo.c @@ -68,6 +68,7 @@ void virSysinfoDefFree(virSysinfoDefPtr def) VIR_FREE(def->system_serial); VIR_FREE(def->system_uuid); VIR_FREE(def->system_sku); + VIR_FREE(def->system_family); VIR_FREE(def); } @@ -217,6 +218,12 @@ virSysinfoRead(void) { if ((eol) && ((ret->system_sku = strndup(cur, eol - cur)) == NULL)) goto no_memory; } + if ((cur = strstr(base, "Family: ")) != NULL) { + cur += 8; + eol = strchr(cur, '\n'); + if ((eol) && ((ret->system_family = strndup(cur, eol - cur)) == NULL)) + goto no_memory; + } cleanup: VIR_FREE(outbuf); diff --git a/src/util/sysinfo.h b/src/util/sysinfo.h index 611d54e..1af7ef6 100644 --- a/src/util/sysinfo.h +++ b/src/util/sysinfo.h @@ -49,6 +49,7 @@ struct _virSysinfoDef { char *system_serial; char *system_uuid; char *system_sku; + char *system_family; }; virSysinfoDefPtr virSysinfoRead(void); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios.args b/tests/qemuxml2argvdata/qemuxml2argv-smbios.args index bd3ede4..b5e4783 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-smbios.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -smbios type=0,vendor=QEmu/KVM,version=0.13 -smbios type=1,manufacturer=Fedora,product=Virt-Manager,version=0.8.2-3.fc14,serial=32dfcb37-5af1-552b-357c-be8c3aa38310,uuid=c7a5fdbd-edaf-9455-926a-d65c16db1809 -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -smbios type=0,vendor=QEmu/KVM,version=0.13 -smbios type=1,manufacturer=Fedora,product=Virt-Manager,version=0.8.2-3.fc14,serial=32dfcb37-5af1-552b-357c-be8c3aa38310,uuid=c7a5fdbd-edaf-9455-926a-d65c16db1809,sku=1234567890,family=Red Hat -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios.xml b/tests/qemuxml2argvdata/qemuxml2argv-smbios.xml index 66cbbb0..45b6dba 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-smbios.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios.xml @@ -15,6 +15,8 @@ <entry name="version">0.8.2-3.fc14</entry> <entry name="serial">32dfcb37-5af1-552b-357c-be8c3aa38310</entry> <entry name="uuid">c7a5fdbd-edaf-9455-926a-d65c16db1809</entry> + <entry name="sku">1234567890</entry> + <entry name="family">Red Hat</entry> </system> </sysinfo> <os> -- 1.7.3.2
On Fri, Dec 03, 2010 at 02:56:15PM -0700, Eric Blake wrote:
* docs/schemas/domain.rng (sysinfo-system-name): Also allow family. * src/util/sysinfo.h (struct _virSysinfoDef): Add system_family. * src/conf/domain_conf.c (virSysinfoParseXML) (virDomainSysinfoDefFormat): Support it. * src/util/sysinfo.c (virSysinfoDefFree, virSysinfoRead): Likewise. * src/qemu/qemu_conf.c (qemuBuildSmbiosSystemStr): Likewise. * tests/qemuxml2argvdata/qemuxml2argv-smbios.xml: Adjust test. * tests/qemuxml2argvdata/qemuxml2argv-smbios.args: Likewise.
ACK, I wasn't sure it was really useful, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
* docs/schemas/domain.rng (sysinf-value): Expand pattern. * tests/qemuxml2argvdata/qemuxml2argv-smbios.xml: Prefer '' over "" for attribute values. Copy real hardware values. * tests/qemuxml2argvdata/qemuxml2argv-smbios.args: Likewise. --- docs/schemas/domain.rng | 2 +- tests/qemuxml2argvdata/qemuxml2argv-smbios.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-smbios.xml | 22 +++++++++++----------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 3dd7aa3..811d559 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -1881,7 +1881,7 @@ <define name="sysinfo-value"> <data type="string"> - <param name='pattern'>[a-zA-Z0-9/\-_\. ]+</param> + <param name='pattern'>[a-zA-Z0-9/\-_\. \(\)]+</param> </data> </define> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios.args b/tests/qemuxml2argvdata/qemuxml2argv-smbios.args index b5e4783..12ef04f 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-smbios.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios.args @@ -1 +1 @@ -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -smbios type=0,vendor=QEmu/KVM,version=0.13 -smbios type=1,manufacturer=Fedora,product=Virt-Manager,version=0.8.2-3.fc14,serial=32dfcb37-5af1-552b-357c-be8c3aa38310,uuid=c7a5fdbd-edaf-9455-926a-d65c16db1809,sku=1234567890,family=Red Hat -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -smbios type=0,vendor=LENOVO,version=6FET82WW (3.12 ) -smbios type=1,manufacturer=Fedora,product=Virt-Manager,version=0.8.2-3.fc14,serial=32dfcb37-5af1-552b-357c-be8c3aa38310,uuid=c7a5fdbd-edaf-9455-926a-d65c16db1809,sku=1234567890,family=Red Hat -nographic -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios.xml b/tests/qemuxml2argvdata/qemuxml2argv-smbios.xml index 45b6dba..23ec1a7 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-smbios.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios.xml @@ -4,25 +4,25 @@ <memory>219200</memory> <currentMemory>219200</currentMemory> <vcpu>1</vcpu> - <sysinfo type="smbios"> + <sysinfo type='smbios'> <bios> - <entry name="vendor">QEmu/KVM</entry> - <entry name="version">0.13</entry> + <entry name='vendor'>LENOVO</entry> + <entry name='version'>6FET82WW (3.12 )</entry> </bios> <system> - <entry name="manufacturer">Fedora</entry> - <entry name="product">Virt-Manager</entry> - <entry name="version">0.8.2-3.fc14</entry> - <entry name="serial">32dfcb37-5af1-552b-357c-be8c3aa38310</entry> - <entry name="uuid">c7a5fdbd-edaf-9455-926a-d65c16db1809</entry> - <entry name="sku">1234567890</entry> - <entry name="family">Red Hat</entry> + <entry name='manufacturer'>Fedora</entry> + <entry name='product'>Virt-Manager</entry> + <entry name='version'>0.8.2-3.fc14</entry> + <entry name='serial'>32dfcb37-5af1-552b-357c-be8c3aa38310</entry> + <entry name='uuid'>c7a5fdbd-edaf-9455-926a-d65c16db1809</entry> + <entry name='sku'>1234567890</entry> + <entry name='family'>Red Hat</entry> </system> </sysinfo> <os> <type arch='i686' machine='pc'>hvm</type> <boot dev='hd'/> - <smbios mode="sysinfo"/> + <smbios mode='sysinfo'/> </os> <clock offset='utc'/> <on_poweroff>destroy</on_poweroff> -- 1.7.3.2
On Fri, Dec 03, 2010 at 02:56:16PM -0700, Eric Blake wrote:
* docs/schemas/domain.rng (sysinf-value): Expand pattern. * tests/qemuxml2argvdata/qemuxml2argv-smbios.xml: Prefer '' over "" for attribute values. Copy real hardware values. * tests/qemuxml2argvdata/qemuxml2argv-smbios.args: Likewise. --- docs/schemas/domain.rng | 2 +- tests/qemuxml2argvdata/qemuxml2argv-smbios.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-smbios.xml | 22 +++++++++++----------- 3 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 3dd7aa3..811d559 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -1881,7 +1881,7 @@
<define name="sysinfo-value"> <data type="string"> - <param name='pattern'>[a-zA-Z0-9/\-_\. ]+</param> + <param name='pattern'>[a-zA-Z0-9/\-_\. \(\)]+</param> </data> </define>
ACK, good point ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
* src/conf/domain_conf.c (virDomainDefParseXML): Prefer sysinfo uuid over generating one, and if both uuids are present, require them to be identical. * src/qemu/qemu_conf.c (qemuBuildSmbiosSystemStr): Allow skipping the uuid. (qemudBuildCommandLine): Adjust caller; <smbios mode=host/> must not use host uuid in place of guest uuid. --- The qemu developers pointed out that '-uuid xyz' and '-smbios type=1,uuid=xyz' both have the same effect. They argued that -uuid is deprecated, but it's easier for libvirt to always supply it. Meanwhile, they were clear that if both values were supplied, the two uuids had better be identical (current qemu favors -uuid over -smbios, but it's not guaranteed to stay that way in the future). src/conf/domain_conf.c | 22 +++++++++++++++++++++- src/qemu/qemu_conf.c | 18 +++++++++++------- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3dfd4ee..9c54a59 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4498,6 +4498,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, long id = -1; virDomainDefPtr def; unsigned long count; + bool uuid_generated = false; if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -4529,7 +4530,9 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, goto error; } - /* Extract domain uuid */ + /* Extract domain uuid. If both uuid and sysinfo/system/entry/uuid + * exist, they must match; and if only the latter exists, it can + * also serve as the uuid. */ tmp = virXPathString("string(./uuid[1])", ctxt); if (!tmp) { if (virUUIDGenerate(def->uuid)) { @@ -4537,6 +4540,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, "%s", _("Failed to generate UUID")); goto error; } + uuid_generated = true; } else { if (virUUIDParse(tmp, def->uuid) < 0) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -5279,6 +5283,22 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, if (def->sysinfo == NULL) goto error; + if (def->sysinfo->system_uuid != NULL) { + unsigned char uuidbuf[VIR_UUID_BUFLEN]; + if (virUUIDParse(def->sysinfo->system_uuid, uuidbuf) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("malformed uuid element")); + goto error; + } + if (uuid_generated) + memcpy(def->uuid, uuidbuf, VIR_UUID_BUFLEN); + else if (memcmp(def->uuid, uuidbuf, VIR_UUID_BUFLEN) != 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("UUID mismatch between <uuid> and " + "<sysinfo>")); + goto error; + } + } } tmp = virXPathString("string(./os/smbios/@mode)", ctxt); if (tmp) { diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 9a9d924..9ca1dad 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -3645,15 +3645,15 @@ error: return(NULL); } -static char *qemuBuildSmbiosSystemStr(virSysinfoDefPtr def) +static char *qemuBuildSmbiosSystemStr(virSysinfoDefPtr def, bool skip_uuid) { virBuffer buf = VIR_BUFFER_INITIALIZER; if ((def->system_manufacturer == NULL) && (def->system_sku == NULL) && - (def->system_product == NULL) && (def->system_uuid == NULL) && - (def->system_version == NULL) && (def->system_serial == NULL) && - (def->system_family == NULL)) - return(NULL); + (def->system_product == NULL) && (def->system_version == NULL) && + (def->system_serial == NULL) && (def->system_family == NULL) && + (def->system_uuid == NULL || skip_uuid)) + return NULL; virBufferAddLit(&buf, "type=1"); @@ -3671,7 +3671,7 @@ static char *qemuBuildSmbiosSystemStr(virSysinfoDefPtr def) if (def->system_serial) virBufferVSprintf(&buf, ",serial=%s", def->system_serial); /* 1:UUID */ - if (def->system_uuid) + if (def->system_uuid && !skip_uuid) virBufferVSprintf(&buf, ",uuid=%s", def->system_uuid); /* 1:SKU Number */ if (def->system_sku) @@ -4173,6 +4173,7 @@ qemudBuildCommandLine(virConnectPtr conn, if ((def->os.smbios_mode != VIR_DOMAIN_SMBIOS_NONE) && (def->os.smbios_mode != VIR_DOMAIN_SMBIOS_EMULATE)) { virSysinfoDefPtr source = NULL; + bool skip_uuid = false; if (!(qemuCmdFlags & QEMUD_CMD_FLAG_SMBIOS_TYPE)) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -4189,6 +4190,8 @@ qemudBuildCommandLine(virConnectPtr conn, goto error; } source = driver->hostsysinfo; + /* Host and guest uuid must differ, by definition of UUID. */ + skip_uuid = true; } else if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_SYSINFO) { if (def->sysinfo == NULL) { qemuReportError(VIR_ERR_XML_ERROR, @@ -4197,6 +4200,7 @@ qemudBuildCommandLine(virConnectPtr conn, goto error; } source = def->sysinfo; + /* domain_conf guaranteed that system_uuid matches guest uuid. */ } if (source != NULL) { char *smbioscmd; @@ -4206,7 +4210,7 @@ qemudBuildCommandLine(virConnectPtr conn, virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL); VIR_FREE(smbioscmd); } - smbioscmd = qemuBuildSmbiosSystemStr(source); + smbioscmd = qemuBuildSmbiosSystemStr(source, skip_uuid); if (smbioscmd != NULL) { virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL); VIR_FREE(smbioscmd); -- 1.7.3.2
On Fri, Dec 03, 2010 at 02:56:17PM -0700, Eric Blake wrote:
* src/conf/domain_conf.c (virDomainDefParseXML): Prefer sysinfo uuid over generating one, and if both uuids are present, require them to be identical. * src/qemu/qemu_conf.c (qemuBuildSmbiosSystemStr): Allow skipping the uuid. (qemudBuildCommandLine): Adjust caller; <smbios mode=host/> must not use host uuid in place of guest uuid. ---
The qemu developers pointed out that '-uuid xyz' and '-smbios type=1,uuid=xyz' both have the same effect. They argued that -uuid is deprecated, but it's easier for libvirt to always supply it. Meanwhile, they were clear that if both values were supplied, the two uuids had better be identical (current qemu favors -uuid over -smbios, but it's not guaranteed to stay that way in the future).
in practice I ended up at least on one test where -uuid and <smbios mode=host/> failed with the code in qemu complaining that the UUID was not well formed, so yes that's a good thing ! ACK Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
* src/util/sysinfo.c (virSysinfoRead): Use virCommand instead of virExec. --- This patch assumes option 2 of my patch to virCommand guarantees on outbuf when the child process had no output (it needs a slight tweak to avoid a NULL dereference if we go with option 1 for virCommand). src/util/sysinfo.c | 50 ++++++++++---------------------------------------- 1 files changed, 10 insertions(+), 40 deletions(-) diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c index cf41773..f44b112 100644 --- a/src/util/sysinfo.c +++ b/src/util/sysinfo.c @@ -36,6 +36,7 @@ #include "conf/domain_conf.h" #include "logging.h" #include "memory.h" +#include "command.h" #define VIR_FROM_THIS VIR_FROM_SYSINFO @@ -94,25 +95,24 @@ virSysinfoRead(void) { virSysinfoDefPtr virSysinfoRead(void) { char *path, *cur, *eol, *base; - int pid, outfd = -1, errfd = -1; virSysinfoDefPtr ret = NULL; const char *argv[] = { NULL, "-q", "-t", "0,1", NULL }; - int res, waitret, exitstatus; + int res; char *outbuf = NULL; - char *errbuf = NULL; + virCommandPtr cmd; path = virFindFileInPath(SYSINFO_SMBIOS_DECODER); if (path == NULL) { virSmbiosReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to find path for %s binary"), SYSINFO_SMBIOS_DECODER); - return(NULL); + return NULL; } - argv[0] = path; - res = virExec(argv, NULL, NULL, &pid, -1, &outfd, &errfd, - VIR_EXEC_NONE | VIR_EXEC_NONBLOCK); - if (res < 0) { + argv[0] = path; + cmd = virCommandNewArgs(argv); + virCommandSetOutputBuffer(cmd, &outbuf); + if (virCommandRun(cmd, NULL) < 0) { virSmbiosReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to execute command %s"), path); @@ -120,34 +120,6 @@ virSysinfoRead(void) { goto cleanup; } - /* - * we are interested in the output, capture it and errors too - */ - if (virPipeReadUntilEOF(outfd, errfd, &outbuf, &errbuf) < 0) { - virReportSystemError(errno, _("cannot wait for '%s'"), path); - while (waitpid(pid, &exitstatus, 0) == -1 && errno == EINTR) - ; - goto cleanup; - } - - if (outbuf) - VIR_DEBUG("Command stdout: %s", outbuf); - if (errbuf) - VIR_DEBUG("Command stderr: %s", errbuf); - - while ((waitret = waitpid(pid, &exitstatus, 0) == -1) && - (errno == EINTR)); - if (waitret == -1) { - virReportSystemError(errno, _("Failed to wait for '%s'"), path); - goto cleanup; - } - if (exitstatus != 0) { - virSmbiosReportError(VIR_ERR_INTERNAL_ERROR, - _("command %s failed with error code %d:%s"), - path, exitstatus, errbuf); - goto cleanup; - } - if (VIR_ALLOC(ret) < 0) goto no_memory; @@ -227,15 +199,13 @@ virSysinfoRead(void) { cleanup: VIR_FREE(outbuf); - VIR_FREE(errbuf); - VIR_FREE(path); + virCommandFree(cmd); - return(ret); + return ret; no_memory: virReportOOMError(); - virSysinfoDefFree(ret); ret = NULL; goto cleanup; -- 1.7.3.2
On Fri, Dec 03, 2010 at 02:56:18PM -0700, Eric Blake wrote:
* src/util/sysinfo.c (virSysinfoRead): Use virCommand instead of virExec. ---
This patch assumes option 2 of my patch to virCommand guarantees on outbuf when the child process had no output (it needs a slight tweak to avoid a NULL dereference if we go with option 1 for virCommand).
src/util/sysinfo.c | 50 ++++++++++---------------------------------------- 1 files changed, 10 insertions(+), 40 deletions(-)
Okay, fine by me, ACK thanks for that nice patchset ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
* src/util/sysinfo.c: Indentation and () fixups. --- No semantic change; noticed this while rebasing 5/5 of previous series (1-4 of previous series are now pushed). src/util/sysinfo.c | 37 ++++++++++++++++++------------------- 1 files changed, 18 insertions(+), 19 deletions(-) diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c index cf41773..a02bd57 100644 --- a/src/util/sysinfo.c +++ b/src/util/sysinfo.c @@ -39,7 +39,7 @@ #define VIR_FROM_THIS VIR_FROM_SYSINFO -#define virSmbiosReportError(code, ...) \ +#define virSmbiosReportError(code, ...) \ virReportErrorHelper(NULL, VIR_FROM_SYSINFO, code, __FILE__, \ __FUNCTION__, __LINE__, __VA_ARGS__) @@ -88,7 +88,7 @@ virSysinfoRead(void) { */ virReportSystemError(ENOSYS, "%s", _("Host sysinfo extraction not supported on this platform")); - return(NULL); + return NULL; } #else virSysinfoDefPtr @@ -104,9 +104,9 @@ virSysinfoRead(void) { path = virFindFileInPath(SYSINFO_SMBIOS_DECODER); if (path == NULL) { virSmbiosReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to find path for %s binary"), + _("Failed to find path for %s binary"), SYSINFO_SMBIOS_DECODER); - return(NULL); + return NULL; } argv[0] = path; @@ -114,7 +114,7 @@ virSysinfoRead(void) { VIR_EXEC_NONE | VIR_EXEC_NONBLOCK); if (res < 0) { virSmbiosReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to execute command %s"), + _("Failed to execute command %s"), path); res = 1; goto cleanup; @@ -143,7 +143,7 @@ virSysinfoRead(void) { } if (exitstatus != 0) { virSmbiosReportError(VIR_ERR_INTERNAL_ERROR, - _("command %s failed with error code %d:%s"), + _("command %s failed with error code %d:%s"), path, exitstatus, errbuf); goto cleanup; } @@ -159,25 +159,25 @@ virSysinfoRead(void) { cur += 8; eol = strchr(cur, '\n'); if ((eol) && ((ret->bios_vendor = strndup(cur, eol - cur)) == NULL)) - goto no_memory; + goto no_memory; } if ((cur = strstr(base, "Version: ")) != NULL) { cur += 9; eol = strchr(cur, '\n'); if ((eol) && ((ret->bios_version = strndup(cur, eol - cur)) == NULL)) - goto no_memory; + goto no_memory; } if ((cur = strstr(base, "Release Date: ")) != NULL) { cur += 14; eol = strchr(cur, '\n'); if ((eol) && ((ret->bios_date = strndup(cur, eol - cur)) == NULL)) - goto no_memory; + goto no_memory; } if ((cur = strstr(base, "BIOS Revision: ")) != NULL) { cur += 15; eol = strchr(cur, '\n'); if ((eol) && ((ret->bios_release = strndup(cur, eol - cur)) == NULL)) - goto no_memory; + goto no_memory; } if ((base = strstr(outbuf, "System Information")) == NULL) goto cleanup; @@ -186,43 +186,43 @@ virSysinfoRead(void) { eol = strchr(cur, '\n'); if ((eol) && ((ret->system_manufacturer = strndup(cur, eol - cur)) == NULL)) - goto no_memory; + goto no_memory; } if ((cur = strstr(base, "Product Name: ")) != NULL) { cur += 14; eol = strchr(cur, '\n'); if ((eol) && ((ret->system_product = strndup(cur, eol - cur)) == NULL)) - goto no_memory; + goto no_memory; } if ((cur = strstr(base, "Version: ")) != NULL) { cur += 9; eol = strchr(cur, '\n'); if ((eol) && ((ret->system_version = strndup(cur, eol - cur)) == NULL)) - goto no_memory; + goto no_memory; } if ((cur = strstr(base, "Serial Number: ")) != NULL) { cur += 15; eol = strchr(cur, '\n'); if ((eol) && ((ret->system_serial = strndup(cur, eol - cur)) == NULL)) - goto no_memory; + goto no_memory; } if ((cur = strstr(base, "UUID: ")) != NULL) { cur += 6; eol = strchr(cur, '\n'); if ((eol) && ((ret->system_uuid = strndup(cur, eol - cur)) == NULL)) - goto no_memory; + goto no_memory; } if ((cur = strstr(base, "SKU Number: ")) != NULL) { cur += 12; eol = strchr(cur, '\n'); if ((eol) && ((ret->system_sku = strndup(cur, eol - cur)) == NULL)) - goto no_memory; + goto no_memory; } if ((cur = strstr(base, "Family: ")) != NULL) { cur += 8; eol = strchr(cur, '\n'); if ((eol) && ((ret->system_family = strndup(cur, eol - cur)) == NULL)) - goto no_memory; + goto no_memory; } cleanup: @@ -230,12 +230,11 @@ cleanup: VIR_FREE(errbuf); VIR_FREE(path); - return(ret); + return ret; no_memory: virReportOOMError(); - virSysinfoDefFree(ret); ret = NULL; goto cleanup; -- 1.7.3.2
On 12/07/2010 02:41 PM, Eric Blake wrote:
* src/util/sysinfo.c: Indentation and () fixups. ---
No semantic change; noticed this while rebasing 5/5 of previous series (1-4 of previous series are now pushed).
src/util/sysinfo.c | 37 ++++++++++++++++++------------------- 1 files changed, 18 insertions(+), 19 deletions(-)
I've now pushed this under the trivial rule. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* src/util/sysinfo.c (virSysinfoRead): Use virCommand instead of virExec. --- v2: remove unused variable, rebase on top of whitespace cleanups. This patch is awaiting review on these prerequisites: https://www.redhat.com/archives/libvir-list/2010-December/msg00318.html https://www.redhat.com/archives/libvir-list/2010-December/msg00321.html src/util/sysinfo.c | 45 ++++++--------------------------------------- 1 files changed, 6 insertions(+), 39 deletions(-) diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c index a02bd57..5b40a10 100644 --- a/src/util/sysinfo.c +++ b/src/util/sysinfo.c @@ -36,6 +36,7 @@ #include "conf/domain_conf.h" #include "logging.h" #include "memory.h" +#include "command.h" #define VIR_FROM_THIS VIR_FROM_SYSINFO @@ -94,12 +95,9 @@ virSysinfoRead(void) { virSysinfoDefPtr virSysinfoRead(void) { char *path, *cur, *eol, *base; - int pid, outfd = -1, errfd = -1; virSysinfoDefPtr ret = NULL; - const char *argv[] = { NULL, "-q", "-t", "0,1", NULL }; - int res, waitret, exitstatus; char *outbuf = NULL; - char *errbuf = NULL; + virCommandPtr cmd; path = virFindFileInPath(SYSINFO_SMBIOS_DECODER); if (path == NULL) { @@ -108,43 +106,13 @@ virSysinfoRead(void) { SYSINFO_SMBIOS_DECODER); return NULL; } - argv[0] = path; - res = virExec(argv, NULL, NULL, &pid, -1, &outfd, &errfd, - VIR_EXEC_NONE | VIR_EXEC_NONBLOCK); - if (res < 0) { + cmd = virCommandNewArgList(path, "-q", "-t", "0,1", NULL); + virCommandSetOutputBuffer(cmd, &outbuf); + if (virCommandRun(cmd, NULL) < 0) { virSmbiosReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to execute command %s"), path); - res = 1; - goto cleanup; - } - - /* - * we are interested in the output, capture it and errors too - */ - if (virPipeReadUntilEOF(outfd, errfd, &outbuf, &errbuf) < 0) { - virReportSystemError(errno, _("cannot wait for '%s'"), path); - while (waitpid(pid, &exitstatus, 0) == -1 && errno == EINTR) - ; - goto cleanup; - } - - if (outbuf) - VIR_DEBUG("Command stdout: %s", outbuf); - if (errbuf) - VIR_DEBUG("Command stderr: %s", errbuf); - - while ((waitret = waitpid(pid, &exitstatus, 0) == -1) && - (errno == EINTR)); - if (waitret == -1) { - virReportSystemError(errno, _("Failed to wait for '%s'"), path); - goto cleanup; - } - if (exitstatus != 0) { - virSmbiosReportError(VIR_ERR_INTERNAL_ERROR, - _("command %s failed with error code %d:%s"), - path, exitstatus, errbuf); goto cleanup; } @@ -227,8 +195,7 @@ virSysinfoRead(void) { cleanup: VIR_FREE(outbuf); - VIR_FREE(errbuf); - VIR_FREE(path); + virCommandFree(cmd); return ret; -- 1.7.3.2
2010/12/7 Eric Blake <eblake@redhat.com>:
* src/util/sysinfo.c (virSysinfoRead): Use virCommand instead of virExec. --- v2: remove unused variable, rebase on top of whitespace cleanups.
This patch is awaiting review on these prerequisites: https://www.redhat.com/archives/libvir-list/2010-December/msg00318.html https://www.redhat.com/archives/libvir-list/2010-December/msg00321.html
path = virFindFileInPath(SYSINFO_SMBIOS_DECODER); if (path == NULL) { @@ -108,43 +106,13 @@ virSysinfoRead(void) { SYSINFO_SMBIOS_DECODER); return NULL; } - argv[0] = path;
- res = virExec(argv, NULL, NULL, &pid, -1, &outfd, &errfd, - VIR_EXEC_NONE | VIR_EXEC_NONBLOCK); - if (res < 0) { + cmd = virCommandNewArgList(path, "-q", "-t", "0,1", NULL); + virCommandSetOutputBuffer(cmd, &outbuf); + if (virCommandRun(cmd, NULL) < 0) { virSmbiosReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to execute command %s"), path); - res = 1; - goto cleanup; - } - - /* - * we are interested in the output, capture it and errors too - */ - if (virPipeReadUntilEOF(outfd, errfd, &outbuf, &errbuf) < 0) { - virReportSystemError(errno, _("cannot wait for '%s'"), path); - while (waitpid(pid, &exitstatus, 0) == -1 && errno == EINTR) - ; - goto cleanup; - } - - if (outbuf) - VIR_DEBUG("Command stdout: %s", outbuf); - if (errbuf) - VIR_DEBUG("Command stderr: %s", errbuf); - - while ((waitret = waitpid(pid, &exitstatus, 0) == -1) && - (errno == EINTR)); - if (waitret == -1) { - virReportSystemError(errno, _("Failed to wait for '%s'"), path); - goto cleanup; - } - if (exitstatus != 0) { - virSmbiosReportError(VIR_ERR_INTERNAL_ERROR, - _("command %s failed with error code %d:%s"), - path, exitstatus, errbuf); goto cleanup; }
@@ -227,8 +195,7 @@ virSysinfoRead(void) {
cleanup: VIR_FREE(outbuf); - VIR_FREE(errbuf); - VIR_FREE(path); + virCommandFree(cmd);
Why not free path anymore, doesn't it leak now? ACK, with that possible leak addressed. Matthias
On 12/13/2010 04:10 PM, Matthias Bolte wrote:
2010/12/7 Eric Blake <eblake@redhat.com>:
* src/util/sysinfo.c (virSysinfoRead): Use virCommand instead of virExec. --- v2: remove unused variable, rebase on top of whitespace cleanups.
path = virFindFileInPath(SYSINFO_SMBIOS_DECODER); if (path == NULL) {
allocated...
@@ -108,43 +106,13 @@ virSysinfoRead(void) { SYSINFO_SMBIOS_DECODER); return NULL; } - argv[0] = path;
the old code freed it as part of argv cleanups
- res = virExec(argv, NULL, NULL, &pid, -1, &outfd, &errfd, - VIR_EXEC_NONE | VIR_EXEC_NONBLOCK); - if (res < 0) { + cmd = virCommandNewArgList(path, "-q", "-t", "0,1", NULL);
oh, you're right - the new code strdup's it, so I did leak it.
cleanup: VIR_FREE(outbuf); - VIR_FREE(errbuf); - VIR_FREE(path); + virCommandFree(cmd);
Why not free path anymore, doesn't it leak now?
ACK, with that possible leak addressed.
I've restored the VIR_FREE(path). Thanks for the review, and pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel Veillard -
Eric Blake -
Matthias Bolte