[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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@xxxxxxxx>
---
  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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.