1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/3] Misc util/ fixes plus tests
  4. View patch

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

Martin Kletzander posted 3 patches 7 years, 3 months ago
[libvirt] [PATCH 2/3] util: Clear unused part of the map in virBitmapShrink
Posted by Martin Kletzander 7 years, 3 months ago 
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.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1540817

Signed-off-by: Martin Kletzander <mkletzan@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)
 {
+    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));
+    }
 }
diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c
index 9c0ffe70cb49..a3258dc0ebad 100644
--- a/tests/virbitmaptest.c
+++ b/tests/virbitmaptest.c
@@ -656,6 +656,11 @@ test12(const void *opaque ATTRIBUTE_UNUSED)
 
     TEST_MAP(1024, "34,1023");
 
+    virBitmapShrink(map, 35);
+    TEST_MAP(35, "34");
+    virBitmapShrink(map, 34);
+    TEST_MAP(34, "");
+
     ret = 0;
 
  cleanup:
-- 
2.16.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] util: Clear unused part of the map in virBitmapShrink
Posted by Peter Krempa 7 years, 3 months ago 
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.

> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1540817
> 
> Signed-off-by: Martin Kletzander <mkletzan@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.

> +    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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] util: Clear unused part of the map in virBitmapShrink
Posted by Martin Kletzander 7 years, 3 months ago 
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@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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] util: Clear unused part of the map in virBitmapShrink
Posted by Peter Krempa 7 years, 3 months ago 
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@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.


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/3] util: Clear unused part of the map in virBitmapShrink
Posted by Martin Kletzander 7 years, 3 months ago 
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@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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.