[libvirt] [PATCH 0/3] Misc util/ fixes plus tests
Yay!!! \o/ Patches! Martin Kletzander (3): util: Fix possible leak in virResctrlAllocMasksAssign util: Clear unused part of the map in virBitmapShrink tests: Add test for properly removing cachetune entries src/util/virbitmap.c | 13 +++++++++ src/util/virresctrl.c | 4 +-- .../genericxml2xmlindata/cachetune-extra-tunes.xml | 33 ++++++++++++++++++++++ .../cachetune-extra-tunes.xml | 30 ++++++++++++++++++++ tests/genericxml2xmltest.c | 1 + tests/virbitmaptest.c | 5 ++++ 6 files changed, 84 insertions(+), 2 deletions(-) create mode 100644 tests/genericxml2xmlindata/cachetune-extra-tunes.xml create mode 100644 tests/genericxml2xmloutdata/cachetune-extra-tunes.xml -- 2.16.1
Found by coverity. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index e1c4998f7151..70426199ce20 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -1444,10 +1444,10 @@ virResctrlAllocMasksAssign(virResctrlInfoPtr resctrl, alloc_default = virResctrlAllocGetDefault(resctrl); if (!alloc_default) - return -1; + goto cleanup; if (virResctrlAllocCopyMasks(alloc, alloc_default) < 0) - return -1; + goto cleanup; for (level = 0; level < alloc->nlevels; level++) { virResctrlAllocPerLevelPtr a_level = alloc->levels[level]; -- 2.16.1
On Fri, Feb 02, 2018 at 08:23:32 +0100, Martin Kletzander wrote:
Found by coverity.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK
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
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.
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.
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.
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.
Cachetune for unavailable vCPUs should be cleared the same way vcpupin and other things do, so let's add tests for it. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- .../genericxml2xmlindata/cachetune-extra-tunes.xml | 33 ++++++++++++++++++++++ .../cachetune-extra-tunes.xml | 30 ++++++++++++++++++++ tests/genericxml2xmltest.c | 1 + 3 files changed, 64 insertions(+) create mode 100644 tests/genericxml2xmlindata/cachetune-extra-tunes.xml create mode 100644 tests/genericxml2xmloutdata/cachetune-extra-tunes.xml diff --git a/tests/genericxml2xmlindata/cachetune-extra-tunes.xml b/tests/genericxml2xmlindata/cachetune-extra-tunes.xml new file mode 100644 index 000000000000..d3b01a8ecdbf --- /dev/null +++ b/tests/genericxml2xmlindata/cachetune-extra-tunes.xml @@ -0,0 +1,33 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>2</vcpu> + <cputune> + <cachetune vcpus='0-3'> + <cache id='0' level='3' type='code' size='7680' unit='KiB'/> + <cache id='1' level='3' type='data' size='3840' unit='KiB'/> + </cachetune> + <cachetune vcpus='100'> + <cache id='1' level='3' type='code' size='6' unit='MiB'/> + </cachetune> + </cputune> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/genericxml2xmloutdata/cachetune-extra-tunes.xml b/tests/genericxml2xmloutdata/cachetune-extra-tunes.xml new file mode 100644 index 000000000000..db85af08da25 --- /dev/null +++ b/tests/genericxml2xmloutdata/cachetune-extra-tunes.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>2</vcpu> + <cputune> + <cachetune vcpus='0-1'> + <cache id='0' level='3' type='code' size='7680' unit='KiB'/> + <cache id='1' level='3' type='data' size='3840' unit='KiB'/> + </cachetune> + </cputune> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-i686</emulator> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c index 496e9260fa86..c33fce192237 100644 --- a/tests/genericxml2xmltest.c +++ b/tests/genericxml2xmltest.c @@ -133,6 +133,7 @@ mymain(void) DO_TEST("cachetune"); DO_TEST("cachetune-small"); DO_TEST("cachetune-cdp"); + DO_TEST_DIFFERENT("cachetune-extra-tunes"); DO_TEST_FULL("cachetune-colliding-allocs", false, true, TEST_COMPARE_DOM_XML2XML_RESULT_FAIL_PARSE); DO_TEST_FULL("cachetune-colliding-tunes", false, true, -- 2.16.1
On Fri, Feb 02, 2018 at 08:23:34 +0100, Martin Kletzander wrote:
Cachetune for unavailable vCPUs should be cleared the same way vcpupin and other things do, so let's add tests for it.
Umm, note that pinning can be setup for offline vCPUs. Obviously not for those which are permanently removed when shrinking number of vcpus in a persistent definition.
On Fri, Feb 02, 2018 at 10:39:05AM +0100, Peter Krempa wrote:
On Fri, Feb 02, 2018 at 08:23:34 +0100, Martin Kletzander wrote:
Cachetune for unavailable vCPUs should be cleared the same way vcpupin and other things do, so let's add tests for it.
Umm, note that pinning can be setup for offline vCPUs. Obviously not for those which are permanently removed when shrinking number of vcpus in a persistent definition.
Yeah, by "unavailable" I meant any vCPU with `id > def->maxvcpus`. The tunings are kept for those that are not started yet and hotplug handles adding those into allocations.
On Fri, Feb 02, 2018 at 11:11:08 +0100, Martin Kletzander wrote:
On Fri, Feb 02, 2018 at 10:39:05AM +0100, Peter Krempa wrote:
On Fri, Feb 02, 2018 at 08:23:34 +0100, Martin Kletzander wrote:
Cachetune for unavailable vCPUs should be cleared the same way vcpupin and other things do, so let's add tests for it.
Umm, note that pinning can be setup for offline vCPUs. Obviously not for those which are permanently removed when shrinking number of vcpus in a persistent definition.
Yeah, by "unavailable" I meant any vCPU with `id > def->maxvcpus`. The tunings are kept for those that are not started yet and hotplug handles adding those into allocations.
Thanks for clarification. I can see it in the XML too now. ACK
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. It's enough to zero out one piece of the map since we're calling realloc() to get rid of the rest (and updating map_len). Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1540817 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 4 +++- src/util/virbitmap.c | 30 ++++++++++++++++++++++-------- src/util/virbitmap.h | 2 +- src/util/virresctrl.c | 3 ++- tests/virbitmaptest.c | 8 ++++++++ 5 files changed, 36 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 01d168eb875b..e827b2a810f7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18453,7 +18453,9 @@ virDomainCachetuneDefParse(virDomainDefPtr def, /* We need to limit the bitmap to number of vCPUs. If there's nothing left, * then we can just clean up and return 0 immediately */ - virBitmapShrink(vcpus, def->maxvcpus); + if (virBitmapShrink(vcpus, def->maxvcpus) < 0) + goto cleanup; + if (virBitmapIsAllClear(vcpus)) { ret = 0; goto cleanup; diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index b2c5c7a6a5ac..33cae2f30569 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -1201,21 +1201,35 @@ virBitmapSubtract(virBitmapPtr a, /** * virBitmapShrink: * @map: Pointer to bitmap - * @b: last bit position to be excluded from bitmap + * @b: Size to reduce the bitmap to * - * 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. - * - * NB: Does not adjust the map->map_len so that a subsequent virBitmapExpand - * doesn't necessarily need to reallocate. + * Reduces the bitmap to size @b. Nothing will change if the size is already + * smaller than or equal to @b. */ -void +int virBitmapShrink(virBitmapPtr map, size_t b) { + size_t nl = 0; + size_t nb = 0; + if (!map) - return; + return 0; 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) + return 0; + + if (VIR_REALLOC_N(map->map, nl) < 0) + return -1; + + map->map_len = nl; + return 0; } diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index 2464814055de..5a3362a19f9f 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -153,6 +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); +int virBitmapShrink(virBitmapPtr map, size_t b); #endif diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 70426199ce20..ef388757a704 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -941,7 +941,8 @@ virResctrlAllocParseProcessCache(virResctrlInfoPtr resctrl, if (!mask) return -1; - virBitmapShrink(mask, resctrl->levels[level]->types[type]->bits); + if (virBitmapShrink(mask, resctrl->levels[level]->types[type]->bits) < 0) + goto cleanup; if (virResctrlAllocUpdateMask(alloc, level, type, cache_id, mask) < 0) goto cleanup; diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index 9c0ffe70cb49..fffecdf1f6ed 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -656,6 +656,14 @@ test12(const void *opaque ATTRIBUTE_UNUSED) TEST_MAP(1024, "34,1023"); + if (virBitmapShrink(map, 35) < 0) + goto cleanup; + TEST_MAP(35, "34"); + + if (virBitmapShrink(map, 34) < 0) + goto cleanup; + TEST_MAP(34, ""); + ret = 0; cleanup: -- 2.16.1
On Fri, Feb 02, 2018 at 14:04:31 +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. It's enough to zero out one piece of the map since we're calling realloc() to get rid of the rest (and updating map_len).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1540817
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 4 +++- src/util/virbitmap.c | 30 ++++++++++++++++++++++-------- src/util/virbitmap.h | 2 +- src/util/virresctrl.c | 3 ++- tests/virbitmaptest.c | 8 ++++++++ 5 files changed, 36 insertions(+), 11 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 01d168eb875b..e827b2a810f7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18453,7 +18453,9 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
/* We need to limit the bitmap to number of vCPUs. If there's nothing left, * then we can just clean up and return 0 immediately */ - virBitmapShrink(vcpus, def->maxvcpus); + if (virBitmapShrink(vcpus, def->maxvcpus) < 0) + goto cleanup; + if (virBitmapIsAllClear(vcpus)) { ret = 0; goto cleanup; diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index b2c5c7a6a5ac..33cae2f30569 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -1201,21 +1201,35 @@ virBitmapSubtract(virBitmapPtr a, /** * virBitmapShrink: * @map: Pointer to bitmap - * @b: last bit position to be excluded from bitmap + * @b: Size to reduce the bitmap to * - * 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. - * - * NB: Does not adjust the map->map_len so that a subsequent virBitmapExpand - * doesn't necessarily need to reallocate. + * Reduces the bitmap to size @b. Nothing will change if the size is already + * smaller than or equal to @b. */ -void +int virBitmapShrink(virBitmapPtr map, size_t b) { + size_t nl = 0; + size_t nb = 0; + if (!map) - return; + return 0;
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) + return 0; + + if (VIR_REALLOC_N(map->map, nl) < 0)
With VIR_SHRINK_N you can avoid the return value ;) ACK
On Fri, Feb 02, 2018 at 02:15:05PM +0100, Peter Krempa wrote:
On Fri, Feb 02, 2018 at 14:04:31 +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. It's enough to zero out one piece of the map since we're calling realloc() to get rid of the rest (and updating map_len).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1540817
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 4 +++- src/util/virbitmap.c | 30 ++++++++++++++++++++++-------- src/util/virbitmap.h | 2 +- src/util/virresctrl.c | 3 ++- tests/virbitmaptest.c | 8 ++++++++ 5 files changed, 36 insertions(+), 11 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 01d168eb875b..e827b2a810f7 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18453,7 +18453,9 @@ virDomainCachetuneDefParse(virDomainDefPtr def,
/* We need to limit the bitmap to number of vCPUs. If there's nothing left, * then we can just clean up and return 0 immediately */ - virBitmapShrink(vcpus, def->maxvcpus); + if (virBitmapShrink(vcpus, def->maxvcpus) < 0) + goto cleanup; + if (virBitmapIsAllClear(vcpus)) { ret = 0; goto cleanup; diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index b2c5c7a6a5ac..33cae2f30569 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -1201,21 +1201,35 @@ virBitmapSubtract(virBitmapPtr a, /** * virBitmapShrink: * @map: Pointer to bitmap - * @b: last bit position to be excluded from bitmap + * @b: Size to reduce the bitmap to * - * 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. - * - * NB: Does not adjust the map->map_len so that a subsequent virBitmapExpand - * doesn't necessarily need to reallocate. + * Reduces the bitmap to size @b. Nothing will change if the size is already + * smaller than or equal to @b. */ -void +int virBitmapShrink(virBitmapPtr map, size_t b) { + size_t nl = 0; + size_t nb = 0; + if (!map) - return; + return 0;
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) + return 0; + + if (VIR_REALLOC_N(map->map, nl) < 0)
With VIR_SHRINK_N you can avoid the return value ;)
or with ignore_value, which is what VIR_SHRINK_N does over virReallocN anyway :D Patches welcome
ACK
Thanks
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Martin Kletzander -
Peter Krempa