[Libvir] RFC PATCH - Initial NodeGetCellsFreeMemory patch

[Richard W.M. Jones]

beth kon wrote:
> Here is my first pass at a patch for accessing the available memory 
> associated with each NUMA cell through libvirt.
> 
> The initial framework patch provided by Daniel Veillard is a prereq of 
> this patch:
> https://www.redhat.com/archives/libvir-list/2007-September/msg00069.html
> 
> I have not yet tested this. I'm just distributing for comments.
> 
> A few comments/questions:
> 1) I think I got the versioning stuff right but that deserves a close look.
> 2) In Daniel's patch, libvirt.h and libvirt.h.in were identical. I 
> assumed the patch would be before running autogen.sh, and only contain 
> changes in libvirt.h.in, so that's how I did mine.  Let me know if I 
> misunderstand something here.

That fine, although be aware that libvirt.h only gets recreated when you 
run ./configure explicitly.  I think there should be a dependency in the 
Makefile so that it can be recreated from config.status, but that's 
another bug so don't worry about it for this.

> 3) I had to put #ifdef PROXY around calls to virXenErrorFunc in 
> xenHypervisorNodeGetCellsFreeMemory to get this to build. I haven't 
> figured out how the proxy code works so am not sure if this is the right 
> approach.

As a general background, the Xen proxy was an earlier attempt to allow 
query Xen without having to be root.  The Xen proxy was a setuid binary 
which only contains "safe" read-only code.  It was built from the same 
sources as libvirt but with any code thought to be unsafe disabled 
(#ifndef PROXY ... #endif).

In any case the proxy ability has been completely superseded by a 
combination of remote access and PolicyKit.  In fact I think Dan was 
planning to remove the Xen proxy entirely.

Comments on the code itself:

  int			 virNodeGetCellsFreeMemory(virConnectPtr conn,
-						   unsigned long *freeMems,
-						   int nbCells);
+						   long long *freeMems,
+						   int startCell,
+						   int maxCells);

I guess Daniel Veillard can comment on this change.  Did we decide that 
this wouldn't be useful on realistic topologies because interesting cell 
ranges would never be contiguous?

+ * @freeMems: pointer to the array of long long
+ * @startCell: index of first cell to return freeMems info on (0 to
+ *             maxCells - 1).
+ * @maxCells: number of entries available in freeMems (-1 for sum of
+ *            free memory across all cells, returned in freeMems[0])

I'm slightly confused by this documentation.  If maxCells > 0, does this 
only fill a subset of the array freeMems?

-    if ((freeMems == NULL) || (nbCells <= 0)) {
+    if ((freeMems == NULL) || (maxCells <= 0) || (startCell > maxCells 
- 1)) {
          virLibConnError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__);
          return (-1);
      }

But we allow maxCells == -1 though, so this test is wrong.

+    if ((conn == NULL) || (freeMems == NULL) || (maxCells < 0))
+        return -1;

Should always call the error function before returning an error. 
Unfortunately the error handling in libvirt is over-engineered in the 
extreme and, what is more, in order to get around this over-engineering 
we have defined "simpler" wrapper error functions in different / 
incompatible ways in each file.  In this particular file 
(xen_internal.c) the "simpler" error function is called virXenErrorFunc.


+    /* get actual number of cells */
+    if (xenDaemonNodeGetInfo(conn, nodeInfoPtr)) {
+        virXenError(VIR_ERR_XEN_CALL, " cannot determine actual number 
of cells", 0);
+        return -1;
+    }
+    nbCells = nodeInfoPtr->nodes;

Is there a way for me to find out the total number of cells (nbCells)?

+    if (maxCells > nbCells)
+        maxCells = nbCells;

I would prefer to return an error here, but it's not too bad because the 
return value tells us how many cells were filled.

+    if (startCell > nbCells - 1)
+        return -1;

Surely, the condition should be `startCell + maxCells > nbCells'?

Rich.


-- 
Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/
Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod
Street, Windsor, Berkshire, SL4 1TE, United Kingdom.  Registered in
England and Wales under Company Registration No. 03798903
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3237 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20070921/13d9c8fe/attachment-0001.bin>