[PATCH] tools/libxl new bitmap functions

[PATCH] tools/libxl new bitmap functions

[Linda Jacobson]

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;
+    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);
+    rc = libxl_bitmap_alloc(ctx, union_bitmap, size);
+    if (rc)
+    {
+        // Following the coding standards here.  
+	//First goto I've written in decades.
+        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
+        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
+        if (libxl_bitmap_test (bitmap1, bit) &&
+		 libxl_bitmap_test (bitmap2, bit) )
+        {
+            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
+        */
+
+        if (libxl_bitmap_test (bitmap1, bit) 
+		&& (!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

Re: [PATCH] tools/libxl new bitmap functions

[Konrad Rzeszutek Wilk]

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

Re: [PATCH] tools/libxl new bitmap functions

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 601 bytes --]

On Thu, 2015-04-02 at 15:34 -0400, Konrad Rzeszutek Wilk wrote:
> On Thu, Apr 02, 2015 at 11:38:16AM -0600, Linda Jacobson wrote:
> > --- a/tools/libxl/libxl_utils.c
> > +++ b/tools/libxl/libxl_utils.c

> > +        goto out;
> > +    }
> > +
> > +    for (int bit = 0; bit < (size * 8); bit++)
> 
> Please move the 'int bit' to the start of the function.
> 
Or, even better, use libxl_for_each_bit, defined in libxl_utils.h. :-)

(I wanted to say this in my previous mail, while commenting about the
name of the functions and about the long lines, but forgot. :-D)

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] tools/libxl new bitmap functions

[Linda]

Hey Konrad,

On 4/2/2015 1:34 PM, Konrad Rzeszutek Wilk wrote:
> 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?
Any guesses on where I might find the max macro.  When I grep max, all I 
find are definitions of constants that are the
max of something.

Thanks.

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

Re: [PATCH] tools/libxl new bitmap functions

[Julien Grall]

Hi Linda,

On 07/04/15 16:45, Linda wrote:
>>> +    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?
> Any guesses on where I might find the max macro.  When I grep max, all I
> find are definitions of constants that are the
> max of something.

There is a macro max_t in libxl_internal.h that can be used to get the
maximum of 2 numbers.

Regards,

-- 
Julien Grall

Re: [PATCH] tools/libxl new bitmap functions

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1767 bytes --]

On Thu, 2015-04-02 at 11:38 -0600, Linda Jacobson wrote:
> From: Linda <lindaj@jma3.com>
> 
> Added bitmap functions for union intersection and difference betweenn two bitmaps
> 
Just wondering, about union and intersection, are them the best names
for these operations, or do we want libxl_bitmap_or() and
libxl_bitmap_and()?  Personally, I'd go for _and() and _or(), it's more
common and more consistent with what we have already in the codebase.

About difference, what it the exact intended semantic of that operation?
In set theory, AFAICR, A-B is the set of elements that are in A and are
not in B. For a binary bitmap, I'd say it is the set of bits that are
set in A and are not set in B, is it so? Whatever it is, we should write
it down somewhere, e.g., in libxl_utils.h, as the function's doc
comment.

> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c

> +int libxl_bitmap_intersection (libxl_ctx *ctx, libxl_bitmap 
> +*intersection_bitmap, libxl_bitmap *bitmap1, libxl_bitmap *bitmap2)
>
Lots of long lines. Limit is 80 charathers (and keeping them a little
less than that is even better :-))

To make that easier to happen, in this specific case (but also because
it is more convenient in general and requires less typing), I'd use
shorter variable names.

Perhaps not as much as ba and bb for the operands and bi for the result
(that would diverge from existing functions too much for my taste), but
at least bitmap1, bitmap2 and intersect, instead of intersection_bitmap.
Or even bitmap1 and bitmap2 for the operands, and just bitmap for the
result.

Of course, that does not guarantee that the fucntion signature stays on
one line. If it still does not, you should break it.

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] tools/libxl new bitmap functions

[Linda]

Dario,
     I like your comments on naming.  We'll see what others say.
     As far as difference, at least as I've implemented it, it's the 
exclusive or.
To all,
     I've been thinking about Konrad's comment that we should disallow 
bitmaps of unequal size, in the XOR/difference function. I've 
implemented it, so the resulting bitmap is the size of the larger input 
bitmap, and the bits past the end of the smaller bitmap are all ones.  I 
think this is inelegant.
     Another possibility, is, if two bitmaps are of unequal size, make 
the XOR bitmap be the size of the smaller bitmap.  This would actually 
yield a reasonable result.
     What do others say?

