[PATCH] new functions libxl_bitmap_{or,and}

[PATCH] new functions libxl_bitmap_{or,and}

[Linda Jacobson]

provide logical and and or of two bitmaps

---

v.1  updated comments and format
v.2  rewrote bitmap functions to manipulate bytes not bits

Signed-off-by: Linda Jacobson <lindaj@jma3.com>
---
 tools/libxl/libxl_utils.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_utils.h |  5 ++++
 2 files changed, 79 insertions(+)

diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 9053b27..5c7178f 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -691,6 +691,80 @@ void libxl_bitmap_reset(libxl_bitmap *bitmap, int bit)
     bitmap->map[bit / 8] &= ~(1 << (bit & 7));
 }
 
+/* provide logical or and logical and of two bitmaps */
+int libxl_bitmap_or(libxl_ctx *ctx, libxl_bitmap *or_map,
+                    libxl_bitmap *map1, libxl_bitmap *map2)
+{
+    GC_INIT(ctx);
+    int rc;
+    uint32_t i;
+    libxl_bitmap *lgmap;
+    libxl_bitmap *smap;
+
+    if (map1->size > map2->size) {
+        lgmap = map1;
+        smap = map2;
+    }
+    else {
+        lgmap = map2;
+        smap = map1;
+    }
+
+    rc = libxl_bitmap_alloc(ctx, or_map, lgmap->size * 8);
+    if (rc)
+        goto out;
+
+    /*
+     *  if bitmaps aren't the same size, their union (logical or) will
+     *  be size of larger bit map.  Any bit past the end of the
+     *  smaller bit map, will match the larger one.
+     */
+    for (i = 0; i < smap->size; i++)
+        or_map->map[i] = (smap->map[i] | lgmap->map[i]);
+
+    for (i = smap->size; i < lgmap->size; i++)
+        or_map->map[i] = lgmap->map[i];
+
+out:
+    GC_FREE;
+    return rc;
+}
+
+int libxl_bitmap_and(libxl_ctx *ctx, libxl_bitmap *and_map,
+                     libxl_bitmap *map1, libxl_bitmap *map2)
+{
+    GC_INIT(ctx);    
+    int rc;
+    uint32_t i;
+    libxl_bitmap *lgmap, *smap;
+
+    if (map1->size >  map2->size) {
+        lgmap = map1;
+        smap = map2;
+    }
+    else {
+        lgmap = map2;
+        smap = map1;
+    }
+
+
+    rc = libxl_bitmap_alloc(ctx, and_map, smap->size * 8);
+    if (rc)
+        goto out;
+
+    /* 
+     *  if bitmaps aren't same size, their 'and' will be size of
+     *  smaller bit map
+     */
+    for (i = 0; i < and_map->size; i++)
+        and_map->map[i] = (lgmap->map[i] & smap->map[i]);
+
+out: 
+    GC_FREE;
+    return rc;
+
+}
+
 int libxl_bitmap_count_set(const libxl_bitmap *bitmap)
 {
     int i, nr_set_bits = 0;
diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
index acacdd9..1c0086b 100644
--- a/tools/libxl/libxl_utils.h
+++ b/tools/libxl/libxl_utils.h
@@ -90,6 +90,11 @@ int libxl_bitmap_test(const libxl_bitmap *bitmap, int bit);
 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);
+/* or and and functions for two bitmaps */
+int libxl_bitmap_or(libxl_ctx *ctx, libxl_bitmap *or_map,
+                    libxl_bitmap *map1, libxl_bitmap *map2); 
+int libxl_bitmap_and(libxl_ctx *ctx, libxl_bitmap *and_map,
+                     libxl_bitmap *map1, libxl_bitmap *map2);
 char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const libxl_bitmap *cpumap);
 static inline void libxl_bitmap_set_any(libxl_bitmap *bitmap)
 {
-- 
1.9.1

Re: [PATCH] new functions libxl_bitmap_{or,and}

[Wei Liu]

Thanks, we're getting there. If my comments confuse you please just ask
for clarification.

There is no need to change the subject line.  However, it would be
useful if you have some kind of version number in you subject line. That
is

   [PATCH vX] libxl: provide libxl_bimap_{and,or}

You can do this by supplying --subject-prefix= to git format-patch

   git format-patch -1 --subject-prefix="[PATCH vX]" ...

where X refers to your version number.

On Mon, Apr 13, 2015 at 01:47:18AM -0600, Linda Jacobson wrote:
> provide logical and and or of two bitmaps
> 

And the SoB line should be here.

> ---
> 
> v.1  updated comments and format
> v.2  rewrote bitmap functions to manipulate bytes not bits
> 
> Signed-off-by: Linda Jacobson <lindaj@jma3.com>
> ---
>  tools/libxl/libxl_utils.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_utils.h |  5 ++++
>  2 files changed, 79 insertions(+)
> 
> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index 9053b27..5c7178f 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -691,6 +691,80 @@ void libxl_bitmap_reset(libxl_bitmap *bitmap, int bit)
>      bitmap->map[bit / 8] &= ~(1 << (bit & 7));
>  }
>  
> +/* provide logical or and logical and of two bitmaps */

