Re: [PATCH] tools/libxl new bitmap functions

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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, 

You have an extra space at the end..
> +libxl_bitmap *bitmap1, libxl_bitmap *bitmap2)

And this should really start at the same line length as 'libxl_ctx'

Ditto for the rest of the functions.
> +{
> +    int size;
> +    int rc;
> +
> +    GC_INIT(ctx);
> +
> +// if bitmaps aren't the same size, union should be size of larger bit map

The comments are in:

 /* Comment here. */


> +    size = (bitmap1->size > bitmap2->size) ? bitmap1->size : bitmap2->size;

There might be an 'max' macro somwhere in the code that you could use
for this?

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

Hehe.

No need to add the braces, you can just do:

	if (rc)
		goto out;

> +        goto out;
> +    }
> +
> +    for (int bit = 0; bit < (size * 8); bit++)

Please move the 'int bit' to the start of the function.

> +    {
> +        // libxl_bitmap_test returns 0 if past end of bitmap
> +        // if the bit is set in either bitmap, set it in their union

I would change that comment to be:

/* NB. libxl_bitmap_test returns 0 if past the end of bitmap. */

> +        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);
> +        }

You can ditch the braces.

> +    }
> +
> +out:
> +    GC_FREE;
> +    return rc;
> +}
> +
> +int libxl_bitmap_intersection (libxl_ctx *ctx, libxl_bitmap 
There is space :                ^ - which should not be there.

> +*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

I believe the comments I've above apply to the code below as well.

> +    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
> +        if (libxl_bitmap_test (bitmap1, bit) &&
> +		 libxl_bitmap_test (bitmap2, bit) )

You have an extra space at the end there. Don't think you need that
in libxl code (thought you do for libxc). Yeah, two different
styleguides!

> +        {
> +            libxl_bitmap_set (intersection_bitmap, bit);
> +        }
> +    }
> +
> +out:
> +    GC_FREE;
> +    return rc;
> +}
> +int libxl_bitmap_difference(libxl_ctx *ctx, libxl_bitmap *difference_bitmap, 
> +libxl_bitmap *bitmap1, libxl_bitmap *bitmap2)
> +{
> +    int size;
> +    int rc;
> +
> +    GC_INIT(ctx);
> +
> +// if bitmaps aren't the same size, difference should be size of larger 
> +    size = (bitmap1->size > bitmap2->size) ? bitmap1->size : bitmap2->size;
> +
> +    libxl_bitmap_init(difference_bitmap);
> +    rc = libxl_bitmap_alloc(ctx, difference_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 one bitmap and not the other, set it in 
> +their difference
> +        NOTE:  if one bit map is larger, this will result in all bits 
> +being set past the size of the smaller bitmap;  if this is not
> +        the desired behavior, please let me know

That would make this a bit strange. Perhaps demand that they
must be the same size? If they are not then just return an error?
> +        */
> +
> +        if (libxl_bitmap_test (bitmap1, bit) 
You have an extra space at the end there.      ^
> +		&& (!libxl_bitmap_test (bitmap2, bit)) )
> +        {
> +            libxl_bitmap_set (difference_bitmap, bit);
> +        }
> +    }
> +
> +out:
> +    GC_FREE;
> +    return rc;
> +}
> +
> +
> +
>  
>  /* NB. caller is responsible for freeing the memory */
>  char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const libxl_bitmap *bitmap)
> diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
> index acacdd9..521e4bb 100644
> --- a/tools/libxl/libxl_utils.h
> +++ b/tools/libxl/libxl_utils.h
> @@ -91,6 +91,16 @@ void libxl_bitmap_set(libxl_bitmap *bitmap, int bit);
>  void libxl_bitmap_reset(libxl_bitmap *bitmap, int bit);
>  int libxl_bitmap_count_set(const libxl_bitmap *cpumap);
>  char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const libxl_bitmap *cpumap);
> +/*
> +   union, intersection and difference functions for
> +	two bitmaps
> +*/
> +int libxl_bitmap_union(libxl_ctx *ctx, libxl_bitmap *new_bitmap, libxl_bitmap *bitmap1, libxl_bitmap *bitmap2);
> +
> +int libxl_bitmap_intersection(libxl_ctx *ctx, libxl_bitmap *new_bitmap, libxl_bitmap *bitmap1, libxl_bitmap *bitmap2);
> +
> +int libxl_bitmap_difference(libxl_ctx *ctx, libxl_bitmap *new_bitmap, libxl_bitmap *bitmap1, libxl_bitmap *bitmap2);
> +
>  static inline void libxl_bitmap_set_any(libxl_bitmap *bitmap)
>  {
>      memset(bitmap->map, -1, bitmap->size);
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel