[libvirt] [PATCH 0/2] fix the error when get the vcpupin info and add a test
I was so ashamed of writing a incorrect commit '848ab68' and didn't test with it. Patch 1/2 fix the issue and Patch 2/2 introduce a test for it. Luyao Huang (2): virsh: really fix the error if vcpu number exceed the guest maxvcpu number test: introduce a function in test driver for check get vcpupin info src/test/test_driver.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/vcpupin | 34 +++++++++++++++++++++++++++---- tools/virsh-domain.c | 4 ++-- 3 files changed, 87 insertions(+), 6 deletions(-) -- 1.8.3.1
Commit '848ab68' left a issue: when we try to get a vcpupin info with no no flags or --current flags, we still will get the wrong error. Because VIR_DOMAIN_AFFECT_CURRENT is 0, so this check flags & VIR_DOMAIN_AFFECT_CURRENT will always get false. Signed-off-by: Luyao Huang <lhuang@redhat.com> Reported by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ac04ded..00b012b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6499,8 +6499,8 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) if (got_vcpu && vcpu >= ncpus) { if (flags & VIR_DOMAIN_AFFECT_LIVE || - (flags & VIR_DOMAIN_AFFECT_CURRENT && - virDomainIsActive(dom) == 1)) + ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) + == VIR_DOMAIN_AFFECT_CURRENT && virDomainIsActive(dom) == 1)) vshError(ctl, _("vcpu %d is out of range of live cpu count %d"), vcpu, ncpus); -- 1.8.3.1
On 07/14/2015 09:10 AM, Luyao Huang wrote:
Commit '848ab68' left a issue: when we try to get a vcpupin info with no no flags or --current flags, we still will get the wrong error. Because VIR_DOMAIN_AFFECT_CURRENT is 0, so this check flags & VIR_DOMAIN_AFFECT_CURRENT will always get false.
Signed-off-by: Luyao Huang <lhuang@redhat.com> Reported by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
hmmm ... see Michal's patch on this (and my response): http://www.redhat.com/archives/libvir-list/2015-July/msg00555.html Of course when I merged your change when I made the original adjustment based on review, I never ran it through my coverity checker either). John
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ac04ded..00b012b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6499,8 +6499,8 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd)
if (got_vcpu && vcpu >= ncpus) { if (flags & VIR_DOMAIN_AFFECT_LIVE || - (flags & VIR_DOMAIN_AFFECT_CURRENT && - virDomainIsActive(dom) == 1)) + ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) + == VIR_DOMAIN_AFFECT_CURRENT && virDomainIsActive(dom) == 1)) vshError(ctl, _("vcpu %d is out of range of live cpu count %d"), vcpu, ncpus);
On 07/15/2015 05:21 AM, John Ferlan wrote:
On 07/14/2015 09:10 AM, Luyao Huang wrote:
Commit '848ab68' left a issue: when we try to get a vcpupin info with no no flags or --current flags, we still will get the wrong error. Because VIR_DOMAIN_AFFECT_CURRENT is 0, so this check flags & VIR_DOMAIN_AFFECT_CURRENT will always get false.
Signed-off-by: Luyao Huang <lhuang@redhat.com> Reported by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
hmmm ... see Michal's patch on this (and my response):
http://www.redhat.com/archives/libvir-list/2015-July/msg00555.html
Of course when I merged your change when I made the original adjustment based on review, I never ran it through my coverity checker either).
yes, Michal must notice the coverity either, so his patch will fix this issue. Thanks a lot for your review.
John
Luyao
As there is a regression in use vcpupin get info, introduce a new function to test the virsh client. Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/test/test_driver.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/vcpupin | 34 +++++++++++++++++++++++++++---- 2 files changed, 85 insertions(+), 4 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index d38006f..213a9a1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2518,6 +2518,60 @@ static int testDomainPinVcpu(virDomainPtr domain, return ret; } +static int +testDomainGetVcpuPinInfo(virDomainPtr dom, + int ncpumaps, + unsigned char *cpumaps, + int maplen, + unsigned int flags) +{ + testDriverPtr privconn = dom->conn->privateData; + virDomainObjPtr privdom; + virDomainDefPtr def; + int ret = -1, hostcpus, vcpu; + virBitmapPtr allcpumap = NULL; + + if (!(privdom = testDomObjFromDomain(dom))) + return -1; + + if (!(def = virDomainObjGetOneDef(privdom, flags))) + goto cleanup; + + hostcpus = VIR_NODEINFO_MAXCPUS(privconn->nodeInfo); + + if (!(allcpumap = virBitmapNew(hostcpus))) + goto cleanup; + + virBitmapSetAll(allcpumap); + + /* Clamp to actual number of vcpus */ + if (ncpumaps > def->vcpus) + ncpumaps = def->vcpus; + + for (vcpu = 0; vcpu < ncpumaps; vcpu++) { + virDomainPinDefPtr pininfo; + virBitmapPtr bitmap = NULL; + + pininfo = virDomainPinFind(def->cputune.vcpupin, + def->cputune.nvcpupin, + vcpu); + + if (pininfo && pininfo->cpumask) + bitmap = pininfo->cpumask; + else + bitmap = allcpumap; + + virBitmapToDataBuf(bitmap, VIR_GET_CPUMAP(cpumaps, maplen, vcpu), maplen); + } + + ret = ncpumaps; + + cleanup: + virBitmapFree(allcpumap); + virDomainObjEndAPI(&privdom); + return ret; +} + static char *testDomainGetXMLDesc(virDomainPtr domain, unsigned int flags) { virDomainDefPtr def; @@ -6598,6 +6652,7 @@ static virHypervisorDriver testHypervisorDriver = { .domainGetVcpusFlags = testDomainGetVcpusFlags, /* 0.8.5 */ .domainPinVcpu = testDomainPinVcpu, /* 0.7.3 */ .domainGetVcpus = testDomainGetVcpus, /* 0.7.3 */ + .domainGetVcpuPinInfo = testDomainGetVcpuPinInfo, /* 1.2.18 */ .domainGetMaxVcpus = testDomainGetMaxVcpus, /* 0.7.3 */ .domainGetXMLDesc = testDomainGetXMLDesc, /* 0.1.4 */ .connectListDefinedDomains = testConnectListDefinedDomains, /* 0.1.11 */ diff --git a/tests/vcpupin b/tests/vcpupin index b6b8b31..213db93 100755 --- a/tests/vcpupin +++ b/tests/vcpupin @@ -66,12 +66,38 @@ error: vcpupin: Missing vCPU number in pin mode. EOF compare exp out || fail=1 -# without arguments. This should succeed but the backend function in the -# test driver isn't implemented -$abs_top_builddir/tools/virsh --connect test:///default vcpupin test > out 2>&1 +# An out-of-range vCPU number when get information with live flag +$abs_top_builddir/tools/virsh --connect test:///default vcpupin test 100 --live > out 2>&1 test $? = 1 || fail=1 cat <<\EOF > exp || fail=1 -error: this function is not supported by the connection driver: virDomainGetVcpuPinInfo +error: vcpu 100 is out of range of live cpu count 2 + +EOF +compare exp out || fail=1 + +# An out-of-range vCPU number when get information without flag +$abs_top_builddir/tools/virsh --connect test:///default vcpupin test 100 > out 2>&1 +test $? = 1 || fail=1 +cat <<\EOF > exp || fail=1 +error: vcpu 100 is out of range of live cpu count 2 + +EOF +compare exp out || fail=1 + +# An out-of-range vCPU number when get information with config flag +$abs_top_builddir/tools/virsh --connect test:///default vcpupin test 100 --config > out 2>&1 +test $? = 1 || fail=1 +cat <<\EOF > exp || fail=1 +error: vcpu 100 is out of range of persistent cpu count 2 + +EOF +compare exp out || fail=1 + +# An out-of-range vCPU number when get information with current flag +$abs_top_builddir/tools/virsh --connect test:///default vcpupin test 100 --current > out 2>&1 +test $? = 1 || fail=1 +cat <<\EOF > exp || fail=1 +error: vcpu 100 is out of range of live cpu count 2 EOF compare exp out || fail=1 -- 1.8.3.1
On 07/14/2015 09:10 AM, Luyao Huang wrote:
As there is a regression in use vcpupin get info, introduce a new function to test the virsh client.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/test/test_driver.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/vcpupin | 34 +++++++++++++++++++++++++++---- 2 files changed, 85 insertions(+), 4 deletions(-)
ACK and pushed the test (since 1/2 was fixed by a different commit) John
On 07/24/2015 06:52 PM, John Ferlan wrote:
On 07/14/2015 09:10 AM, Luyao Huang wrote:
As there is a regression in use vcpupin get info, introduce a new function to test the virsh client.
Signed-off-by: Luyao Huang <lhuang@redhat.com> --- src/test/test_driver.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/vcpupin | 34 +++++++++++++++++++++++++++---- 2 files changed, 85 insertions(+), 4 deletions(-)
ACK and pushed the test (since 1/2 was fixed by a different commit)
Thanks a lot for your review.
John
Luyao
participants (3)
-
John Ferlan -
lhuang -
Luyao Huang