11:42 a.m.
On 05/09/2013 06:27 AM, Martin Kletzander wrote:
On 04/30/2013 08:19 PM, John Ferlan wrote:
> If the <sysinfo> system table 'uuid' field is improperly formatted,
> then qemu will fail to start the guest with the error:
>
> virsh start dom
> error: Failed to start domain dom
> error: internal error process exited while connecting to monitor: Invalid SMBIOS UUID
string
>
> In this case the "system_uuid" field was
a94b4335-6a14-8bc4-d6da-f7ea590b68-16
> which passed the virUUIDParse() code because 32 hexadecimal digits were found
> and the extra hyphen in the last section was ignored.
>
> Add checks to not only parse the read field, but then use virUUIDFormat() to
> validate that what gets formatted matches what was read - if not, then fail
> the edit.
I feel like we could do better. Either 1) such UUID is not valid (which
I think it really isn't [1]) and we should fail when when parsing it or
2) it is valid, but qemu doesn't like it, so we should fixup the UUID
before passing it to qemu (and maybe request proper UUID parsing from
qemu guys).
What do you think?
That seemed to be a much larger fish to fry than I wanted to take on
with this particular patch. That is by avoiding the dashes ("-") in
virUUIDParse() already leads to this particular validation. The numbers
are all correct, it's just additional dashes that were a problem.
Changing that algorithm to be less forgiving led me down a path of
wondering why that code is forgiving w/r/t "-" and " " and what could
fall out from changing that?
I suppose for this particular case it could have been possible to take
the provided 'sysinfo' uuid field in qemuBuildSmbiosSystemStr() and make
the virUUIDParse() and virUUIDFormat() calls prior to the
virBufferAsprintf() which adds it to the command line. If that's
desired, I can take that route.
Other that that, the patch look fine.
Martin
[1] http://www.ietf.org/rfc/rfc4122.txt
> ---
> src/conf/domain_conf.c | 23 +++++++++++++++++++----
> 1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 43273f8..c1fd99b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -11577,17 +11577,32 @@ virDomainDefParseXML(xmlDocPtr xml,
> goto error;
> if (def->sysinfo->system_uuid != NULL) {
> unsigned char uuidbuf[VIR_UUID_BUFLEN];
> - if (virUUIDParse(def->sysinfo->system_uuid, uuidbuf) < 0) {
> + char uuidstr[VIR_UUID_STRING_BUFLEN];
> + /* Ensure that what we convert to a uuidbuf is converted back to
> + * the same string when formatted as a UUID. This field may be
> + * used by the underlying hypervisor driver instead of the domain
> + * uuid field and must be properly formatted. The virUUIDParse()
> + * is designed to "skip" extra "-"'s in the
values and only
> + * validate that there are 32 hexadecimal digits. virUUIDFormat()
> + * returns uuidstr formatted properly.
> + */
> + if (virUUIDParse(def->sysinfo->system_uuid, uuidbuf) < 0 ||
> + memcmp(def->sysinfo->system_uuid,
> + virUUIDFormat(uuidbuf, uuidstr),
> + VIR_UUID_STRING_BUFLEN) != 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> - "%s", _("malformed uuid
element"));
> + _("malformed <sysinfo> uuid element
'%s' "
> + "found on '%s'"),
> + def->sysinfo->system_uuid, def->name);
> goto error;
> }
> if (uuid_generated)
> memcpy(def->uuid, uuidbuf, VIR_UUID_BUFLEN);
> else if (memcmp(def->uuid, uuidbuf, VIR_UUID_BUFLEN) != 0) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + virReportError(VIR_ERR_INTERNAL_ERROR,
Pre-existing, but it looks like another abuse of internal error.
To be consistent w/ the bios_date change, I changed from INTERNAL_ERROR
to XML_DETAIL
> _("UUID mismatch between
<uuid> and "
> - "<sysinfo>"));
> + "<sysinfo> 'uuid' <entry>
on '%s'"),
> + def->name);
> goto error;
> }
> }
>
I squashed the following in - I could send a v2 if that's desired...
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios-uuid-format.xml
b/tests/
new file mode 100644
index 0000000..ccbce54
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios-uuid-format.xml
@@ -0,0 +1,24 @@
+<domain type='qemu'>
+ <name>smbios</name>
+ <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid>
+ <memory unit='KiB'>1048576</memory>
+ <currentMemory unit='KiB'>1048576</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <smbios mode="sysinfo"/>
+ </os>
+ <sysinfo type="smbios">
+ <system>
+ <entry
name="uuid">362d1fc1-df7d-193e-5c18-49a71b-d1da66</entry>
+ </system>
+ </sysinfo>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>restart</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ </devices>
+</domain>
+
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-smbios-uuid-match.xml
b/tests/q
new file mode 100644
index 0000000..e23c95e
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-smbios-uuid-match.xml
@@ -0,0 +1,24 @@
+<domain type='qemu'>
+ <name>smbios</name>
+ <uuid>362d1fc1-df7d-193e-5c18-49a71bd1da66</uuid>
+ <memory unit='KiB'>1048576</memory>
+ <currentMemory unit='KiB'>1048576</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <smbios mode="sysinfo"/>
+ </os>
+ <sysinfo type="smbios">
+ <system>
+ <entry
name="uuid">a94b4335-6a14-8bc4-d6da-f7ea590b6816</entry>
+ </system>
+ </sysinfo>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>restart</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ </devices>
+</domain>
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 1286273..35427a1 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -814,6 +814,9 @@ mymain(void)
DO_TEST("smbios", QEMU_CAPS_SMBIOS_TYPE);
DO_TEST_PARSE_ERROR("smbios-date", QEMU_CAPS_SMBIOS_TYPE);
+ DO_TEST_PARSE_ERROR("smbios-uuid-format", QEMU_CAPS_SMBIOS_TYPE);
+ DO_TEST_PARSE_ERROR("smbios-uuid-match", QEMU_CAPS_SMBIOS_TYPE);
DO_TEST("watchdog", NONE);
DO_TEST("watchdog-device", QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);