> -----Original Message----- > From: Osier Yang [mailto:jyang@xxxxxxxxxx] > Sent: Monday, September 09, 2013 6:15 PM > To: Liuji (Jeremy) > Cc: libvir-list@xxxxxxxxxx; Jinbo (Justin); Luohao (brian); Haofeng > Subject: Re: [PATCH] virBitmapFree: Change the function to a macro > > [......] > > >> Regardless of what the problem is...... > >> > >>> Signed-off-by: Liuji (Jeremy) <jeremy.liu@xxxxxxxxxx> > >>> --- > >>> 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 > > I thought I have already clearly explained the problem. > > > > The virBitmapFree is: > > void virBitmapFree(virBitmapPtr bitmap) > > { > > if (bitmap) { > > VIR_FREE(bitmap->map); > > VIR_FREE(bitmap); > > } > > } > > Bitmap is a parameter (formal parameter). The address which is pointed by > Bitmap is the same as argument(actual parameter) pointed. > > In " VIR_FREE(bitmap);", it frees the same address. But only assigns the > NULL to the Bitmap, not to the argument(actual parameter). > > After calling virBitmapFree, if there is not assigning NULL to the > argument(actual parameter), and using this argument(actual parameter) to > call > > the virBitmapFree again, it will free an already freed address. It will generate > a crash. > > I knew your meaning, what I was wondering was: > > > > > There are 3 solutions: > > 1> After call virBitmapFree function, add a assignment which is assign NULL > to the argument(actual parameter) of virBitmapFree. > > 2> Change the virBitmapFree to : > > void virBitmapFree(virBitmapPtr *bitmap) > > { > > if (*bitmap) { > > VIR_FREE((*bitmap)->map); > > VIR_FREE(*bitmap); > > } > > } > > And change all virBitmapFree calling from > > virBitmapFree(abc) > > to > > virBitmapFree(&abc) > > 3> change the virBitmapFree to a macro, like my first mail. > > > > > 1) Regardless of what the problem is, and whether to change the interface of > virBitmapFree or introduce a macro, I didn't see you use it, I.E > fixing the crash > problems. Without the changes on the use of virBitmapFree, the patch is > quite confused between the commit log and the patch itself. > > 2) And actually the macro still doesn't help if we don't pass a > "virBitmapPtr *" > argument for it. So the patch doesn't change things. In my patch, no need to modify the virBitmapFree called code, I.E NO need to change virBitmapFree(abc) to virBitmapFree(&abc) After the virBitmapFree was changed to a macro, there is no parameter(formal parameter). virBitmapFree directly operate the argument(actual parameter). > I prefer to change the interface of virBitmapFree instead, since it will > need the > "virBitmapPtr *" argument anyway. > > Osier > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list