Re: [Libvir] RFC PATCH - Initial NodeGetCellsFreeMemory patch

beth kon wrote: 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. 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