Re: [libvirt] freecell gives out bytes instead of kilobytes

On Fri, Apr 03, 2009 at 12:04:05AM +0200, Gerrit Slomma wrote: For command line tools like virsh I prefer to have it consistently report in the same units, so if someone wants to script it from the shell they don't have to concern themselves with changing units. All the other virsh commands report in KB, so the simple fix is to just divide by 1024. In checking this I discovered a whole bunch of other fun bugs in the NUMA support :-) The QEMU impl was not returning the correct return code - it used -1 instead of 0. The QEMU impl was also not setting an error if the requested cell was out of range. The libvirtd remote driver was not correctly seeing return value of -1 due to casting it to an unsigned int. virsh was not checking return values correctly either. So I propose the following patch.... Index: qemud/remote.c =================================================================== RCS file: /data/cvs/libvirt/qemud/remote.c,v retrieving revision 1.65 diff -u -p -r1.65 remote.c --- qemud/remote.c 16 Mar 2009 10:33:01 -0000 1.65 +++ qemud/remote.c 3 Apr 2009 10:18:38 -0000 @@ -662,6 +662,7 @@ remoteDispatchNodeGetCellsFreeMemory (st remote_node_get_cells_free_memory_args *args, remote_node_get_cells_free_memory_ret *ret) { + int err; if (args->maxCells > REMOTE_NODE_MAX_CELLS) { remoteDispatchFormatError (rerr, @@ -675,15 +676,16 @@ remoteDispatchNodeGetCellsFreeMemory (st return -1; } - ret->freeMems.freeMems_len = virNodeGetCellsFreeMemory(conn, - (unsigned long long *)ret->freeMems.freeMems_val, - args->startCell, - args->maxCells); - if (ret->freeMems.freeMems_len == 0) { + err = virNodeGetCellsFreeMemory(conn, + (unsigned long long *)ret->freeMems.freeMems_val, + args->startCell, + args->maxCells); + if (err <= 0) { VIR_FREE(ret->freeMems.freeMems_val); remoteDispatchConnError(rerr, conn); return -1; } + ret->freeMems.freeMems_len = err; return 0; } Index: src/qemu_driver.c =================================================================== RCS file: /data/cvs/libvirt/src/qemu_driver.c,v retrieving revision 1.223 diff -u -p -r1.223 qemu_driver.c --- src/qemu_driver.c 1 Apr 2009 09:54:20 -0000 1.223 +++ src/qemu_driver.c 3 Apr 2009 10:18:38 -0000 @@ -1878,15 +1878,23 @@ qemudNodeGetCellsFreeMemory(virConnectPt { int n, lastCell, numCells; int ret = -1; + int maxCell; if (numa_available() < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_NO_SUPPORT, "%s", _("NUMA not supported on this host")); goto cleanup; } + maxCell = numa_max_node(); + if (startCell > maxCell) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("start cell %d out of range (0-%d)"), + startCell, maxCell); + goto cleanup; + } lastCell = startCell + maxCells - 1; - if (lastCell > numa_max_node()) - lastCell = numa_max_node(); + if (lastCell > maxCell) + lastCell = maxCell; for (numCells = 0, n = startCell ; n <= lastCell ; n++) { long long mem; @@ -1906,7 +1914,7 @@ cleanup: static unsigned long long qemudNodeGetFreeMemory (virConnectPtr conn) { - unsigned long long freeMem = -1; + unsigned long long freeMem = 0; int n; if (numa_available() < 0) { Index: src/virsh.c =================================================================== RCS file: /data/cvs/libvirt/src/virsh.c,v retrieving revision 1.198 diff -u -p -r1.198 virsh.c --- src/virsh.c 1 Apr 2009 09:52:59 -0000 1.198 +++ src/virsh.c 3 Apr 2009 10:18:38 -0000 @@ -1654,6 +1654,8 @@ cmdFreecell(vshControl *ctl, const vshCm cell = vshCommandOptInt(cmd, "cellno", &cell_given); if (!cell_given) { memory = virNodeGetFreeMemory(ctl->conn); + if (memory == 0) + return FALSE; } else { ret = virNodeGetCellsFreeMemory(ctl->conn, &memory, cell, 1); if (ret != 1) @@ -1661,9 +1663,9 @@ cmdFreecell(vshControl *ctl, const vshCm } if (cell == -1) - vshPrint(ctl, "%s: %llu kB\n", _("Total"), memory); + vshPrint(ctl, "%s: %llu kB\n", _("Total"), (memory/1024)); else - vshPrint(ctl, "%d: %llu kB\n", cell, memory); + vshPrint(ctl, "%d: %llu kB\n", cell, (memory/1024)); return TRUE; } Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|