On 4/25/19 5:42 AM, Pavel Hrdina wrote:
> On Mon, Apr 15, 2019 at 02:43:07PM +0000, Allen, John wrote:
>> If a bitmap of a shorter length than the data buffer is passed to
>> virBitmapToDataBuf, it will read off the end of the bitmap and copy junk
>> into the returned buffer. Add a check to only copy the length of the
>> bitmap to the buffer.
>>
>> The problem can be observed after setting a vcpu affinity using the vcpupin
>> command on a system with a large number of cores:
>> # virsh vcpupin example_domain 0 0
>> # virsh vcpupin example_domain 0
>> VCPU CPU Affinity
>> ---------------------------
>> 0 0,192,197-198,202
>>
>> Signed-off-by: John Allen <john.allen@xxxxxxx>
>> ---
>
> Thanks for the patch, pushed now.
>
>> diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c
>> index d074f29e54..021174cbb3 100644
>> --- a/src/util/virbitmap.c
>> +++ b/src/util/virbitmap.c
>> @@ -824,11 +824,15 @@ virBitmapToDataBuf(virBitmapPtr bitmap,
>> unsigned char *bytes,
>> size_t len)
>> {
>> + size_t nbytes = bitmap->map_len * (VIR_BITMAP_BITS_PER_UNIT / CHAR_BIT);
>> unsigned long *l;
>> size_t i, j;
>>
>> memset(bytes, 0, len);
>>
>> + /* If bitmap and buffer differ in size, only fill to the smaller length */
>> + len = MIN(len, nbytes);
>> +
>
> I would probably store the min back to nbytes and used that but it
> doesn't matter.
>
>> /* htole64 is not provided by gnulib, so we do the conversion by hand */
>> l = bitmap->map;
>> for (i = j = 0; i < len; i++, j++) {
>
ACK to the patch as well. I hit a similar issue in my attempts to test
this, thought they had a different root cause but I was wrong, so ignore
my message attached to the test driver patch.
FWIW if someone wants to manually reproduce, grab my test driver patch
posted earlier and attached, and the testdriver.xml attached. Revert
this patch, build and reproduce with:
$ ./tools/virsh --connect test://`pwd`/testdriver.xml "vcpupin test 0 4;
vcpuinfo test"
Similarly this gives wrong results too, only first 13 host cpus should
be online:
$ ./tools/virsh --connect test://`pwd`/testdriver.xml nodecpumap
The latter is due to some bad virBitmapToData usage, but it's separate.
Thanks,
Cole
<node>
<cpu>
<nodes>1</nodes>
<sockets>32</sockets>
<cores>4</cores>
<threads>4</threads>
<active>13</active>
<mhz>4000</mhz>
<model>i686</model>
</cpu>
<memory>10000000</memory>
<domain type='test'>
<name>test</name>
<uuid>4a64cc71-19c4-2fd0-2323-3050941ea3c3</uuid>
<memory>8388608</memory>
<currentMemory>2097152</currentMemory>
<vcpu>1</vcpu>
<os>
<type arch='i686'>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>
</devices>
</domain>
</node>
>From 407cb2b56fea2a9621e5dbabe38fdb45764eb691 Mon Sep 17 00:00:00 2001
Message-Id: <407cb2b56fea2a9621e5dbabe38fdb45764eb691.1556196423.git.crobinso@xxxxxxxxxx>
From: Cole Robinson <crobinso@xxxxxxxxxx>
Date: Wed, 24 Apr 2019 19:13:04 -0400
Subject: [PATCH] test: Use nodeInfo content for virNodeGetCPUMap
Right now the test driver implementation of virNodeGetCPUMap uses
hardcoded data. Instead use cpu data from our nodeInfo, which can be
overridden with input test XML. This helps to emulate some virsh
commands (vcpupin, hostcpumap) with a large number of host cpus. [1]
The content reported for test:///default is now slightly different:
previously it would report 8 total cpus with 0,2,4 online. Now it
reports 16 cpus with all online, which is the pre-existing nodeInfo
default.
[1] https://www.redhat.com/archives/libvir-list/2019-March/msg02042.html
Signed-off-by: Cole Robinson <crobinso@xxxxxxxxxx>
---
John, in trying to understand and test your bitmap patch, I added
this to the test driver. I think there's a bigger issue at play
but I'm still trying to work it out, I'll send a follow up email
when I've collected my thoughts
src/test/test_driver.c | 35 +++++++++++++++++++++++++++++------
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index d5eecf4b7f..f00b6eb1c8 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -5811,23 +5811,46 @@ static int testConnectListAllDomains(virConnectPtr conn,
}
static int
-testNodeGetCPUMap(virConnectPtr conn ATTRIBUTE_UNUSED,
+testNodeGetCPUMap(virConnectPtr conn,
unsigned char **cpumap,
unsigned int *online,
unsigned int flags)
{
+ testDriverPtr privconn = conn->privateData;
+ int maxcpus = VIR_NODEINFO_MAXCPUS(privconn->nodeInfo);
+ virBitmapPtr cpus = NULL;
+ int ret = -1;
+ int dummy;
+ size_t i;
+
virCheckFlags(0, -1);
+ if (!cpumap && !online)
+ return maxcpus;
+
+ if (!(cpus = virBitmapNew(maxcpus)))
+ goto cleanup;
+
if (cpumap) {
- if (VIR_ALLOC_N(*cpumap, 1) < 0)
- return -1;
- *cpumap[0] = 0x15;
+ for (i = 0; i < privconn->nodeInfo.cpus; i++) {
+ if (virBitmapSetBit(cpus, i) < 0)
+ goto cleanup;
+ }
+
+ if (virBitmapToData(cpus, cpumap, &dummy) < 0)
+ goto cleanup;
}
if (online)
- *online = 3;
+ *online = privconn->nodeInfo.cpus;
- return 8;
+ ret = maxcpus;
+
+ cleanup:
+ if (ret < 0 && cpumap)
+ VIR_FREE(*cpumap);
+ VIR_FREE(cpus);
+ return ret;
}
static char *
--
2.21.0
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
