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

[Martin Kletzander]

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.

>> +    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.
-------------- 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/c5673e0d/attachment-0001.sig>