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

> + > + 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. 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.