[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


  • To: "anthony.perard@xxxxxxxxxx" <anthony.perard@xxxxxxxxxx>
  • From: Dario Faggioli <dfaggioli@xxxxxxxx>
  • Date: Wed, 19 Jan 2022 09:02:46 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=q1nCisR+MpxMjCvUUdyHb0D1eVkTXqFbRGIji655TQI=; b=E3eHApyMsuWKb7e9L1qUygszpG15ipe0zPi5yMNrJfLYtnEuC92OOwVgeN6da6nqADxxQvIaW8yataZzS9eFsR6KVfLG7PPKqZo7A41+Y/pHwCncwKN8Id+Ha5X54w4iv0+gyxzNwbegk1djDzjA2wHrce9XShBWyhKAVwTpaES9lTiFzQ6IwKFhrlxsXAjVqRsW8FKzIlNeT3yhFyflkPn/8ewhvCfagMvRfAYW9Cx1zz//vieHp4Znku4RUqpqtlFt3/UXHvQRdqC3IJZPemhDj45Tnpc+sPjLiD60bTI1hzwGBGGD3dgHg87a+Jn/DqvyW5oMCtic1LbydpKCnA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=nOw6WYiUV2E2sgU6ibKwx4ZEJA1OxuxO8JfPSSlCQEYZxch3WU0QGdoUtqO5yeZiyldl3cLat9NDr7QCFfzvGgbh05N0gitoEVpjYS+WgRR0UE6yE9lSqcM0BF9KJlrQ0yThYeB08p573hG47Sxf3mz2moGbW7zCl9ArUwh/8AK06j6IcaVvPI5qJ6uPgC9M3AGDYly6Qsi4ZcXW7gzzSOW/TtRPyuaolE7slSIyXRiStyZddwPh1FlX5L/TyuZw1g35wvvclGyGPvZiOHh7j5A0s07p5pelh1i0KfDv8Niz9eUtwNMY14d3TYKQxAde9QjTkG62eHgBPCiZr3JypQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Juergen Gross <JGross@xxxxxxxx>, Jim Fehlig <jfehlig@xxxxxxxx>, "wl@xxxxxxx" <wl@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 19 Jan 2022 09:03:04 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHYCHXrUuOcqEMe8ki4TaQGMIKek6xjKiKAgAQ6pgCAArDxgA==
  • Thread-topic: [PATCH 1/2] tools/libs/light: numa placement: don't try to free a NULL list of vcpus

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.