Re: [XEN PATCH] xen/types: address Rule 10.1 for macro BITS_TO_LONGS

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.09.2023 10:20, Nicola Vetrini wrote:
> On 05/09/2023 09:46, Jan Beulich wrote:
>> On 05.09.2023 09:31, Nicola Vetrini wrote:
>>> Given its use in the declaration
>>> 'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument
>>> 'bits' has essential type 'enum iommu_feature', which is not
>>> allowed by the Rule as an operand to the addition operator.
>>> Given that its value can be represented by a signed integer,
>>> the explicit cast resolves the violation.
>>
>> Wait - why would this lead to a change to BITS_TO_LONGS()? And if that
>> was to be changed, why plain int? I don't think negative input makes
>> sense there, and in principle I'd expect values beyond 4 billion to
>> also be permissible (even if likely no such use will ever appear in a
>> DECLARE_BITMAP(), but elsewhere it may make sense). Even going to
>> "unsigned long" may be too limiting ...
>>
> 
> You have a point. I can think of doing it like this:
> DECLARE_BITMAP(features, (int)IOMMU_FEAT_count)
> on the grounds that the enum constant is representable in an int, and it 
> does not seem likely
> to get much bigger.
> Having an unsigned cast requires making the whole expression
> essentially unsigned, otherwise Rule 10.4 is violated because 
> BITS_PER_LONG is
> essentially signed. This can be done, but it depends on how 
> BITS_TO_LONGS will be/is used.

It'll need looking closely, yes, but I expect that actually wants to be an
unsigned constant. I wouldn't be surprised if some use of DECLARE_BITMAP()
appeared (or already existed) where the 2nd argument involves sizeof() in
some way.

>>> --- a/xen/include/xen/types.h
>>> +++ b/xen/include/xen/types.h
>>> @@ -23,7 +23,7 @@ typedef signed long ssize_t;
>>>  typedef __PTRDIFF_TYPE__ ptrdiff_t;
>>>
>>>  #define BITS_TO_LONGS(bits) \
>>> -    (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
>>> +    (((int)(bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
>>>  #define DECLARE_BITMAP(name,bits) \
>>>      unsigned long name[BITS_TO_LONGS(bits)]
>>>
>>
>> Furthermore, as always - if this was to be touched, please take care
>> of style violations (numerous missing blanks) at this occasion.
> 
> Then the whole file needs a cleanup.

Perhaps, but going as we touch things is generally deemed better than doing
a single cleanup-only patch. First and foremost to not unduly affect the
usefulness of "git blame" and alike.

Jan