Re: [PATCH] libxl: provide libxl_bitmap_{and,or}

[Linda <lindaj@jma3.com>]

Hi Julien and all,
     Julien, re: your comment on moving GC_INIT to the top.
     In Wei's template for the functions, he wrote:
     int rc;
     GC_INIT...
/*    your own stuff */
     GC_FREE;
     return rc;

I can move the other declarations below GC_INIT, but should I leave the 
"int rc;" before it?

Thanks.

Linda


On 4/11/2015 2:31 PM, Julien Grall wrote:
> Hi Linda,
>
> Thank you for sending the new version.
>
> It's common to CC the maintainers of the patch. Most of us have filter 
> in order to get directly mail they are involved too and avoid watching 
> every time the ML. Otherwise answer to your mail may take longer.
>
> You can find them with the scripts scripts/get_maintainers.pl. I've 
> cced them.
>
> On 10/04/2015 04:34, Linda Jacobson wrote:
>> Added functions to create the logical 'and' and logical 'or'
>> of two input bitmaps.  Cleaned up spacing and comments.
>>
>> Removed accidentally committed libxl_u_disk* files.
>
> Thank you for saying what you changed in this version. While it's 
> useful during review process, this is not necessary to keep them in 
> the final commit message.
>
> When the committers will commit your patch, git will ignore everything 
> after "---" and a newline.
>
> For instance you could write
>
> ---
>    Changes in v3:
>     - Foo ...
>
>    Changes in v2:
>     - Added functions...
>         - Removed ...
>
>
>> Signed-off-by: Linda Jacobson <lindaj@jma3.com>
>> ---
>>   tools/libxl/libxl_utils.c | 55 
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>   tools/libxl/libxl_utils.h |  6 ++++++
>>   2 files changed, 61 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
>> index 9053b27..fb8830a 100644
>> --- a/tools/libxl/libxl_utils.c
>> +++ b/tools/libxl/libxl_utils.c
>> @@ -691,6 +691,61 @@ void libxl_bitmap_reset(libxl_bitmap *bitmap, 
>> int bit)
>>       bitmap->map[bit / 8] &= ~(1 << (bit & 7));
>>   }
>>
>> +int libxl_bitmap_or(libxl_ctx *ctx, libxl_bitmap *or_map,
>> +                    libxl_bitmap *map1, libxl_bitmap *map2)
>> +{
>> +    uint32_t size;
>> +    int rc;
>> +    int bit;
>> +
>> +    GC_INIT(ctx);
>
> GC_INIT is a macro which declaring a variable. It seems common to 
> declare at the very beginning of the function. Can you move it there?
>
>> +    /* if bitmaps aren't same size, the bitmap of their logical
>> +       or should be the size of the larger bit map
>> +    */
>
> The coding style for multi-lines comment is:
>
> /*
>  * My comment
>  * continue
>  */
>
>> +    size = max(map1->size, map2->size);
>> +    rc = libxl_bitmap_alloc(ctx, or_map, size);
>> +    if (rc)
>> +        goto out;
>> +
>> +    for (bit = 0; bit < (size * 8); bit++) {
>> +        if (libxl_bitmap_test(map1, bit))
>> +            libxl_bitmap_set(or_map, bit);
>> +        else if (libxl_bitmap_test(map2, bit))
>> +            libxl_bitmap_set(or_map, bit);
>> +    }
>> +
>> +out:
>> +    GC_FREE;
>> +    return rc;
>> +}
>> +
>> +int libxl_bitmap_and(libxl_ctx *ctx, libxl_bitmap *and_map,
>> +                     libxl_bitmap *map1, libxl_bitmap *map2)
>> +{
>> +    uint32_t size;
>> +    int rc;
>> +    int bit;
>> +
>> +    GC_INIT(ctx);
>
> Same remark.
>
>> +    /* if bitmaps aren't same size, logical and should
>> +       be the size of the smaller bit map
>> +    */
>
> Coding style
>
>> +    size = min(map1->size, map2->size);
>> +    rc = libxl_bitmap_alloc(ctx, and_map, size);
>> +    if (rc)
>> +        goto out;
>> +
>> +    for (bit = 0; bit < (size * 8); bit++) {
>> +        if (libxl_bitmap_test (map1, bit) &&
>> +            libxl_bitmap_test (map2, bit) )
>> +            libxl_bitmap_set (and_map, bit);
>> +    }
>> +
>> +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 68b5580..0b6480d 100644
>> --- a/tools/libxl/libxl_utils.h
>> +++ b/tools/libxl/libxl_utils.h
>> @@ -91,6 +91,12 @@ 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 *bitmap);
>>   char *libxl_bitmap_to_hex_string(libxl_ctx *ctx, const libxl_bitmap 
>> *bitmap);
>> +/* or, and and xor functions for two bitmaps
>> + */
>
> I saw you sent a separate patch for fixing the coding style. I don't 
> think it's necessary. Can you merge the 2 patches?
>
> Although, the coding style for a one line comment is
> /* My comment */
>
>> +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);
>>   static inline void libxl_bitmap_set_any(libxl_bitmap *bitmap)
>>   {
>>       memset(bitmap->map, -1, bitmap->size);
>>
>
> Other than few coding style issue, I think the 2 functions (or & and) 
> can be optimize (i.e working with byte rather than bit). Although, I'm 
> not a maintainers so I will let them decide if it's worth to do it.
>
> Regards,
>