On 01/21/2016 01:56 AM, Markus Armbruster wrote: >>> Before: nobody implements type_uint64(), and the core falls back to >>> type_int64(), casting negative values to large positive ones. With an >>> implementation of type_int64() that parses large positive values as >>> negative, the two casts cancel out. >>> >>> After: everybody implements type_uint64() incorrectly, by reusing >>> type_int64() code. The problem moves from visitor core to visitor >>> implementations, where we can actually fix it if we care. >>> >>> Correct? >> >> Close. opts-visitor.c already implemented type_uint64, but also has its >> known bugs (and Paolo has been pinged about his claim for fixes in that >> file...) > > Ah, yes. opts_type_uint64() doesn't reuse opts_type_int64(), but > largely duplicates it. Potentially less wrong :) > >> But otherwise, yes, in this patch, we are not fixing insanity so much as >> localizing and better documenting it. > > Let me try to clarify the commit message a bit: > > This patch does not address the disparity in handling large values > as negatives. It merely moves the fallback from uint64 to int64 > from the visitor core to the visitors, where the issue can actually > be fixed, by implementing the missing type_uint64() callbacks on top > of the respective type_int64() callbacks, with a FIXME comment > explaining why that's wrong.
Looks good. I think we're starting to rack up enough tweaks to make it worth me posting a v10 respin to fold them all in. >>>> The dealloc visitor no longer needs a type_size callback, >>>> since that now safely falls back to the type_uint64 callback. >>> >>> Did it need the callback before this patch? >> >> Not really. Should I split out the deletion of that callback as a >> separate patch? > > Probably cleanest, but merely adjusting the commit message would also > work for me: > > The dealloc visitor doesn't actually need a type_size() callback, > since the visitor core can safely fall back to the type_uint64() > callback. Drop it. Okay, I won't bother to split this one; the commit message tweak seems sufficient. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
