Re: [Libvir] RFC PATCH - Initial NodeGetCellsFreeMemory patch
Richard W.M. Jones wrote:
beth kon wrote:
Comments on the code itself:
+ * @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?
maxCells is size of the freeMems array. I will clarify.
- 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;
yep. I will clean up the maxCells and startCell code.
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.
I saw other cases where this was done and was following that precedent,
though usually I did call an error function. In any case, it is
obviously better to do so, so I will correct this.
+ /* 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)?
It is returned by virNodeGetInfo, and also in the virNodeGetTopology XML
string (which I'm currently working on). Do you think it should also be
included here? I would think it reasonable to assume the caller has done
virNodeGetInfo before virNodeGetCellsFreeMemory, though it is easy
enough to add here.
+ 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'?
I will rework all this so we can discuss the new version.
Thanks for the comments!
Rich.
--
Elizabeth Kon (Beth)
IBM Linux Technology Center
Open Hypervisor Team
email: eak(a)us.ibm.com