[libvirt] [PATCH 4/4] Need better validation of <sysinfo> uuid

[Martin Kletzander]

On 05/10/2013 06:52 PM, John Ferlan wrote:
> 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.:
> 

With hearing from Eric about why we do that, I would do what you
described, with one difference; you can get the uuid into temporary
buffer and parse it into system_uuid.  You don't have to check anything
and just format the string when the system_uuid is used.  That should be
the same way as def->uuid is used and it seems like the cleanest one.

[...]
> 
> Would it be better to see a v2 with everything?  
> Or push patches 1-3 and make a v2 of just this one?
> 

v2 of [3/4] and [4/4] would be great, thanks.

Martin