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

[Wei Liu <wei.liu2@citrix.com>]

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]" ...

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.

> ---
> 
> 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.

> +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)
		       

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?

> +    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