[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH] libxl: provide libxl_bitmap_{and,or}
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@xxxxxxxx>
---
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,
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
