[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Fixed URI parsing



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)?

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

>> +            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 :-)


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]