Re: [PATCH 2/2] tools/libs/light: don't touch nr_vcpus_out if listing vcpus and returning NULL

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Jan 12, 2022 at 05:41:42PM +0100, Dario Faggioli wrote:
> If we are in libvxl_list_vcpu() and we are returning NULL, let's avoid

extra 'v'         ^ here.

> touching the output parameter *nr_vcpus_out (which should contain the
> number of vcpus in the list). Ideally, the caller initialized it to 0,
> which is therefore consistent with us returning NULL (or, as an alternative,
> we can explicitly set it to 0 if we're returning null... But just not
> touching it seems the best behavior).
> 
> In fact, the current behavior is especially problematic if, for
> instance, a domain is destroyed after we have done some steps of the
> for() loop. In which case, calls like xc_vcpu_getinfo() or
> xc_vcpu_getaffinity() will start to fail, and we return back to the
> caller inconsistent information, such as a NULL list of vcpus, but a
> modified and not 0 any longer, number of vcpus in the list.
> 
> Signed-off-by: Dario Faggioli <dfaggioli@xxxxxxxx>
> Tested-by: James Fehlig <jfehlig@xxxxxxxx>
> ---
> diff --git a/tools/libs/light/libxl_domain.c b/tools/libs/light/libxl_domain.c
> index 544a9bf59d..aabc264e16 100644
> --- a/tools/libs/light/libxl_domain.c
> +++ b/tools/libs/light/libxl_domain.c
> @@ -1705,6 +1706,7 @@ libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, 
> uint32_t domid,
>          ptr->vcpu_time = vcpuinfo.cpu_time;
>      }
>      GC_FREE;
> +    *nr_vcpus_out = nr_vcpus;

Can you swap those two lines, to keep GC_FREE just before return?

>      return ret;
>  
>  err:
> diff --git a/tools/libs/light/libxl_numa.c b/tools/libs/light/libxl_numa.c
> index 3679028c79..b04e3917a0 100644
> --- a/tools/libs/light/libxl_numa.c
> +++ b/tools/libs/light/libxl_numa.c
> @@ -219,8 +219,10 @@ static int nr_vcpus_on_nodes(libxl__gc *gc, 
> libxl_cputopology *tinfo,
>              goto next;
>  
>          vinfo = libxl_list_vcpu(CTX, dinfo[i].domid, &nr_dom_vcpus, 
> &nr_cpus);
> -        if (vinfo == NULL)
> +        if (vinfo == NULL) {
> +            assert(nr_dom_vcpus == 0);

I don't think this assert is necessary.

>              goto next;
> +        }

Otherwise, this patch looks fine.

Thanks,

-- 
Anthony PERARD