On Tue, Mar 10, 2015 at 13:52:31 +0100, Ján Tomko wrote:
> On Fri, Mar 06, 2015 at 01:01:13PM -0700, Eric Blake wrote:
> > On 03/06/2015 10:02 AM, Ján Tomko wrote:
> > > Most of the callers use the pattern:
> > > bool b;
> > > ignore_value(virBitmapGetBit(.., &b));
> > > if (b) {
> > > ...
> > > }
> > >
> > > And the rest treats the failure (which can only happen
> > > when the requested bit is out of bitmap bounds) the same
> > > as they treat a 'false' bit.
> >
> > Blindly returning false for out of bounds may work for some (most?)
> > callers, but I still wonder if we should expose the full bounds-checking
> > version under a different name, and/or expose a variant that lets the
> > caller specify the default value to return for an out-of-bounds
> > reference. That is, while you've made the common case short, I wonder
> > if we need the extra control for correctness elsewhere.
> >
>
>
> From the 23 callers in this patch:
> 10 explicitly ignore_value
> 5 are in a for loop from 0 to virBitmapSize (virProcessSetAffinity,
> nodeCapsInitNUMA, virDomainSnapshotAlignDisks, libxlDomainGetVcpuPinInfo)
> 1 checks the range before calling GetBit (virPortAllocatorSetUsed)
> 1 loops against the bitmap range (virPortAllocatorAcquired)
>
> I think these don't need any bounds-checking
>
> 2 of them take an enum as an argument (*CapsGet), I don't think we need
> a special case for QEMU_CAPS_LAST either.
>
> qemuBuildMemoryBackendStr admits to asking for out-of-range bits, so it
> would require checking the bounds up front, or a default of 'false'.
>
> In the rest of the callers, out-of-bounds queries are programming bugs,
> but since the origins of their indexes are not so straightforward,
> perhaps those should keep using the range-checking version.
>
>
> So there's only one caller where a default of 'false' makes sense.
> I think a function named virBitmapIsBitSet could return false on
> out-of-range (how can it be set if it's not in the bitmap?)
I think this solution would be okay to get rid of the cruft in cases we
don't care about the range checks while we can keep the old impl for the
cases where it's not certain.
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list