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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list