Re: [PATCH for-4.5 v2 2/2] tools/[lib]xl: Fix `xl vcpu-list` when domains without vcpus exist

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 21/10/14 14:06, Wei Liu wrote:
> On Mon, Oct 20, 2014 at 05:38:54PM +0100, Andrew Cooper wrote:
>> On a system which looks like this:
>>
>> [root@st04 ~]# xl list
>> Name                                        ID   Mem VCPUs      State   Time(s)
>> Domain-0                                     0   752     4     r-----   46699.3
>> (null)                                       1     0     0     --p---       0.0
>> (null)                                       2     0     0     --p---       0.0
>> (null)                                       3     0     0     --p---       0.0
>> badger                                      25     0     1     --p---       0.0
>>
>> `xl vcpu-list` failes as so:
>>
>> [root@st04 ~]# xl vcpu-list
>> Name                                ID  VCPU   CPU State   Time(s) CPU Affinity
>> Domain-0                             0     0    0   -b-   12171.0  all
>> Domain-0                             0     1    1   -b-   11779.6  all
>> Domain-0                             0     2    2   -b-   11599.0  all
>> Domain-0                             0     3    3   r--   11007.0  all
>> libxl: critical: libxl__calloc: libxl: FATAL ERROR: memory allocation failure (libxl__calloc, 4294935299 x 40)
>> : Cannot allocate memory
>> libxl: FATAL ERROR: memory allocation failure (libxl__calloc, 4294935299 x 40)
>>
>> The root cause of this is that, in the case that a domain has no vcpus,
>> libxl_list_vcpu() attempts to calloc() with domaininfo.max_vcpu_id, which
>> is uninitialised on the stack.
>>
>> Check domaininfo.max_vcpu_id against the new sentinel value
>> XEN_INVALID_MAX_VCPU_ID (introduced in the previous patch), and return early.
>> This means that it is now valid for libxl_list_vcpu() to return NULL for a
>> domain which lacks any vcpus.
>>
>> As part of this change, remove the pointless call to libxl_get_max_cpus(),
>> whose returned value is unconditionally clobbered in the for() loop.
>>
>> Reported-by: Euan Harris <euan.harris@citrix.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>>
>> ---
>> v2: s/nr_online_cpus/max_vcpu_id/g - I accidentally sent an unrefreshed patch
>> ---
>>  tools/libxl/libxl.c      |    8 +++++++-
>>  tools/libxl/xl_cmdimpl.c |    4 +---
>>  2 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index f27b581..b7a9952 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -5246,7 +5246,13 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
>>          GC_FREE;
>>          return NULL;
>>      }
>> -    *nr_cpus_out = libxl_get_max_cpus(ctx);
>> +
>> +    if (domaininfo.max_vcpu_id == XEN_INVALID_MAX_VCPU_ID) {
>> +        *nr_cpus_out = 0;
> Do you not need to set nr_vcpus_out to 0 as well?

Hmm - probably not.  NULL should do.

I will respin a v3 with the hypervisor and tool sides merged

~Andrew