On 05/10/2013 10:57 AM, Eric Blake wrote:
> On 05/10/2013 05:07 AM, Martin Kletzander wrote:
>>
>> I must admit I have no idea why we allow spaces and hyphens everywhere
>> in the UUID string. Changing it to proper UUID parser is not just
>> fixing this issue, but also removing more lines than adding, since the
>> parser could get halved. That's why I was thinking more about taking
>> that route. I'd love to hear some input from someone else in case this
>> looks like it would break something. IMNSHO that's a bad usage unless
>> we explicitly allowed spaces and hyphens somewhere in the docs.
>
> We're lax in what we parse and strict in what we output. We can't
> change the lax parser, because there really is code out there that
> doesn't format valid uuids itself and relies on libvirt to clean up the
> mess, but I don't think the current lax parser is all that bad (as long
> as we see exactly enough hex digits after skipping all dashes and
> spaces, we have something we can then turn into valid output).
>
>> If I read the code correctly, we are properly saving the UUID in the
>> definition in our format and using virUUIDFormat() properly to build the
>> command line. Aren't we doing the same with system_uuid? If not, that
>> could be helpful even for the future (actually until someone files a bug
>> about dumpxml generating invalid UUIDs ;-) )
>>
>> I can't help myself, but parsing, formatting and throwing away the
>> result just to check the validity seems weird.
>
> If we parse and then format, we should use what we formatted from then
> on, instead of the original user's (potentially unusual) formatting.
>
It would seem that you are thus advocating we change what was read
to be formatted properly and don't tell the user we did that, e.g.:
if (def->sysinfo->system_uuid != NULL) {
unsigned char uuidbuf[VIR_UUID_BUFLEN];
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
if (virUUIDParse(def->sysinfo->system_uuid, uuidbuf) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("malformed uuid element"));
+ "%s", _("malformed <sysinfo> uuid element"));
goto error;
}
if (uuid_generated)
@@ -11593,6 +11594,45 @@ virDomainDefParseXML(xmlDocPtr xml,
"<sysinfo>"));
goto error;
}
+ /* Although we've validated the UUID as good, virUUIDParse() is
+ * lax with respect to allowing extraneous "-" and " ", but the
+ * underlying hypervisor may be less forgiving. Use virUUIDFormat()
+ * to validate format in xml is right. If not, then format it
+ * properly so that it's used correctly later.
+ */
+ virUUIDFormat(uuidbuf, uuidstr);
+ if (memcmp(def->sysinfo->system_uuid, uuidstr,
+ VIR_UUID_STRING_BUFLEN) != 0) {
+ VIR_FREE(def->sysinfo->system_uuid);
+ if (VIR_STRDUP(def->sysinfo->system_uuid, uudstr) < 0) {
+ goto error;
+ }
+ }
+ }
Would it be better to see a v2 with everything?
Or push patches 1-3 and make a v2 of just this one?
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list