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

[Wei Yang <richardw.yang@linux.intel.com>]

On Wed, Jul 17, 2019 at 03:43:11PM +0800, Peter Xu wrote:
>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.
>

oops, you are right.

>> +
>> +    /* 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.
>

good suggestion

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

ok, let me take a look into this.

>Thanks,
>
>-- 
>Peter Xu

-- 
Wei Yang
Help you, Help me