On 10/24/2012 01:21 AM, Eric Blake wrote:
>> +
>> + 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).
Good point, the messages are not really useful.
>
> 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.
Right. I was just (ab)using what was currently there. Not sure whether
someone would be interested in an offline bitmap though. So maybe one
would want to have both flavors, online and offline and use a boolean
or enum to select the bitmap type.
>
> 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.
>
Great, thanks!
>> +
>> + 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?
>
Right, but either virBitmapToData should return the number of bits set or
a function as you suggest below is needed.
>> + 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.
>
Many thanks for reworking the preceding patches!
--
Mit freundlichen Grüßen/Kind Regards
Viktor Mihajlovski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list