Normally we take the line before as the comment for the function that
follows it. So you don't need to mention "and" function here.

Actually I don't think you need a comment for such obvious function at
all.

> +int libxl_bitmap_or(libxl_ctx *ctx, libxl_bitmap *or_map,
> +                    libxl_bitmap *map1, libxl_bitmap *map2)

Actually I think you need to constify map1 and map2. I.e.

                       const libxl_bitmap *map1,
		       const libxl_bitmap *map2)
		       

Sorry for not having mentioned these earlier. Since you're going to
resend due to other issues so you might as well just take care of these
nits.

> +{
> +    GC_INIT(ctx);
> +    int rc;
> +    uint32_t i;
> +    libxl_bitmap *lgmap;
> +    libxl_bitmap *smap;
> +

Constify these as well.

And probably use better name like large_map and small_map?

> +    if (map1->size > map2->size) {
> +        lgmap = map1;
> +        smap = map2;
> +    }
> +    else {

Coding style.  Should be

       } else {

> +        lgmap = map2;
> +        smap = map1;
> +    }
> +
> +    rc = libxl_bitmap_alloc(ctx, or_map, lgmap->size * 8);
> +    if (rc)
> +        goto out;
> +
> +    /*
> +     *  if bitmaps aren't the same size, their union (logical or) will

           If

> +     *  be size of larger bit map.  Any bit past the end of the
> +     *  smaller bit map, will match the larger one.
> +     */
> +    for (i = 0; i < smap->size; i++)
> +        or_map->map[i] = (smap->map[i] | lgmap->map[i]);
> +
> +    for (i = smap->size; i < lgmap->size; i++)
> +        or_map->map[i] = lgmap->map[i];
> +
> +out:
> +    GC_FREE;
> +    return rc;
> +}
> +
> +int libxl_bitmap_and(libxl_ctx *ctx, libxl_bitmap *and_map,
> +                     libxl_bitmap *map1, libxl_bitmap *map2)

Constify map1 and map2.

> +{
> +    GC_INIT(ctx);    
> +    int rc;
> +    uint32_t i;
> +    libxl_bitmap *lgmap, *smap;

Constify lgmap and smap.

> +
> +    if (map1->size >  map2->size) {
> +        lgmap = map1;
> +        smap = map2;
> +    }
> +    else {

Coding style.

> +        lgmap = map2;
> +        smap = map1;
> +    }
> +
> +
> +    rc = libxl_bitmap_alloc(ctx, and_map, smap->size * 8);
> +    if (rc)
> +        goto out;
> +
> +    /* 
> +     *  if bitmaps aren't same size, their 'and' will be size of

           If

> +     *  smaller bit map
> +     */
> +    for (i = 0; i < and_map->size; i++)
> +        and_map->map[i] = (lgmap->map[i] & smap->map[i]);
> +
> +out: 
> +    GC_FREE;
> +    return rc;
> +
> +}
> +
>  int libxl_bitmap_count_set(const libxl_bitmap *bitmap)
>  {
>      int i, nr_set_bits = 0;
> diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
> index acacdd9..1c0086b 100644
> --- a/tools/libxl/libxl_utils.h
> +++ b/tools/libxl/libxl_utils.h
> @@ -90,6 +90,11 @@ int libxl_bitmap_test(const libxl_bitmap *bitmap, int bit);
>  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);
> +/* or and and functions for two bitmaps */

      Or

> +int libxl_bitmap_or(libxl_ctx *ctx, libxl_bitmap *or_map,
> +                    libxl_bitmap *map1, libxl_bitmap *map2); 
> +int libxl_bitmap_and(libxl_ctx *ctx, libxl_bitmap *and_map,
> +                     libxl_bitmap *map1, libxl_bitmap *map2);

