On 22.09.2014 12:14, Jincheng Miao wrote:

The unit of '--pagesize' of freepages is kibibytes.

https://bugzilla.redhat.com/show_bug.cgi?id=1145048

Signed-off-by: Jincheng Miao <jmiao@redhat.com> --- tools/virsh-host.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/tools/virsh-host.c b/tools/virsh-host.c index 7fc2120..6dcf77e 100644 --- a/tools/virsh-host.c +++ b/tools/virsh-host.c @@ -293,7 +293,7 @@ cmdFreepages(vshControl *ctl, const vshCmd *cmd) bool ret = false; unsigned int npages; unsigned int *pagesize = NULL; - unsigned long long tmp = 0; + unsigned int tmp = 0; int cell; unsigned long long *counts = NULL; size_t i, j; @@ -304,9 +304,20 @@ cmdFreepages(vshControl *ctl, const vshCmd *cmd) xmlXPathContextPtr ctxt = NULL; bool all = vshCommandOptBool(cmd, "all"); bool cellno = vshCommandOptBool(cmd, "cellno"); + bool pagesz = vshCommandOptBool(cmd, "pagesize");

VSH_EXCLUSIVE_OPTIONS_VAR(all, cellno);

+ if (vshCommandOptUInt(cmd, "pagesize", &tmp) < 0) { + vshError(ctl, "%s", _("Invalid pagesize argument")); + goto cleanup; + } +

No, previously we accepted something like '--pagesize 2M'. This undo this feature.

+ if ((pagesz || cellno) && tmp < 1) { + vshError(ctl, "%s", _("page size must be at least 1KiB")); + goto cleanup; + } + if (all) { if (!(cap_xml = virConnectGetCapabilities(ctl->conn))) { vshError(ctl, "%s", _("unable to get node capabilities")); @@ -363,6 +374,8 @@ cmdFreepages(vshControl *ctl, const vshCmd *cmd)

vshPrint(ctl, _("Node %d:\n"), cell); for (j = 0; j < npages; j++) { + if (pagesz && tmp != pagesize[j]) + continue;

Instead of this, I'd honor --pagesize when creating @pagesize array to call viNodeGetFreePages() a few lines above.

vshPrint(ctl, "%uKiB: %lld\n", pagesize[j], counts[j]); } vshPrint(ctl, "%c", '\n'); @@ -380,20 +393,16 @@ cmdFreepages(vshControl *ctl, const vshCmd *cmd) }

if (cell < -1) { - vshError(ctl, "%s", _("cell number must be non-negative integer or -1")); + vshError(ctl, "%s", + _("cell number must be non-negative integer or -1")); goto cleanup; }

- if (vshCommandOptScaledInt(cmd, "pagesize", &tmp, 1, UINT_MAX) < 0) {

In fact this is the bit that's buggy here. It should have been 1024 instead of 1.

- vshError(ctl, "%s", _("page size has to be a number")); - goto cleanup; - } /* page size is expected in kibibytes */ pagesize = vshMalloc(ctl, sizeof(*pagesize)); - *pagesize = tmp / 1024;

- if (!pagesize[0]) { - vshError(ctl, "%s", _("page size must be at least 1KiB")); + if (vshCommandOptUInt(cmd, "pagesize", pagesize) < 0) { + vshError(ctl, "%s", _("Invalid cellno argument")); goto cleanup; }

Michal

On 22.09.2014 12:14, Jincheng Miao wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1145050

Signed-off-by: Jincheng Miao <jmiao@redhat.com> --- src/nodeinfo.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c index af23b8b..1728891 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -2028,16 +2028,34 @@ nodeGetFreePages(unsigned int npages, unsigned long long *counts) { int ret = -1; - int cell; + int cell, lastCell; size_t i, ncounts = 0;

- for (cell = startCell; cell < (int) (startCell + cellCount); cell++) { + if ((lastCell = virNumaGetMaxNode()) < 0) + return 0; + + if (startCell > lastCell) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("start cell %d out of range (0-%d)"), + startCell, lastCell); + goto cleanup; + } + + lastCell = startCell + cellCount; + if (startCell + cellCount < lastCell) + lastCell = startCell + cellCount; + + for (cell = startCell; cell < lastCell; cell++) { for (i = 0; i < npages; i++) { unsigned int page_size = pages[i]; unsigned int page_free;

- if (virNumaGetPageInfo(cell, page_size, 0, NULL, &page_free) < 0) + if (virNumaGetPageInfo(cell, page_size, 0, NULL, &page_free) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get %dKib page info of cell %d"), + page_size, cell);

Noo. This overwrites any error reported by virNumaGetPageInfo (which usually is much more helpful than this generic one).

goto cleanup; + }

counts[ncounts++] = page_free; }

ACK with that discarded. Michal

On 22.09.2014 12:14, Jincheng Miao wrote:

Signed-off-by: Jincheng Miao <jmiao@redhat.com> --- src/libvirt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/libvirt.c b/src/libvirt.c index 7c63825..aa83365 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -21307,7 +21307,7 @@ virDomainSetTime(virDomainPtr dom, * @flags: extra flags; not used yet, so callers should always pass 0 * * This calls queries the host system on free pages of - * specified size. Ont the input, @pages is expected to be + * specified size. For the input, @pages is expected to be * filled with pages that caller is interested in (the size * unit is kibibytes, so e.g. pass 2048 for 2MB), then @startcell * refers to the first NUMA node that info should be collected

ACK Michal