[libvirt] freecell gives out bytes instead of kilobytes

[Daniel P. Berrange]

On Fri, Apr 03, 2009 at 12:04:05AM +0200, Gerrit Slomma wrote:
> The virsh-command freecell hands out bytes but affixes those with kB.
> The error ist found in virsh.c on line 1663 and following.
> I have corrected this and altered the output to the method i chose for 
> virt-manager - in the days of 96 GB per socket (Nehalem-EP) no one cares 
> even about a fraction of a Megabyte.

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 :|