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 <[email protected]> > Tested-by: James Fehlig <[email protected]> > --- > 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
