On 02/09/2012 11:48 AM, Martin Kletzander wrote:
On 02/09/2012 04:38 PM, Laine Stump wrote:
Function virParseURI was added. This function is wrapper around xmlParseURI. In this wrapper we are fixing what doesn't seems to be fixed in libxml2 in the near future. The wrapper is written in such way that if anything gets fixed in libxml2, it'll still work. The problem alluded to here is that when xmlParseURI encounters a numeric IPv6 address in brackets, it returns the entire string including
On 02/09/2012 09:43 AM, Martin Kletzander wrote: the brackets. getaddrxx(), by contrast, expects the brackets to not be there. And yet the brackets *must* be included to specify a numeric IP address, according to section 6 of RFC5952 (why am I ready with this information? Because I looked it up when commenting on a qemu bug last night :-)
(https://bugzilla.redhat.com/show_bug.cgi?id=680356 if you're interested) I'll definitely check this out, thanks for the info.
I almost ACKed this patch (with one small change to replace the loop with memmove), but then thought about what happens if we need to reconstruct the URI from this modified xmlURIPtr (e.g., with xmlSaveUri, which libvirt calls in two places). Jiri told me about this, it's just my fault that I've forgot to modify that part as well. What is your opinion for me setting up one more function (virSaveURI let's say) that constructs the string back as it is supposed to (again just a wrapper for xmlSaveURI)?
The problems I can see with that: 1) That would require you to once again modify the server string in-place, but you can't rely on it still having the 2 extra bytes, so you would have to create a new string. 2) If somebody decided in the future to use different libxml2 API on the URI (e.g. xmlPrintURI), or wrote their own code to do something with the string, they would have to know to provide a wrapper for that as well. 3) Just the oddness of determining whether or not to add brackets back based on whether or not you find a ":" in the string. On the other hand, if you leave the brackets in, then everybody who uses uri->server directly needs to know that it may have extraneous brackets around it, so I'm undecided. Maybe someone else has an opinion that will take it over the edge.
So, I think that the correct solution here, rather than permanently modifying the server string returned from xmlParseURI, is to look at the places where the server string is used for something other than just checking for != NULL (that leaves very few places) and modify a copy of the string to remove the brackets in those cases. This way you always have the exact original server string so that the original URI can be reconstructed.
That was the first thing I wanted to change but unfortunately I did that only for ssh so it was unusable.
You can just do a cscope search for "server", then look within those results for the string "->server".
+ for (size_t i = 0; i< last; i++) + ret->server[i] = ret->server[i + 1]; (this actually copies one character too many, but that's harmless). Just replace this with:
memmove(ret->server, ret->server+1, last-1) I know about memmove(), it's just that in this case I really didn't like that it copes the string somewhere else and than again back (in case of overlapping) but no problem to change this :-)
I would be surprised if gcc's memmove was that braindead. Especially when moving memory to a lower address, they can still use the same rep movs (or whatever) and it goes in the correct direction already - don't even have to reverse it. The manpage doesn't say that the bytes *are* copied into a temporary array and back, it just says "copying takes place *as if* the bytes are first copied into a temporary array".