On Mon, 2022-01-17 at 15:56 +0000, Anthony PERARD wrote: > On Fri, Jan 14, 2022 at 11:22:00PM +0000, Dario Faggioli wrote: > > > > 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? > > I actually don't know if that would be useful. Those functions do > work > as expected (I think) with the right parameters: they do nothing when > called with (NULL, 0). free(NULL) does nothing. > Right.
> So checking for NULL before using `nr` would probably just be a > workaround for programming mistake in the function allocating the > object > or some missing initialisation in the caller. So I don't think it is > important anymore. > Agreed. That's why, as I said, I though about doing something like that, but ended up deciding not to. > > > 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. :-) > > Freeing resources should always been attempted, even if that mean to > check whether there's something to free before calling the associated > free function. Imagine someone adding some code that could fail after > the libxl_list_vcpu(), then when that new code fails it would `goto > next;` and the allocated data from libxl_list_vcpu() would never be > freed. > Yeah, sure, whoever adds code that does a 'goto next' with an allocated list, should also realize that libxl_vcpuinfo_list_free() needs to be moved after 'next' itself, as a consequence of the very change being done, and this seems fair to me. But, at the end of the day, it's not a big deal at all. Thanks for satisfying my curiosity and, since you also agree on... > > 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. > > This patch is just a workaround a bug in libxl_list_vcpu(), so I > think > it can be dropped. > ...this, I'll just drop this patch and proceed with just what, in this series, was patch 2. 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)
signature.asc
Description: This is a digitally signed message part
