Re: [libvirt] [PATCH] Fixed URI parsing
On 02/09/2012 11:48 AM, Martin Kletzander wrote:
On 02/09/2012 04:38 PM, Laine Stump wrote:
> On 02/09/2012 09:43 AM, Martin Kletzander 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
> 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".