[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Libvir] RFC PATCH - Initial NodeGetCellsFreeMemory patch



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

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]