Constify map1 and map2.


Wei.

>  char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const libxl_bitmap *cpumap);
>  static inline void libxl_bitmap_set_any(libxl_bitmap *bitmap)
>  {
> -- 
> 1.9.1

Re: [PATCH] new functions libxl_bitmap_{or,and}

[Linda]

On 4/14/2015 3:19 AM, Wei Liu wrote:
> Thanks, we're getting there. If my comments confuse you please just ask
> for clarification.
>
> There is no need to change the subject line.  However, it would be
> useful if you have some kind of version number in you subject line. That
> is
>
>     [PATCH vX] libxl: provide libxl_bimap_{and,or}
>
> You can do this by supplying --subject-prefix= to git format-patch
>
>     git format-patch -1 --subject-prefix="[PATCH vX]" ...
What version do you want me to use at this point?  I've sort of lost 
count, since many changes have been style changes.
I am assuming you usually reserve new versions for substantive changes?
>
> where X refers to your version number.
>
> On Mon, Apr 13, 2015 at 01:47:18AM -0600, Linda Jacobson wrote:
>> provide logical and and or of two bitmaps
>>
> And the SoB line should be here.
What does SoB stand for in this context?
>
>> ---
>>
>> v.1  updated comments and format
>> v.2  rewrote bitmap functions to manipulate bytes not bits
>>
>> Signed-off-by: Linda Jacobson <lindaj@jma3.com>
>> ---
>>   tools/libxl/libxl_utils.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++
>>   tools/libxl/libxl_utils.h |  5 ++++
>>   2 files changed, 79 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
>> index 9053b27..5c7178f 100644
>> --- a/tools/libxl/libxl_utils.c
>> +++ b/tools/libxl/libxl_utils.c
>> @@ -691,6 +691,80 @@ void libxl_bitmap_reset(libxl_bitmap *bitmap, int bit)
>>       bitmap->map[bit / 8] &= ~(1 << (bit & 7));
>>   }
>>   
>> +/* provide logical or and logical and of two bitmaps */
> Normally we take the line before as the comment for the function that
> follows it. So you don't need to mention "and" function here.
>
> Actually I don't think you need a comment for such obvious function at
> all.
I'll take it out.
>
>> +int libxl_bitmap_or(libxl_ctx *ctx, libxl_bitmap *or_map,
>> +                    libxl_bitmap *map1, libxl_bitmap *map2)
> Actually I think you need to constify map1 and map2. I.e.
>
>                         const libxl_bitmap *map1,
> 		       const libxl_bitmap *map2)
How come?  Out of curiosity.
> 		
>
> Sorry for not having mentioned these earlier. Since you're going to
> resend due to other issues so you might as well just take care of these
> nits.
>
>> +{
>> +    GC_INIT(ctx);
>> +    int rc;
>> +    uint32_t i;
>> +    libxl_bitmap *lgmap;
>> +    libxl_bitmap *smap;
>> +
> Constify these as well.
>
> And probably use better name like large_map and small_map?
OK.  Happy to.  Earlier someone complained about my names being too 
verbose.

Linda
>
>> +    if (map1->size > map2->size) {
>> +        lgmap = map1;
>> +        smap = map2;
>> +    }
>> +    else {
> Coding style.  Should be
>
>         } else {
>
>> +        lgmap = map2;
>> +        smap = map1;
>> +    }
>> +
>> +    rc = libxl_bitmap_alloc(ctx, or_map, lgmap->size * 8);
>> +    if (rc)
>> +        goto out;
>> +
>> +    /*
>> +     *  if bitmaps aren't the same size, their union (logical or) will
>             If
>
>> +     *  be size of larger bit map.  Any bit past the end of the
>> +     *  smaller bit map, will match the larger one.
>> +     */
>> +    for (i = 0; i < smap->size; i++)
>> +        or_map->map[i] = (smap->map[i] | lgmap->map[i]);
>> +
>> +    for (i = smap->size; i < lgmap->size; i++)
>> +        or_map->map[i] = lgmap->map[i];
>> +
>> +out:
>> +    GC_FREE;
>> +    return rc;
>> +}
>> +
>> +int libxl_bitmap_and(libxl_ctx *ctx, libxl_bitmap *and_map,
>> +                     libxl_bitmap *map1, libxl_bitmap *map2)
> Constify map1 and map2.
>
>> +{
>> +    GC_INIT(ctx);
>> +    int rc;
>> +    uint32_t i;
>> +    libxl_bitmap *lgmap, *smap;
> Constify lgmap and smap.
>
>> +
>> +    if (map1->size >  map2->size) {
>> +        lgmap = map1;
>> +        smap = map2;
>> +    }
>> +    else {
> Coding style.
>
>> +        lgmap = map2;
>> +        smap = map1;
>> +    }
>> +
>> +
>> +    rc = libxl_bitmap_alloc(ctx, and_map, smap->size * 8);
>> +    if (rc)
>> +        goto out;
>> +
>> +    /*
>> +     *  if bitmaps aren't same size, their 'and' will be size of
>             If
>
>> +     *  smaller bit map
>> +     */
>> +    for (i = 0; i < and_map->size; i++)
>> +        and_map->map[i] = (lgmap->map[i] & smap->map[i]);
>> +
>> +out:
>> +    GC_FREE;
>> +    return rc;
>> +
>> +}
>> +
>>   int libxl_bitmap_count_set(const libxl_bitmap *bitmap)
>>   {
>>       int i, nr_set_bits = 0;
>> diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
>> index acacdd9..1c0086b 100644
>> --- a/tools/libxl/libxl_utils.h
>> +++ b/tools/libxl/libxl_utils.h
>> @@ -90,6 +90,11 @@ int libxl_bitmap_test(const libxl_bitmap *bitmap, int bit);
>>   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);
>> +/* or and and functions for two bitmaps */
>        Or
>
>> +int libxl_bitmap_or(libxl_ctx *ctx, libxl_bitmap *or_map,
>> +                    libxl_bitmap *map1, libxl_bitmap *map2);
>> +int libxl_bitmap_and(libxl_ctx *ctx, libxl_bitmap *and_map,
>> +                     libxl_bitmap *map1, libxl_bitmap *map2);
> Constify map1 and map2.
>
>
> Wei.
>
>>   char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const libxl_bitmap *cpumap);
>>   static inline void libxl_bitmap_set_any(libxl_bitmap *bitmap)
>>   {
>> -- 
>> 1.9.1

Re: [PATCH] new functions libxl_bitmap_{or,and}

[Julien Grall]

Hi Linda,

Hi Linda,

On 14/04/15 13:14, Linda wrote:
> On 4/14/2015 3:19 AM, Wei Liu wrote:
>> Thanks, we're getting there. If my comments confuse you please just ask
>> for clarification.
>>
>> There is no need to change the subject line.  However, it would be
>> useful if you have some kind of version number in you subject line. That
>> is
>>
>>     [PATCH vX] libxl: provide libxl_bimap_{and,or}
>>
>> You can do this by supplying --subject-prefix= to git format-patch
>>
>>     git format-patch -1 --subject-prefix="[PATCH vX]" ...
> What version do you want me to use at this point?  I've sort of lost
> count, since many changes have been style changes.

IIRC, this is the v3, so the next will be v4.

> I am assuming you usually reserve new versions for substantive changes?

The version number should be incremented every time you send a new
version of the patch to the mailing list.

>>
>> where X refers to your version number.
>>
>> On Mon, Apr 13, 2015 at 01:47:18AM -0600, Linda Jacobson wrote:
>>> provide logical and and or of two bitmaps
>>>
>> And the SoB line should be here.
> What does SoB stand for in this context?

Signed-off-by.

As said on a previous mail, everything after "---" will be dropped when
the committer will apply your patch to the tree. So your SoB will
disappear too.

You have to move it before the "---"

>>
>>> ---
>>>
>>> v.1  updated comments and format
>>> v.2  rewrote bitmap functions to manipulate bytes not bits
>>>
>>> Signed-off-by: Linda Jacobson <lindaj@jma3.com>

[..]

>>
>>> +int libxl_bitmap_or(libxl_ctx *ctx, libxl_bitmap *or_map,
>>> +                    libxl_bitmap *map1, libxl_bitmap *map2)
>> Actually I think you need to constify map1 and map2. I.e.
>>
>>                         const libxl_bitmap *map1,
>>                const libxl_bitmap *map2)
> How come?  Out of curiosity.

You know that the 2 bitmaps won't be modified within function.
Constifying them will allow the compiler to catch any attempt to modify
the content of the bitmap.

Regards,

-- 
Julien Grall