[libvirt] [PATCH 0/6] virsh: Fix 'vcpuinfo' on inactive VMs with new hotplug vcpus
See patch 6/6. Peter Krempa (6): util: bitmap: Make bitmaps const in virBitmapNewData and virBitmapDataToString virsh: domain: Fix broken indentation in virshCPUCountCollect virsh: Fix xpath queries for retrieving vcpu count virsh: Extract cpumap formatting in cmdVcpuinfo virsh: Extract fallback handling in cmdVcpuinfo virsh: vcpuinfo: Report proper vcpu numbers and data for offline VMs src/util/virbitmap.c | 6 +- src/util/virbitmap.h | 4 +- tools/virsh-domain.c | 287 ++++++++++++++++++++++++++++++++++++++------------- 3 files changed, 219 insertions(+), 78 deletions(-) -- 2.10.0
The functions just read the passed pointer so it can be marked as const. --- src/util/virbitmap.c | 6 +++--- src/util/virbitmap.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 3b85c16..0c04f1a 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -689,12 +689,12 @@ virBitmapPtr virBitmapNewCopy(virBitmapPtr src) * Returns a pointer to the allocated bitmap or NULL if * memory cannot be allocated. */ -virBitmapPtr virBitmapNewData(void *data, int len) +virBitmapPtr virBitmapNewData(const void *data, int len) { virBitmapPtr bitmap; size_t i, j; unsigned long *p; - unsigned char *bytes = data; + const unsigned char *bytes = data; bitmap = virBitmapNew(len * CHAR_BIT); if (!bitmap) @@ -1058,7 +1058,7 @@ virBitmapCountBits(virBitmapPtr bitmap) * Returns: a string representation of the data, or NULL on error */ char * -virBitmapDataToString(void *data, +virBitmapDataToString(const void *data, int len) { virBitmapPtr map = NULL; diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index 5984b80..58e0ee6 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -101,7 +101,7 @@ virBitmapParseUnlimited(const char *str, virBitmapPtr virBitmapNewCopy(virBitmapPtr src) ATTRIBUTE_NONNULL(1); -virBitmapPtr virBitmapNewData(void *data, int len) ATTRIBUTE_NONNULL(1); +virBitmapPtr virBitmapNewData(const void *data, int len) ATTRIBUTE_NONNULL(1); int virBitmapToData(virBitmapPtr bitmap, unsigned char **data, int *dataLen) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); @@ -138,7 +138,7 @@ ssize_t virBitmapNextClearBit(virBitmapPtr bitmap, ssize_t pos) size_t virBitmapCountBits(virBitmapPtr bitmap) ATTRIBUTE_NONNULL(1); -char *virBitmapDataToString(void *data, +char *virBitmapDataToString(const void *data, int len) ATTRIBUTE_NONNULL(1); bool virBitmapOverlaps(virBitmapPtr b1, -- 2.10.0
I managed to space most of the code by 5 spaces instead of 4 when orignally implementing this function. --- tools/virsh-domain.c | 68 ++++++++++++++++++++++++++-------------------------- 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index b19f499..03bf032 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6125,50 +6125,50 @@ virshCPUCountCollect(vshControl *ctl, return count; /* fallback code */ - if (!(last_error->code == VIR_ERR_NO_SUPPORT || - last_error->code == VIR_ERR_INVALID_ARG)) - goto cleanup; + if (!(last_error->code == VIR_ERR_NO_SUPPORT || + last_error->code == VIR_ERR_INVALID_ARG)) + goto cleanup; - if (flags & VIR_DOMAIN_VCPU_GUEST) { - vshError(ctl, "%s", _("Failed to retrieve vCPU count from the guest")); - goto cleanup; - } + if (flags & VIR_DOMAIN_VCPU_GUEST) { + vshError(ctl, "%s", _("Failed to retrieve vCPU count from the guest")); + goto cleanup; + } - if (!(flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) && - virDomainIsActive(dom) == 1) - flags |= VIR_DOMAIN_AFFECT_LIVE; + if (!(flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) && + virDomainIsActive(dom) == 1) + flags |= VIR_DOMAIN_AFFECT_LIVE; - vshResetLibvirtError(); + vshResetLibvirtError(); - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (flags & VIR_DOMAIN_VCPU_MAXIMUM) { + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (flags & VIR_DOMAIN_VCPU_MAXIMUM) { count = virDomainGetMaxVcpus(dom); - } else { - if (virDomainGetInfo(dom, &info) < 0) - goto cleanup; + } else { + if (virDomainGetInfo(dom, &info) < 0) + goto cleanup; - count = info.nrVirtCpu; - } - } else { - if (!(def = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE))) - goto cleanup; + count = info.nrVirtCpu; + } + } else { + if (!(def = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_INACTIVE))) + goto cleanup; if (!(xml = virXMLParseStringCtxt(def, _("(domain_definition)"), &ctxt))) goto cleanup; - if (flags & VIR_DOMAIN_VCPU_MAXIMUM) { - if (virXPathInt("string(/domain/vcpus)", ctxt, &count) < 0) { - vshError(ctl, "%s", _("Failed to retrieve maximum vcpu count")); - goto cleanup; - } - } else { - if (virXPathInt("string(/domain/vcpus/@current)", - ctxt, &count) < 0) { - vshError(ctl, "%s", _("Failed to retrieve current vcpu count")); - goto cleanup; - } - } - } + if (flags & VIR_DOMAIN_VCPU_MAXIMUM) { + if (virXPathInt("string(/domain/vcpus)", ctxt, &count) < 0) { + vshError(ctl, "%s", _("Failed to retrieve maximum vcpu count")); + goto cleanup; + } + } else { + if (virXPathInt("string(/domain/vcpus/@current)", + ctxt, &count) < 0) { + vshError(ctl, "%s", _("Failed to retrieve current vcpu count")); + goto cleanup; + } + } + } ret = count; cleanup: -- 2.10.0
The fallback code used if virDomainGetVcpusFlags is not supported used wrong XPath queries and basically did not work at all. Fix them to point to the <domain> <vcpu> element instead of <vcpus> which was not present until lately. --- tools/virsh-domain.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 03bf032..5fdad1b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6157,13 +6157,12 @@ virshCPUCountCollect(vshControl *ctl, goto cleanup; if (flags & VIR_DOMAIN_VCPU_MAXIMUM) { - if (virXPathInt("string(/domain/vcpus)", ctxt, &count) < 0) { + if (virXPathInt("string(/domain/vcpu)", ctxt, &count) < 0) { vshError(ctl, "%s", _("Failed to retrieve maximum vcpu count")); goto cleanup; } } else { - if (virXPathInt("string(/domain/vcpus/@current)", - ctxt, &count) < 0) { + if (virXPathInt("string(/domain/vcpu/@current)", ctxt, &count) < 0) { vshError(ctl, "%s", _("Failed to retrieve current vcpu count")); goto cleanup; } -- 2.10.0
cmdVcpuinfo will be split in upcomming patches thus extract the common code that formats pinning cpumaps for the vcpus. --- tools/virsh-domain.c | 52 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 17 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 5fdad1b..8e1b9ed 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6280,6 +6280,36 @@ static const vshCmdOptDef opts_vcpuinfo[] = { {.name = NULL} }; + +static int +virshVcpuinfoPrintAffinity(vshControl *ctl, + const unsigned char *cpumap, + int maxcpu, + bool pretty) +{ + char *str = NULL; + size_t i; + int ret = -1; + + vshPrint(ctl, "%-15s ", _("CPU Affinity:")); + if (pretty) { + if (!(str = virBitmapDataToString(cpumap, VIR_CPU_MAPLEN(maxcpu)))) + goto cleanup; + vshPrint(ctl, _("%s (out of %d)"), str, maxcpu); + } else { + for (i = 0; i < maxcpu; i++) + vshPrint(ctl, "%c", VIR_CPU_USED(cpumap, i) ? 'y' : '-'); + } + vshPrint(ctl, "\n"); + + ret = 0; + + cleanup: + VIR_FREE(str); + return ret; +} + + static bool cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd) { @@ -6291,7 +6321,7 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd) size_t cpumaplen; bool ret = false; bool pretty = vshCommandOptBool(cmd, "pretty"); - int n, m; + int n; virshControlPtr priv = ctl->privData; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) @@ -6340,23 +6370,11 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd) vshPrint(ctl, "%-15s %s\n", _("State:"), _("N/A")); vshPrint(ctl, "%-15s %s\n", _("CPU time"), _("N/A")); } - vshPrint(ctl, "%-15s ", _("CPU Affinity:")); - if (pretty) { - char *str; - str = virBitmapDataToString(VIR_GET_CPUMAP(cpumaps, cpumaplen, n), - cpumaplen); - if (!str) - goto cleanup; - vshPrint(ctl, _("%s (out of %d)"), str, maxcpu); - VIR_FREE(str); - } else { - for (m = 0; m < maxcpu; m++) { - vshPrint(ctl, "%c", - VIR_CPU_USABLE(cpumaps, cpumaplen, n, m) ? 'y' : '-'); - } - } - vshPrint(ctl, "\n"); + if (virshVcpuinfoPrintAffinity(ctl, VIR_GET_CPUMAP(cpumaps, cpumaplen, n), + maxcpu, pretty) < 0) + goto cleanup; + if (n < (ncpus - 1)) vshPrint(ctl, "\n"); } -- 2.10.0
Put it into a separate function so that more fallback handling can be added without making a mess. --- tools/virsh-domain.c | 75 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 54 insertions(+), 21 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 8e1b9ed..84a4854 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6311,6 +6311,49 @@ virshVcpuinfoPrintAffinity(vshControl *ctl, static bool +virshVcpuinfoInactive(vshControl *ctl, + virDomainPtr dom, + int nvcpus, + int maxcpu, + bool pretty) +{ + unsigned char *cpumaps = NULL; + size_t cpumaplen; + int ncpus; + size_t i; + bool ret = false; + + cpumaplen = VIR_CPU_MAPLEN(maxcpu); + cpumaps = vshMalloc(ctl, nvcpus * cpumaplen); + + if ((ncpus = virDomainGetVcpuPinInfo(dom, nvcpus, + cpumaps, cpumaplen, + VIR_DOMAIN_AFFECT_CONFIG)) < 0) + goto cleanup; + + for (i = 0; i < ncpus; i++) { + if (i != 0) + vshPrint(ctl, "\n"); + + vshPrint(ctl, "%-15s %zu\n", _("VCPU:"), i); + vshPrint(ctl, "%-15s %s\n", _("CPU:"), _("N/A")); + vshPrint(ctl, "%-15s %s\n", _("State:"), _("N/A")); + vshPrint(ctl, "%-15s %s\n", _("CPU time"), _("N/A")); + + if (virshVcpuinfoPrintAffinity(ctl, VIR_GET_CPUMAP(cpumaps, cpumaplen, i), + maxcpu, pretty) < 0) + goto cleanup; + } + + ret = true; + + cleanup: + VIR_FREE(cpumaps); + return ret; +} + + +static bool cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd) { virDomainInfo info; @@ -6343,32 +6386,22 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd) if (info.state != VIR_DOMAIN_SHUTOFF) goto cleanup; - /* fall back to virDomainGetVcpuPinInfo and free cpuinfo to mark this */ - VIR_FREE(cpuinfo); - if ((ncpus = virDomainGetVcpuPinInfo(dom, info.nrVirtCpu, - cpumaps, cpumaplen, - VIR_DOMAIN_AFFECT_CONFIG)) < 0) - goto cleanup; + /* for offline VMs we can return pinning information */ + ret = virshVcpuinfoInactive(ctl, dom, info.nrVirtCpu, maxcpu, pretty); + goto cleanup; } for (n = 0; n < ncpus; n++) { - if (cpuinfo) { - vshPrint(ctl, "%-15s %d\n", _("VCPU:"), cpuinfo[n].number); - vshPrint(ctl, "%-15s %d\n", _("CPU:"), cpuinfo[n].cpu); - vshPrint(ctl, "%-15s %s\n", _("State:"), - virshDomainVcpuStateToString(cpuinfo[n].state)); - if (cpuinfo[n].cpuTime != 0) { - double cpuUsed = cpuinfo[n].cpuTime; + vshPrint(ctl, "%-15s %d\n", _("VCPU:"), cpuinfo[n].number); + vshPrint(ctl, "%-15s %d\n", _("CPU:"), cpuinfo[n].cpu); + vshPrint(ctl, "%-15s %s\n", _("State:"), + virshDomainVcpuStateToString(cpuinfo[n].state)); + if (cpuinfo[n].cpuTime != 0) { + double cpuUsed = cpuinfo[n].cpuTime; - cpuUsed /= 1000000000.0; + cpuUsed /= 1000000000.0; - vshPrint(ctl, "%-15s %.1lfs\n", _("CPU time:"), cpuUsed); - } - } else { - vshPrint(ctl, "%-15s %d\n", _("VCPU:"), n); - vshPrint(ctl, "%-15s %s\n", _("CPU:"), _("N/A")); - vshPrint(ctl, "%-15s %s\n", _("State:"), _("N/A")); - vshPrint(ctl, "%-15s %s\n", _("CPU time"), _("N/A")); + vshPrint(ctl, "%-15s %.1lfs\n", _("CPU time:"), cpuUsed); } if (virshVcpuinfoPrintAffinity(ctl, VIR_GET_CPUMAP(cpumaps, cpumaplen, n), -- 2.10.0
If the VM is offline virsh attempted to at least report the pinning information for the VM. This would not work properly now that the vcpus can be sparse. Fix it by getting the vcpu states from the XML. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1375920 --- tools/virsh-domain.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 100 insertions(+), 9 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 84a4854..050e7fb 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6310,37 +6310,125 @@ virshVcpuinfoPrintAffinity(vshControl *ctl, } +static virBitmapPtr +virshDomainGetVcpuBitmap(vshControl *ctl, + virDomainPtr dom, + bool inactive) +{ + unsigned int flags = 0; + char *def = NULL; + virBitmapPtr ret = NULL; + xmlDocPtr xml = NULL; + xmlXPathContextPtr ctxt = NULL; + xmlNodePtr *nodes = NULL; + xmlNodePtr old; + int nnodes; + size_t i; + unsigned int curvcpus = 0; + unsigned int maxvcpus = 0; + unsigned int vcpuid; + char *online = NULL; + + if (inactive) + flags |= VIR_DOMAIN_XML_INACTIVE; + + if (!(def = virDomainGetXMLDesc(dom, flags))) + goto cleanup; + + if (!(xml = virXMLParseStringCtxt(def, _("(domain_definition)"), &ctxt))) + goto cleanup; + + if (virXPathUInt("string(/domain/vcpu)", ctxt, &maxvcpus) < 0) { + vshError(ctl, "%s", _("Failed to retrieve maximum vcpu count")); + goto cleanup; + } + + ignore_value(virXPathUInt("string(/domain/vcpu/@current)", ctxt, &curvcpus)); + + if (curvcpus == 0) + curvcpus = maxvcpus; + + if (!(ret = virBitmapNew(maxvcpus))) + goto cleanup; + + if ((nnodes = virXPathNodeSet("/domain/vcpus/vcpu", ctxt, &nodes)) <= 0) { + /* if the specific vcpu state is missing provide a fallback */ + for (i = 0; i < curvcpus; i++) + ignore_value(virBitmapSetBit(ret, i)); + + goto cleanup; + } + + old = ctxt->node; + + for (i = 0; i < nnodes; i++) { + ctxt->node = nodes[i]; + + if (virXPathUInt("string(@id)", ctxt, &vcpuid) < 0 || + !(online = virXPathString("string(@enabled)", ctxt))) + continue; + + if (STREQ(online, "yes")) + ignore_value(virBitmapSetBit(ret, vcpuid)); + + VIR_FREE(online); + } + + ctxt->node = old; + + if (virBitmapCountBits(ret) != curvcpus) { + vshError(ctl, "%s", _("Failed to retrieve vcpu state bitmap")); + virBitmapFree(ret); + ret = NULL; + } + + cleanup: + VIR_FREE(online); + VIR_FREE(nodes); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(xml); + VIR_FREE(def); + return ret; +} + + static bool virshVcpuinfoInactive(vshControl *ctl, virDomainPtr dom, - int nvcpus, int maxcpu, bool pretty) { unsigned char *cpumaps = NULL; size_t cpumaplen; int ncpus; - size_t i; + virBitmapPtr vcpus = NULL; + ssize_t nextvcpu = -1; bool ret = false; + bool first = true; + + if (!(vcpus = virshDomainGetVcpuBitmap(ctl, dom, true))) + goto cleanup; cpumaplen = VIR_CPU_MAPLEN(maxcpu); - cpumaps = vshMalloc(ctl, nvcpus * cpumaplen); + cpumaps = vshMalloc(ctl, virBitmapSize(vcpus) * cpumaplen); - if ((ncpus = virDomainGetVcpuPinInfo(dom, nvcpus, + if ((ncpus = virDomainGetVcpuPinInfo(dom, virBitmapSize(vcpus), cpumaps, cpumaplen, VIR_DOMAIN_AFFECT_CONFIG)) < 0) goto cleanup; - for (i = 0; i < ncpus; i++) { - if (i != 0) + while ((nextvcpu = virBitmapNextSetBit(vcpus, nextvcpu)) >= 0) { + if (!first) vshPrint(ctl, "\n"); + first = false; - vshPrint(ctl, "%-15s %zu\n", _("VCPU:"), i); + vshPrint(ctl, "%-15s %zd\n", _("VCPU:"), nextvcpu); vshPrint(ctl, "%-15s %s\n", _("CPU:"), _("N/A")); vshPrint(ctl, "%-15s %s\n", _("State:"), _("N/A")); vshPrint(ctl, "%-15s %s\n", _("CPU time"), _("N/A")); - if (virshVcpuinfoPrintAffinity(ctl, VIR_GET_CPUMAP(cpumaps, cpumaplen, i), + if (virshVcpuinfoPrintAffinity(ctl, + VIR_GET_CPUMAP(cpumaps, cpumaplen, nextvcpu), maxcpu, pretty) < 0) goto cleanup; } @@ -6348,6 +6436,7 @@ virshVcpuinfoInactive(vshControl *ctl, ret = true; cleanup: + virBitmapFree(vcpus); VIR_FREE(cpumaps); return ret; } @@ -6386,8 +6475,10 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd) if (info.state != VIR_DOMAIN_SHUTOFF) goto cleanup; + vshResetLibvirtError(); + /* for offline VMs we can return pinning information */ - ret = virshVcpuinfoInactive(ctl, dom, info.nrVirtCpu, maxcpu, pretty); + ret = virshVcpuinfoInactive(ctl, dom, maxcpu, pretty); goto cleanup; } -- 2.10.0
On 12.10.2016 00:08, Peter Krempa wrote:
See patch 6/6.
Peter Krempa (6): util: bitmap: Make bitmaps const in virBitmapNewData and virBitmapDataToString virsh: domain: Fix broken indentation in virshCPUCountCollect virsh: Fix xpath queries for retrieving vcpu count virsh: Extract cpumap formatting in cmdVcpuinfo virsh: Extract fallback handling in cmdVcpuinfo virsh: vcpuinfo: Report proper vcpu numbers and data for offline VMs
src/util/virbitmap.c | 6 +- src/util/virbitmap.h | 4 +- tools/virsh-domain.c | 287 ++++++++++++++++++++++++++++++++++++++------------- 3 files changed, 219 insertions(+), 78 deletions(-)
ACK series. Michal
participants (2)
-
Michal Privoznik -
Peter Krempa