Re: [PATCH 1/2] tools/libs/light: numa placement: don't try to free a NULL list of vcpus

[Dario Faggioli <dfaggioli@xxxxxxxx>]

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)

Attachment: signature.asc
Description: This is a digitally signed message part