Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro

-----Original Message----- From: Osier Yang [mailto:jyang@redhat.com] Sent: Monday, September 09, 2013 10:46 AM To: Liuji (Jeremy) Cc: libvir-list(a)redhat.com; Jinbo (Justin); Luohao (brian); Haofeng Subject: Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro On 06/09/13 18:30, Liuji (Jeremy) wrote: > The parameter of virBitmapFree function is just a pointer, not a pointer of pointer. > The second VIR_FREE on virBitmapFree only assign NULL to the formal parameter. > After calling the virBitmapFree function, the actual parameter are still not NULL. > There are many code segment don't assign NULL to the formal parameter after calling > the virBitmapFree function. This will bring potential risks. > > A problem scenario: > 1) The XML of VM contain the below segment: > <numatune> > <memory mode='preferred' placement='auto' nodeset='0'/> > </numatune> > 2)virsh create the VM > 3)In the virDomainDefParseXML funtion: > /* Ignore 'nodeset' if 'placement' is 'auto' finally */ > if (placement_mode == VIR_NUMA_TUNE_MEM_PLACEMENT_MODE_AUTO) { > virBitmapFree(def->numatune.memory.nodemask); > def->numatune.memory.nodemask = NULL; > } > 4)Then, virsh destroy the VM. In the virDomainDefFree funtion, it also call the > virBitmapFree function to free the nodemask: > virBitmapFree(def->numatune.memory.nodemask); > But after this call, the value of def->numatune.memory.nodemask is still not NULL. > This will generate an exception. > > > >From d2b69b130bca89df85005d395c6d45d8df0b74f1 Mon Sep 17 00:00:00 2001 > From: "Liuji (Jeremy)" <jeremy.liu(a)huawei.com&gt; > Date: Fri, 6 Sep 2013 04:49:30 -0400 > Subject: [PATCH] virBitmapFree: Change the function to a macro > > The parameter of virBitmapFree function is just a pointer, not a pointer of pointer. > The second VIR_FREE on virBitmapFree only assign NULL to the formal parameter. > After calling the virBitmapFree function, the actual parameter are still not NULL. > There are many code segment don't assign NULL to the formal parameter after calling > the virBitmapFree function. This will bring potential risks. Regardless of what the problem is...... > > Signed-off-by: Liuji (Jeremy) <jeremy.liu(a)huawei.com&gt; > --- > src/util/virbitmap.c | 21 --------------------- > src/util/virbitmap.h | 21 ++++++++++++++++----- > 2 files changed, 16 insertions(+), 26 deletions(-) > > diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c > index 7e1cd02..cde502f 100644 > --- a/src/util/virbitmap.c > +++ b/src/util/virbitmap.c > @@ -40,13 +40,6 @@ > > #define VIR_FROM_THIS VIR_FROM_NONE > > -struct _virBitmap { > - size_t max_bit; > - size_t map_len; > - unsigned long *map; > -}; > - > - > #define VIR_BITMAP_BITS_PER_UNIT ((int) sizeof(unsigned long) * CHAR_BIT) > #define VIR_BITMAP_UNIT_OFFSET(b) ((b) / VIR_BITMAP_BITS_PER_UNIT) > #define VIR_BITMAP_BIT_OFFSET(b) ((b) % VIR_BITMAP_BITS_PER_UNIT) > @@ -88,20 +81,6 @@ virBitmapPtr virBitmapNew(size_t size) > return bitmap; > } > > -/** > - * virBitmapFree: > - * @bitmap: previously allocated bitmap > - * > - * Free @bitmap previously allocated by virBitmapNew. > - */ > -void virBitmapFree(virBitmapPtr bitmap) > -{ > - if (bitmap) { > - VIR_FREE(bitmap->map); > - VIR_FREE(bitmap); > - } > -} > - > > int virBitmapCopy(virBitmapPtr dst, virBitmapPtr src) > { > diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h > index b682523..9b93b88 100644 > --- a/src/util/virbitmap.h > +++ b/src/util/virbitmap.h > @@ -28,6 +28,22 @@ > > # include <sys/types.h> > > +struct _virBitmap { > + size_t max_bit; > + size_t map_len; > + unsigned long *map; > +}; > + > +/* > + * Free previously allocated bitmap > + */ > +#define virBitmapFree(bitmap) \ > + do { \ > + if (bitmap) { \ > + VIR_FREE((bitmap)->map); \ > + VIR_FREE(bitmap); \ > + } \ > + } while (0); > ... What does this make difference? Unless I missed something, what you do is just changing the function into a macro. And I don't see any benifit from it. Osier