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

[Liuji (Jeremy)]

> -----Original Message-----
> From: Osier Yang [mailto:jyang at redhat.com]
> Sent: Monday, September 09, 2013 6:15 PM
> To: Liuji (Jeremy)
> Cc: libvir-list at redhat.com; Jinbo (Justin); Luohao (brian); Haofeng
> Subject: Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro
> 
> [......]
> 
> >> Regardless of what the problem is......
> >>
> >>> Signed-off-by: Liuji (Jeremy) <jeremy.liu at huawei.com>
> >>> ---
> >>>    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
>