Re: [Qemu-devel] [PATCH v2 2/2] test-bitmap: add test for bitmap_set

[Peter Xu <zhexu@redhat.com>]

On Wed, Jul 17, 2019 at 03:11:14PM +0800, Wei Yang wrote:
> Add a test for bitmap_set. There are three cases:
> 
>   * Both start and end is BITS_PER_LONG aligned
>   * Only start is BITS_PER_LONG aligned
>   * Only end is BITS_PER_LONG aligned
> 
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>

Hi, Wei,

Thanks for doing this.  I've got a few comments below.

> ---
>  tests/test-bitmap.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/tests/test-bitmap.c b/tests/test-bitmap.c
> index cb7c5e462d..1f0123f604 100644
> --- a/tests/test-bitmap.c
> +++ b/tests/test-bitmap.c
> @@ -59,12 +59,45 @@ static void check_bitmap_copy_with_offset(void)
>      g_free(bmap3);
>  }
>  
> +static void check_bitmap_set(void)
> +{
> +    unsigned long *bmap;
> +
> +    bmap = bitmap_new(BMAP_SIZE);

Need to free this.

> +
> +    /* Both Aligned, set bits [BITS_PER_LONG, 2*BITS_PER_LONG] */
> +    bitmap_set(bmap, BITS_PER_LONG, BITS_PER_LONG);
> +    g_assert_cmpint(find_first_bit(bmap, BITS_PER_LONG), ==, BITS_PER_LONG);

Can check all 1's set correctly.

       g_assert_cmpuint(bmap[1], ==, -1ul);

Can also make this at least across multiple long fields.

> +    g_assert_cmpint(find_next_zero_bit(bmap, 2 * BITS_PER_LONG, BITS_PER_LONG),
> +                    ==, 2 * BITS_PER_LONG);
> +
> +    bitmap_clear(bmap, 0, BMAP_SIZE);
> +    /* End Aligned, set bits [BITS_PER_LONG - 5, 2*BITS_PER_LONG] */
> +    bitmap_set(bmap, BITS_PER_LONG - 5, BITS_PER_LONG + 5);

Same here.

> +    g_assert_cmpint(find_first_bit(bmap, BITS_PER_LONG),
> +                    ==, BITS_PER_LONG - 5);
> +    g_assert_cmpint(find_next_zero_bit(bmap,
> +                                       2 * BITS_PER_LONG, BITS_PER_LONG - 5),
> +                    ==, 2 * BITS_PER_LONG);
> +
> +    bitmap_clear(bmap, 0, BMAP_SIZE);
> +    /* Start Aligned, set bits [BITS_PER_LONG, 2*BITS_PER_LONG + 5] */
> +    bitmap_set(bmap, BITS_PER_LONG, BITS_PER_LONG + 5);

And here.

> +    g_assert_cmpint(find_first_bit(bmap, BITS_PER_LONG),
> +                    ==, BITS_PER_LONG);
> +    g_assert_cmpint(find_next_zero_bit(bmap,
> +                                       2 * BITS_PER_LONG + 5, BITS_PER_LONG),
> +                    ==, 2 * BITS_PER_LONG + 5);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      g_test_init(&argc, &argv, NULL);
>  
>      g_test_add_func("/bitmap/bitmap_copy_with_offset",
>                      check_bitmap_copy_with_offset);
> +    g_test_add_func("/bitmap/bitmap_set",
> +                    check_bitmap_set);

Can at least do the same test to bitmap_set_atomic too, simply by
allowing your helper test function to take a func pointer:

void (*bmap_set_func)(unsigned long *map, long i, long len);

Then call with both bitmap_set{_atomic}.

Thanks,

-- 
Peter Xu