[libvirt] [PATCHv2 7/9] virNodeGetCPUMapFlags: Implement support function in nodeinfo

[Eric Blake]

On 10/16/2012 08:05 AM, Viktor Mihajlovski wrote:
> Added an implemention of virNodeGetCPUMapFlags to nodeinfo.c,

s/implemention/implementation/

s/Flags//g

> (nodeGetCPUMapFlags) which can be used by all drivers for a Linux
> hypervisor host.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov at linux.vnet.ibm.com>
> ---
>  src/libvirt_private.syms |    1 +
>  src/nodeinfo.c           |   49 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/nodeinfo.h           |    6 +++++
>  3 files changed, 56 insertions(+), 0 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index fe31bbe..44d5927 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -900,6 +900,7 @@ virNodeDeviceObjUnlock;
>  # nodeinfo.h
>  nodeCapsInitNUMA;
>  nodeGetCPUmap;
> +nodeGetCPUMapFlags;

Ah, I see what you are up against.  Well, like I said in 1/9, we should
not hold the public API hostage to internal naming, so I'd prefer to
s/nodeGetCPUmap/nodeGetCPUBitmap for the internal function.

>  nodeGetCPUStats;
>  nodeGetCellsFreeMemory;
>  nodeGetFreeMemory;
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index c0e60d8..84cf796 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -1203,6 +1203,55 @@ nodeGetMemoryParameters(virConnectPtr conn ATTRIBUTE_UNUSED,
>  #endif
>  }
>  
> +int nodeGetCPUMapFlags(virConnectPtr conn,
> +                       unsigned char **cpumap,
> +                       unsigned int *online,
> +                       unsigned int flags)
> +{
> +    virBitmapPtr cpusPresent = NULL;
> +    virBitmapPtr cpusOnline = NULL;
> +    int maxpresent, maxonline, i;
> +    int ret = -1;
> +
> +    virCheckFlags(0, 1);

s/1/-1/ - you must fail for unknown flags, rather than treating it like
returning a single online CPU.

> +
> +    if (!(cpusPresent = nodeGetCPUmap(conn, &maxpresent, "present"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Unable to retrieve 'present' CPU map"));
> +        goto cleanup;
> +    }
> +
> +    if (!(cpusOnline = nodeGetCPUmap(conn, &maxonline, "online"))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Unable to retrieve 'online' CPU map"));
> +        goto cleanup;
> +    }

You are throwing away the inner error message, which may have been more
informative (such as OOM or missing platform support).

Eww.  Why should we have to collect two bitmaps?  nodeGetCPUmap (which
I'm renaming to nodeGetCPUBitmap) is returning non-useful information -
the "present" map will always be contiguous, and the only interesting
data is in "online"; meanwhile, the maximum online cpu for "online" is
not what we care about, so much as the maximum present.

For that matter, qemu_driver.c is using nodeGetCPUmap incorrectly - it
is trying to collect the list of online CPUs (which is variable), but
passes "present" instead of "online" (the list of present CPUs is not
variable, at least not on bare metal; I suppose it is variable in a
guest once vcpu hotplug works, but that's a completely different level
of complication).  I argue that nodeGetCPUmap shouldn't need a 'name'
argument, but should just magically give us the max present and the
online bitmap every time.

I'm going to do some major refactoring of this area of code as a
preliminary, and then we can rebase this patch on top of my improvements.

> +
> +    if (cpumap && VIR_ALLOC_N(*cpumap, VIR_CPU_MAPLEN(maxpresent)) < 0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    if (online)
> +        *online = 0;
> +
> +    i = -1;
> +    while ((i=virBitmapNextSetBit(cpusOnline, i)) >= 0) {

Space around operators.  Besides, isn't virBitmapToData better than
rolling this loop by hand?

> +        if (online)
> +            (*online)++;

This argues that util/bitmap.h should have a function for returning
bitmap population.

> +
> +        if (cpumap)
> +            VIR_USE_CPU(*cpumap,i);

Space after comma.

I'm going to commit the earlier patches in this series today (to hit the
freeze deadline, so that we guarantee that we are committed to the API
for 1.0.0), while leaving this patch and later for further work for
after the freeze when I finish my improvements.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121023/2a60e063/attachment-0001.sig>