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

[Linda <lindaj@jma3.com>]

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