Thanks.

Linda Jacobson


On 4/3/2015 3:25 AM, Dario Faggioli wrote:
> On Thu, 2015-04-02 at 11:38 -0600, Linda Jacobson wrote:
>> From: Linda <lindaj@jma3.com>
>>
>> Added bitmap functions for union intersection and difference betweenn two bitmaps
>>
> Just wondering, about union and intersection, are them the best names
> for these operations, or do we want libxl_bitmap_or() and
> libxl_bitmap_and()?  Personally, I'd go for _and() and _or(), it's more
> common and more consistent with what we have already in the codebase.
>
> About difference, what it the exact intended semantic of that operation?
> In set theory, AFAICR, A-B is the set of elements that are in A and are
> not in B. For a binary bitmap, I'd say it is the set of bits that are
> set in A and are not set in B, is it so? Whatever it is, we should write
> it down somewhere, e.g., in libxl_utils.h, as the function's doc
> comment.
>
>> --- a/tools/libxl/libxl_utils.c
>> +++ b/tools/libxl/libxl_utils.c
>> +int libxl_bitmap_intersection (libxl_ctx *ctx, libxl_bitmap
>> +*intersection_bitmap, libxl_bitmap *bitmap1, libxl_bitmap *bitmap2)
>>
> Lots of long lines. Limit is 80 charathers (and keeping them a little
> less than that is even better :-))
>
> To make that easier to happen, in this specific case (but also because
> it is more convenient in general and requires less typing), I'd use
> shorter variable names.
>
> Perhaps not as much as ba and bb for the operands and bi for the result
> (that would diverge from existing functions too much for my taste), but
> at least bitmap1, bitmap2 and intersect, instead of intersection_bitmap.
> Or even bitmap1 and bitmap2 for the operands, and just bitmap for the
> result.
>
> Of course, that does not guarantee that the fucntion signature stays on
> one line. If it still does not, you should break it.
>
> Regards,
> Dario

Re: [PATCH] tools/libxl new bitmap functions

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2483 bytes --]

On Fri, 2015-04-03 at 07:48 -0600, Linda wrote:
> Dario,
>
Hi,

One thing: here in the ML, please, don't top post.

>      I like your comments on naming.  We'll see what others say.
>      As far as difference, at least as I've implemented it, it's the 
> exclusive or.
>
Well, then I'd call it libxl_bitmap_xor().  :-)

BTW, is there an actual use case for it? Intersection and union, I'm
sure they can be useful in many places (I know it from my direct
experience!)... xor, not sure I ever needed it. I'm not complaining or
objecting, I'm just asking. :-D

> To all,
>      I've been thinking about Konrad's comment that we should disallow 
> bitmaps of unequal size, in the XOR/difference function. I've 
> implemented it, so the resulting bitmap is the size of the larger input 
> bitmap, and the bits past the end of the smaller bitmap are all ones.  I 
> think this is inelegant.
>
I don't like this either.

>      Another possibility, is, if two bitmaps are of unequal size, make 
> the XOR bitmap be the size of the smaller bitmap.  This would actually 
> yield a reasonable result.
>      What do others say?
> 
I'm really not sure. This certainly looks at least better than the
solution above, IMO. Another thing that you could do to actually
disallow this is to just return an error if you detect the two bitmaps
have different sizes, but again, I'm not sure... let's see what Wei and
other libxl maintainers (which I'm Cc-ing) think.

Regards,
Dario

> Thanks.
> 
> Linda Jacobson
> 
> 
> On 4/3/2015 3:25 AM, Dario Faggioli wrote:
> > On Thu, 2015-04-02 at 11:38 -0600, Linda Jacobson wrote:
> >> From: Linda <lindaj@jma3.com>
> >>
> >> Added bitmap functions for union intersection and difference betweenn two bitmaps
> >>
> > Just wondering, about union and intersection, are them the best names
> > for these operations, or do we want libxl_bitmap_or() and
> > libxl_bitmap_and()?  Personally, I'd go for _and() and _or(), it's more
> > common and more consistent with what we have already in the codebase.
> >
> > About difference, what it the exact intended semantic of that operation?
> > In set theory, AFAICR, A-B is the set of elements that are in A and are
> > not in B. For a binary bitmap, I'd say it is the set of bits that are
> > set in A and are not set in B, is it so? Whatever it is, we should write
> > it down somewhere, e.g., in libxl_utils.h, as the function's doc
> > comment.
> >

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] tools/libxl new bitmap functions

[Wei Liu]

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.