On 05/22/2018 11:54 AM, Roland Schulz wrote:

Signed-off-by: Roland Schulz <schullzroll@gmail.com> --- tools/virsh-completer.c | 15 ++++++++++++++- tools/virsh-host.c | 2 +- 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c index 2816e7b76..21c73f048 100644 --- a/tools/virsh-completer.c +++ b/tools/virsh-completer.c @@ -579,6 +579,9 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, double size = 0; size_t i = 0; const char *suffix = NULL; + const char *cellnum = NULL; + bool cellno = vshCommandOptBool(cmd, "cellno"); + char *path = NULL; char *pagesize = NULL; char *cap_xml = NULL; char **ret = NULL; @@ -595,7 +598,17 @@ virshAllocpagesPagesizeCompleter(vshControl *ctl, if (!(virXMLParseStringCtxt(cap_xml, _("capabilities"), &ctxt))) goto error;

- npages = virXPathNodeSet("/capabilities/host/cpu/pages", ctxt, &pages); + if (cellno && vshCommandOptStringQuiet(ctl, cmd, "cellno", &cellnum) > 0) { + if (virAsprintf(&path, + "/capabilities/host/topology/cells/cell[@id=\"%s\"]/pages", + cellnum) < 0) + goto error; + } + else + if (virAsprintf(&path, "/capabilities/host/cpu/pages") < 0) + goto error; +

What an innovative way of avoiding syntax-check rule ;-) This should be wrapped by curly braces too.

+ npages = virXPathNodeSet(path, ctxt, &pages); if (npages <= 0) goto error;

So you're allocating @path but never free it.

diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 293f06e9e..793a10452 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -473,7 +473,7 @@ static const vshCmdOptDef opts_allocpages[] = { .type = VSH_OT_INT, .flags = VSH_OFLAG_REQ, .completer = virshAllocpagesPagesizeCompleter, - .help = N_("page size (in kibibytes)") + .help = N_("page size")

This change is not necessary. 'allocpages --pagesize 2048' just works, just like 'allocpages --pagesize 2MiB'. I'm fixing the curly braces and mem leak problems, ACKing and pushing. However, there are couple of problems that deserve fixing: 1) @pages shouldn't be freed individually like they are under the cleanup label. 2) There's still one memleak (retval of virXMLParseStringCtxt) 3) The misordering of variable declaration and free calls makes it hard to follow which variables are freed and which are forgotten. Michal

On 05/22/2018 11:54 AM, Roland Schulz wrote:

Signed-off-by: Roland Schulz <schullzroll@gmail.com> --- src/test/test_driver.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)

This one is a bit tricky.

diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 467587b19..40c366cb8 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -322,24 +322,27 @@ testBuildCapabilities(virConnectPtr conn) if (virCapabilitiesAddHostFeature(caps, "nonpae") < 0) goto error;

- if (VIR_ALLOC_N(caps->host.pagesSize, 2) < 0) + if (VIR_ALLOC_N(caps->host.pagesSize, 3) < 0) goto error;

caps->host.pagesSize[caps->host.nPagesSize++] = 4; caps->host.pagesSize[caps->host.nPagesSize++] = 2048; + caps->host.pagesSize[caps->host.nPagesSize++] = 1024 * 1024;

So numa node 1 supports 8K pages but not 4K. Okay, but in this case the overall @caps should report all 4K, 8K, 2M and 1G.

for (i = 0; i < privconn->numCells; i++) { virCapsHostNUMACellCPUPtr cpu_cells; virCapsHostNUMACellPageInfoPtr pages; - size_t nPages; + size_t nPages = caps->host.nPagesSize;

if (VIR_ALLOC_N(cpu_cells, privconn->cells[i].numCpus) < 0 || VIR_ALLOC_N(pages, caps->host.nPagesSize) < 0) { VIR_FREE(cpu_cells); goto error; } - - nPages = caps->host.nPagesSize; + if (i == 1) { + nPages--; + caps->host.pagesSize[caps->host.nPagesSize - 3] = 8;

This makes capabilities report 8K, 2M and 1G. No 4K. That doesn't look right. What we want is a bit more complicated. Let's have any node but #1 support 4K, 2M and 1G, and node #1 support 8K, 2M, and 1G. The code will have to look slightly different in that case. Looking forward to v2. Michal