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

[Osier Yang]

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 at huawei.com>
> 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 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