[libvirt] [PATCH 2/3] util: Clear unused part of the map in virBitmapShrink

[Martin Kletzander]

On Fri, Feb 02, 2018 at 12:43:32PM +0100, Peter Krempa wrote:
>On Fri, Feb 02, 2018 at 12:30:10 +0100, Martin Kletzander wrote:
>> On Fri, Feb 02, 2018 at 10:37:33AM +0100, Peter Krempa wrote:
>> > On Fri, Feb 02, 2018 at 08:23:33 +0100, Martin Kletzander wrote:
>> > > Some of the other functions depend on the fact that unused bits and longs are
>> > > always zero and it's less error-prone to clear it than fix the other functions.
>> >
>> > Clearing the bitmap is okay with me, since if you grow it again it
>> > should not magically re-gain it's old values.
>> >
>> > Said that I think that also virBitmapNext*Bit functions should be fixed
>> > anyways.
>> >
>>
>> Sure, I can do that as well.
>>
>> > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1540817
>> > >
>> > > Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
>> > > ---
>> > >  src/util/virbitmap.c  | 13 +++++++++++++
>> > >  tests/virbitmaptest.c |  5 +++++
>> > >  2 files changed, 18 insertions(+)
>> > >
>> > > diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
>> > > index b2c5c7a6a5ac..b32342024e19 100644
>> > > --- a/src/util/virbitmap.c
>> > > +++ b/src/util/virbitmap.c
>> > > @@ -1213,9 +1213,22 @@ void
>> > >  virBitmapShrink(virBitmapPtr map,
>> > >                  size_t b)
>> > >  {
>> >
>> > I must say that I'm not a fan of the semantics of this API. The
>> > expansion function makes sure that bit position 'b' is included in the
>> > bitmap, while this makes sure that it's excluded.
>> >
>>
>> Well, yes, I thought of it as setting the bitmap `size` as opposed to the `last
>> included bit`, but I have no problem with adding `b++;` to the top of this
>> function.
>
>Actually that makes sense. I got confused by the expanding semantics
>which are actually weird.
>
>I'd only change the wording that this reduces the bitmap size to 'b'
>which would be consistent with the size argument when creating the
>bitmap.
>
>Also 'max_bit' is confusing. I think I'll patch that to 'nbits'.
>
>>
>> > > +    size_t nl = 0;
>> > > +    size_t nb = 0;
>> > > +
>> > >      if (!map)
>> > >          return;
>> > >
>> > >      if (map->max_bit >= b)
>> > >          map->max_bit = b;
>> > > +
>> > > +    nl = map->max_bit / VIR_BITMAP_BITS_PER_UNIT;
>> > > +    nb = map->max_bit % VIR_BITMAP_BITS_PER_UNIT;
>> > > +    map->map[nl] &= ((1UL << nb) - 1);
>> > > +
>> > > +    nl++;
>> > > +    if (nl < map->map_len) {
>> > > +        memset(map->map + nl, 0,
>> > > +               (map->map_len - nl) * (VIR_BITMAP_BITS_PER_UNIT / CHAR_BIT));
>> >
>> > Removing the memory allocation beyond the last bit should remove the
>> > need to do this, since growing it back would then call the clearing
>> > function and also remove potentially lingering memory.
>> >
>> > If you don't have any strong opinion against shrinking the storage array
>> > itself I'd prefer that solution.
>>
>> Well, I had, but since I'm going to send v2 anyway...  I'll add the realloc
>> there.
>
>I'd like to hear it actually, that's why I asked.
>

It's in the comment, just os Expand doesn't do yet another realloc
again, but that's not used anywhere in the code anyway.  Also, for some
reason, I thought we over-allocate the bitmap with the initial
virBitmapNew(), but it looks like we're not so that's a moo point as
well.  I don't really have a preference.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180202/36359c0b/attachment-0001.sig>