[Qemu-devel] [PATCH 0/2] bitmap: some fixes

[Qemu-devel] [PATCH 0/2] bitmap: some fixes

[Wei Wang]

"nbits = 0" is not applicable to BITMAP_LAST_WORD_MASK, callers should
avoid that. Fix bitmap_count_one to avoid passing 0 to the macro.

Wei Wang (2):
  bitmap.h: add comments to BITMAP_LAST_WORD_MASK
  bitmap: fix bitmap_count_one

 include/qemu/bitmap.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] bitmap.h: add comments to BITMAP_LAST_WORD_MASK

[Wei Wang]

This macro was ported from Linux and we've reached an aggreement there
that the corner case "nbits = 0" is not applicable to this macro, because
when "nbits = 0", which means no bits to mask, this macro is expected to
return 0, instead of 0xffffffff. This patch simply adds a comment above
the macro as a note to users about the corner case.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Peter Xu <peterx@redhat.com>
---
 include/qemu/bitmap.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
index 509eedd..f53c640 100644
--- a/include/qemu/bitmap.h
+++ b/include/qemu/bitmap.h
@@ -60,6 +60,7 @@
  */
 
 #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
+/* "nbits = 0" is not applicable to this macro. Callers should avoid that. */
 #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))
 
 #define DECLARE_BITMAP(name,bits)                  \
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] bitmap: fix bitmap_count_one

[Wei Wang]

Since "nbits = 0" is not applicable to the BITMAP_LAST_WORD_MASK macro,
callers need to avoid passing "nbits = 0" to this macro, which generates
incorrect results.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
CC: Juan Quintela <quintela@redhat.com>
CC: Peter Xu <peterx@redhat.com>
---
 include/qemu/bitmap.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
index f53c640..e45e9c0 100644
--- a/include/qemu/bitmap.h
+++ b/include/qemu/bitmap.h
@@ -223,7 +223,7 @@ static inline int bitmap_intersects(const unsigned long *src1,
 static inline long bitmap_count_one(const unsigned long *bitmap, long nbits)
 {
     if (small_nbits(nbits)) {
-        return ctpopl(*bitmap & BITMAP_LAST_WORD_MASK(nbits));
+        return nbits ? ctpopl(*bitmap & BITMAP_LAST_WORD_MASK(nbits)) : 0;
     } else {
         return slow_bitmap_count_one(bitmap, nbits);
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] bitmap.h: add comments to BITMAP_LAST_WORD_MASK

[Wei Wang]

On 07/30/2018 03:13 PM, Wei Wang wrote:
> This macro was ported from Linux and we've reached an aggreement there
> that the corner case "nbits = 0" is not applicable to this macro, because
> when "nbits = 0", which means no bits to mask, this macro is expected to
> return 0, instead of 0xffffffff. This patch simply adds a comment above
> the macro as a note to users about the corner case.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Peter Xu <peterx@redhat.com>
> ---
>   include/qemu/bitmap.h | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
> index 509eedd..f53c640 100644
> --- a/include/qemu/bitmap.h
> +++ b/include/qemu/bitmap.h
> @@ -60,6 +60,7 @@
>    */
>   
>   #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
> +/* "nbits = 0" is not applicable to this macro. Callers should avoid that. */
>   #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))
>   
>   #define DECLARE_BITMAP(name,bits)                  \

A better fix would be to directly change the macro to:  nbits ? (~0UL >> 
(-(nbits) & (BITS_PER_LONG - 1))) : 0,
so that we don't need to fix other callers like bitmap_full, 
bitmap_intersects.

So just post this out for a discussion whether it's preferred to just 
adding note comments as we did for linux or fixing the macro directly.

Best,
Wei

Re: [Qemu-devel] [PATCH 1/2] bitmap.h: add comments to BITMAP_LAST_WORD_MASK

[Juan Quintela]

Wei Wang <wei.w.wang@intel.com> wrote:
> On 07/30/2018 03:13 PM, Wei Wang wrote:
>> This macro was ported from Linux and we've reached an aggreement there
>> that the corner case "nbits = 0" is not applicable to this macro, because
>> when "nbits = 0", which means no bits to mask, this macro is expected to
>> return 0, instead of 0xffffffff. This patch simply adds a comment above
>> the macro as a note to users about the corner case.
>>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> CC: Juan Quintela <quintela@redhat.com>
>> CC: Peter Xu <peterx@redhat.com>
>> ---
>>   include/qemu/bitmap.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
>> index 509eedd..f53c640 100644
>> --- a/include/qemu/bitmap.h
>> +++ b/include/qemu/bitmap.h
>> @@ -60,6 +60,7 @@
>>    */
>>     #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) &
>> (BITS_PER_LONG - 1)))
>> +/* "nbits = 0" is not applicable to this macro. Callers should avoid that. */
>>   #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))
>>     #define DECLARE_BITMAP(name,bits)                  \
>
> A better fix would be to directly change the macro to:  nbits ? (~0UL
>>> (-(nbits) & (BITS_PER_LONG - 1))) : 0,
> so that we don't need to fix other callers like bitmap_full,
> bitmap_intersects.
>
> So just post this out for a discussion whether it's preferred to just
> adding note comments as we did for linux or fixing the macro directly.
>
> Best,
> Wei

On one hand:
a - we have copied it form linux, we don't want to diverge
On the other hand:
b - it is much easier to use if we change the macro

So, it is a tought one.

I slightly preffer b), but will not object to a either.  As you are the
one doing the patch, your choice.

Later, Juan.

Re: [Qemu-devel] [PATCH 1/2] bitmap.h: add comments to BITMAP_LAST_WORD_MASK

[Wei Wang]

On 07/30/2018 09:19 PM, Juan Quintela wrote:
> Wei Wang <wei.w.wang@intel.com> wrote:
>> On 07/30/2018 03:13 PM, Wei Wang wrote:
>>> This macro was ported from Linux and we've reached an aggreement there
>>> that the corner case "nbits = 0" is not applicable to this macro, because
>>> when "nbits = 0", which means no bits to mask, this macro is expected to
>>> return 0, instead of 0xffffffff. This patch simply adds a comment above
>>> the macro as a note to users about the corner case.
>>>
>>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>>> CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> CC: Juan Quintela <quintela@redhat.com>
>>> CC: Peter Xu <peterx@redhat.com>
>>> ---
>>>    include/qemu/bitmap.h | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/include/qemu/bitmap.h b/include/qemu/bitmap.h
>>> index 509eedd..f53c640 100644
>>> --- a/include/qemu/bitmap.h
>>> +++ b/include/qemu/bitmap.h
>>> @@ -60,6 +60,7 @@
>>>     */
>>>      #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) &
>>> (BITS_PER_LONG - 1)))
>>> +/* "nbits = 0" is not applicable to this macro. Callers should avoid that. */
>>>    #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))
>>>      #define DECLARE_BITMAP(name,bits)                  \
>> A better fix would be to directly change the macro to:  nbits ? (~0UL
>>>> (-(nbits) & (BITS_PER_LONG - 1))) : 0,
>> so that we don't need to fix other callers like bitmap_full,
>> bitmap_intersects.
>>
>> So just post this out for a discussion whether it's preferred to just
>> adding note comments as we did for linux or fixing the macro directly.
>>
>> Best,
>> Wei
> On one hand:
> a - we have copied it form linux, we don't want to diverge
> On the other hand:
> b - it is much easier to use if we change the macro
>
> So, it is a tought one.
>
> I slightly preffer b), but will not object to a either.  As you are the
> one doing the patch, your choice.

Thanks Juan. I plan to choose b - fixing the macro directly.

Best,
Wei