[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/2] tools/libs/light: numa placement: don't try to free a NULL list of vcpus
On Thu, 2022-01-13 at 12:05 +0000, Anthony PERARD wrote: > On Wed, Jan 12, 2022 at 05:41:36PM +0100, Dario Faggioli wrote: > > > 2) there should be nothing to free anyway > > The issue here is that it doesn't appear to be true. Even if "info" > is > NULL, "nr" have an other value than 0, so libxl_vcpuinfo_list_free() > will try to access random addresses. > My point here was that if vinfo is NULL (because libxl_list_vcpu() returned NULL), there aren't any list element or any list to free, so we can avoid calling libxl_vcpuinfo_list_free(). Then, sure, if we call it with a NULL vinfo and a non-zero nr_dom_vcpus, things explode, but this was being addressed in patch 2. This first one was really only about not trying to free an empty list. And not to workaround the fact that it currently can make things explode, just because it feels pointless. > > Signed-off-by: Dario Faggioli <dfaggioli@xxxxxxxx> > > Tested-by: James Fehlig <jfehlig@xxxxxxxx> > > Can I suggest to make libxl_vcpuinfo_list_free() work a bit better in > case it's "nr" parameter is wrong? It will do nothing if "list" is > NULL. > I thought about that, and we certainly can do it. However, I think it's unrelated to this patch so, if I do it, I'll do it in its own one. Also, if we go that way, I guess we want to change libxl_cputopology_list_free(), libxl_pcitopology_list_free(), libxl_numainfo_list_free(), libxl_dominfo_list_free(), libxl_vminfo_list_free() and libxl_cpupoolinfo_list_free(), don't we? > Also I think it is better to keep the free in the exit path at the > end > of the loop. > Can I ask why as, as I was trying to say above, if we are sent directly to next by one of the goto, we do know that vinfo is NULL and libxl_vcpuinfo_list_free() will be basically a NOP, however it is implemented. Of course, you're the maintainer of this code, so I'll do like that if you ask... I'm just curious. :-) Actually, if you really think that the call to libxl_vcpuinfo_list_free() should stay where it is, I can just drop this patch and go on with patch 2 only, which is enough for fixing the crash. Thanks and Regards -- Dario Faggioli, Ph.D http://about.me/dario.faggioli Virtualization Software Engineer SUSE Labs, SUSE https://www.suse.com/ ------------------------------------------------------------------- <<This happens because _I_ choose it to happen!>> (Raistlin Majere) Attachment:
signature.asc
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |