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

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.:

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