[libvirt] [PATCH] Fix virCgroupGetPercpuStats with non-continuous present CPUs

[Michal Privoznik]

On 22.01.2015 15:52, Ján Tomko wrote:
> On 01/22/2015 03:21 PM, Michal Privoznik wrote:
>> On 22.01.2015 12:26, Ján Tomko wrote:
>>> Per-cpu stats are only shown for present CPUs in the cgroups,
>>> but we were only parsing the largest CPU number from
>>> /sys/devices/system/cpu/present and looking for stats even for
>>> non-present CPUs.
>>> This resulted in:
>>> internal error: cpuacct parse error
>>> ---
>>>  cfg.mk                |  2 +-
>>>  src/nodeinfo.c        | 17 +++++++++++++++++
>>>  src/nodeinfo.h        |  1 +
>>>  src/util/vircgroup.c  | 24 ++++++++++++++++++------
>>>  tests/vircgroupmock.c | 44 ++++++++++++++++++++++++++++++++++++++------
>>>  tests/vircgrouptest.c | 47 +++++++++++++++++++++++++++++++++++------------
>>>  6 files changed, 110 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/cfg.mk b/cfg.mk
>>> index 21f83c3..70612f8 100644
>>> --- a/cfg.mk
>>> +++ b/cfg.mk
>>> @@ -1095,7 +1095,7 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \
>>>    ^(bootstrap.conf$$|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$)
>>>  
>>>  exclude_file_name_regexp--sc_prohibit_strdup = \
>>> -  ^(docs/|examples/|src/util/virstring\.c|tests/virnetserverclientmock.c$$)
>>> +  ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$)
>>>  
>>>  exclude_file_name_regexp--sc_prohibit_close = \
>>>    (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir(cgroup|pci)mock\.c)$$)
>>> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
>>> index 3c22ebc..3a27c22 100644
>>> --- a/src/nodeinfo.c
>>> +++ b/src/nodeinfo.c
>>> @@ -1242,6 +1242,23 @@ nodeGetCPUCount(void)
>>>  }
>>>  
>>>  virBitmapPtr
>>> +nodeGetPresentCPUBitmap(void)
>>> +{
>>> +    int max_present;
>>> +
>>> +    if ((max_present = nodeGetCPUCount()) < 0)
>>> +        return NULL;
>>> +
>>> +#ifdef __linux__
>>> +    if (virFileExists(SYSFS_SYSTEM_PATH "/cpu/present"))
>>> +        return linuxParseCPUmap(max_present, SYSFS_SYSTEM_PATH "/cpu/present");
>>> +#endif
>>> +    virReportError(VIR_ERR_NO_SUPPORT, "%s",
>>> +                   _("non-continuous host cpu numbers not implemented on this platform"));
>>> +    return NULL;
>>> +}
>>> +
>>> +virBitmapPtr
>>>  nodeGetCPUBitmap(int *max_id ATTRIBUTE_UNUSED)
>>>  {
>>
>> >From my understanding the nodeGetPresentCPUBitmap() is no different than
>> nodeGetCPUBitmap(). The latter can do everything that the former and
>> more. Can we just use already existing function then?
>>
> 
> This functions checks the present CPUs, nodeGetCPUBitmap checks the online CPUs.
> 
> I don't think splitting their common part into another function would be more
> readable - IMO the fact that both use linuxParseCPUmap should be enough.


Okay, ACK then.

Michal