Re: [PATCH 09/21] util: Introduce virBitmapShrink

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Mon, Nov 13, 2017 at 03:42:40PM -0500, John Ferlan wrote:
> On 11/13/2017 03:50 AM, Martin Kletzander wrote:
> > Sometimes the size of the bitmap matters and it might not be guessed correctly
> > when parsing from some type of input.  For example virBitmapNewData() has Byte
> > granularity, virBitmapNewString() has nibble granularity and so on.
> > virBitmapParseUnlimited() can be tricked into creating huge bitmap that's not
> > needed (e.g.: "0-2,^99999999").  This function provides a way to shrink the
> > bitmap.  It is not supposed to free any memory.
>
> Is there a specific reason why you don't free memory?  Consider that the
> corollary virBitmapExpand can always be used to regrow the bitmap. I'm
> fine with not free'ing, but maybe someone would want to...  OK, sure
> they can supply the patch some day, I know...

I don't free it because a) it would cost more time and b) we over-allocate a bit
anyway.  Also this is mainly used so that the bitmap size is predictable, not
much else.

> > Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> > ---
> >  src/libvirt_private.syms |  1 +
> >  src/util/virbitmap.c     | 19 +++++++++++++++++++
> >  src/util/virbitmap.h     |  2 ++
> >  3 files changed, 22 insertions(+)
>
> With a couple of notes below handled,
>
> Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
>
> John
>
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 0b78a0681c5e..3986cc523e39 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -1369,6 +1369,7 @@ virBitmapParseUnlimited;
> >  virBitmapSetAll;
> >  virBitmapSetBit;
> >  virBitmapSetBitExpand;
> > +virBitmapShrink;
> >  virBitmapSize;
> >  virBitmapSubtract;
> >  virBitmapToData;
> > diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
> > index ac6ff4f6d26d..95b1f8656907 100644
> > --- a/src/util/virbitmap.c
> > +++ b/src/util/virbitmap.c
> > @@ -176,6 +176,25 @@ int virBitmapSetBit(virBitmapPtr bitmap, size_t b)
> >      return 0;
> >  }
> >
> > +
> > +/**
> > + * virBitmapShrink:
> > + * @map: Pointer to bitmap
> > + * @b: last bit position to be excluded from bitmap
> > + *
> > + * Resizes the bitmap so that no more than @b bits will fit into it.  Nothing
> > + * will change if the size is already smaller than @b.
>
> Considering adding, "NB: Does not adjust the map->map_len so that a
> subsequent virBitmapExpand doesn't necessarily need to reallocate."
> (not required, just a suggestion)

Added

> > + */
> > +void virBitmapShrink(virBitmapPtr map, size_t b)
>
> void
> virBitmapStrink(virBitmapPtr map,
>                size_t b)

done

> > +{
> > +    if (!map)
> > +        return;
> > +
> > +    if (map->max_bit >= b)
> > +        map->max_bit = b;
> > +}
> > +
> > +
> >  /**
> >   * virBitmapExpand:
> >   * @map: Pointer to bitmap
> > diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h
> > index 7b2bea8b534c..2464814055de 100644
> > --- a/src/util/virbitmap.h
> > +++ b/src/util/virbitmap.h
> > @@ -153,4 +153,6 @@ void virBitmapIntersect(virBitmapPtr a, virBitmapPtr b)
> >  void virBitmapSubtract(virBitmapPtr a, virBitmapPtr b)
> >      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> >
> > +void virBitmapShrink(virBitmapPtr map, size_t b);
> > +
>
> Not that it matters but it's always nice to keep the .h file in the same
> relative order as the .c file...  So this would move below virBitmapSetBit

I went the other way and moved it in the .c file, I have no idea why I didn't
put it in the end anyway.

> >  #endif

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list