[Libvir] RFC PATCH - Initial NodeGetCellsFreeMemory patch

[beth kon]

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 at us.ibm.com