* Re: [PATCH] libxl: provide libxl_bitmap_{and,or}
2015-04-11 20:31 ` Julien Grall
@ 2015-04-11 21:27 ` Linda
2015-04-12 16:13 ` Linda
2015-04-12 19:24 ` Linda
2 siblings, 0 replies; 5+ messages in thread
From: Linda @ 2015-04-11 21:27 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Lars Kurth, Ian Jackson, Wei Liu, Ian Campbell,
Stefano Stabellini
PS - it's not much work to change to using bytes instead of bits. I
agree it would be better. I'll change it if the maintainers want.
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,
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] libxl: provide libxl_bitmap_{and,or}
2015-04-11 20:31 ` Julien Grall
2015-04-11 21:27 ` Linda
@ 2015-04-12 16:13 ` Linda
2015-04-12 19:24 ` Linda
2 siblings, 0 replies; 5+ messages in thread
From: Linda @ 2015-04-12 16:13 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Lars Kurth, Ian Jackson, Wei Liu, Ian Campbell,
Stefano Stabellini
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,
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] libxl: provide libxl_bitmap_{and,or}
2015-04-11 20:31 ` Julien Grall
2015-04-11 21:27 ` Linda
2015-04-12 16:13 ` Linda
@ 2015-04-12 19:24 ` Linda
2 siblings, 0 replies; 5+ messages in thread
From: Linda @ 2015-04-12 19:24 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Lars Kurth, Ian Jackson, Wei Liu, Ian Campbell,
Stefano Stabellini
Hi all,
Just for grins, I created and unit tested a little code snippet for
'and' and 'or', using bytes instead of bits. It didn't take that long.
If you want me to replace the code I submitted with this new code, I'd
be happy to. Otherwise, I'll just make the style changes Julian
requested, (unless there is a disagreement on where the GC_INIT should
be), and merge the changes.
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,
>
^ permalink raw reply [flat|nested] 5+ messages in thread