Re: [PATCH] tools/libxl new bitmap functions

[Wei Liu <wei.liu2@citrix.com>]

Sorry for keeping you waiting. I just get back from vacation.

First I would like to thank Dario and Konrad for their reviews. Their
comments are quite to the point and you should fix your code as
suggested.

In addition to their comments, I have a few comments. See below.

On Thu, Apr 02, 2015 at 11:38:16AM -0600, Linda Jacobson wrote:
> From: Linda <lindaj@jma3.com>
> 
> Added bitmap functions for union intersection and difference betweenn two bitmaps
> 
> Signed-off-by: Linda <lindaj@jma3.com>
> ---
>  tools/libxl/libxl_utils.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_utils.h |  10 ++++
>  2 files changed, 125 insertions(+)
> 
> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index 9053b27..c390ddc 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -699,6 +699,121 @@ int libxl_bitmap_count_set(const libxl_bitmap *bitmap)
>  
>      return nr_set_bits;
>  }
> +int libxl_bitmap_union(libxl_ctx *ctx, libxl_bitmap *union_bitmap, 
> +libxl_bitmap *bitmap1, libxl_bitmap *bitmap2)
> +{
> +    int size;

uint32_t because "size" field in bitmap type is uint32_t.

> +    int rc;
> +
> +    GC_INIT(ctx);
> +
> +// if bitmaps aren't the same size, union should be size of larger bit map
> +    size = (bitmap1->size > bitmap2->size) ? bitmap1->size : bitmap2->size;
> +
> +    libxl_bitmap_init(union_bitmap);

There is no need to call this, because the caller of this function is
supposed to do that.

> +    rc = libxl_bitmap_alloc(ctx, union_bitmap, size);
> +    if (rc)
> +    {
> +        // Following the coding standards here.  
> +	//First goto I've written in decades.

Don't put in such comments please.

And you can omit the {} if there is only one statement.

> +        goto out;
> +    }
> +
> +    for (int bit = 0; bit < (size * 8); bit++)
> +    {
> +        // libxl_bitmap_test returns 0 if past end of bitmap
> +        // if the bit is set in either bitmap, set it in their union

The above comment is not needed. Not that it is wrong, just that the
code below is self-explanatory.

> +        if (libxl_bitmap_test(bitmap1, bit))
> +        {
> +            libxl_bitmap_set(union_bitmap, bit);
> +        }
> +        else if (libxl_bitmap_test(bitmap2, bit))
> +        {
> +            libxl_bitmap_set(union_bitmap, bit);
> +        }
> +    }
> +
> +out:
> +    GC_FREE;
> +    return rc;
> +}
> +
> +int libxl_bitmap_intersection (libxl_ctx *ctx, libxl_bitmap 
> +*intersection_bitmap, libxl_bitmap *bitmap1, libxl_bitmap *bitmap2)
> +{
> +    int size;
> +    int rc;
> +
> +    GC_INIT(ctx);
> +
> +// if bitmaps aren't same size, intersection should be size of 
> +// smaller bit map
> +    size = (bitmap1->size > bitmap2->size) ? bitmap2->size : bitmap1->size;
> +
> +    libxl_bitmap_init(intersection_bitmap);
> +    rc = libxl_bitmap_alloc(ctx, intersection_bitmap, size);
> +    if (rc)
> +    {
> +        goto out;
> +    }
> +
> +    for (int bit = 0; bit < (size * 8); bit++)
> +    {
> +        // libxl_bitmap_test returns 0 if past end of bitmap
> +        // if the bit is set in both bitmaps, set it in their intersection

The above comment is not needed.

> +        if (libxl_bitmap_test (bitmap1, bit) &&
> +		 libxl_bitmap_test (bitmap2, bit) )
> +        {
> +            libxl_bitmap_set (intersection_bitmap, bit);
> +        }
> +    }
> +
> +out:
> +    GC_FREE;
> +    return rc;
> +}

Please have an extra line between functions.

> +int libxl_bitmap_difference(libxl_ctx *ctx, libxl_bitmap *difference_bitmap, 
> +libxl_bitmap *bitmap1, libxl_bitmap *bitmap2)

I think there is confusion on how this function should be implemented
and the semantics of this function.  Since we're short on time I think
it's better to just ignore this for now and polish the other two well
defined functions.

Wei.