On 5/14/25 02:49, Jan Beulich wrote:
> On 13.05.2025 21:54, Stewart Hildebrand wrote:
>> --- a/xen/common/rangeset.c
>> +++ b/xen/common/rangeset.c
>> @@ -433,6 +433,20 @@ bool rangeset_is_empty(
>> return ((r == NULL) || list_empty(&r->range_list));
>> }
>>
>> +int rangeset_count_ranges(const struct rangeset *r)
>> +{
>> + int nr = 0;
>
> Ehem - this and the function's return type want to be unsigned.
>
>> + struct list_head *list;
>> +
>> + if ( r == NULL )
>> + return 0;
>> +
>> + list_for_each( list, &r->range_list )
>
> Nit: Either you deem list_for_each a pseudo-keyword (then a blank is
> missing) or you don't (then there are excess blanks).
>
> Further I don't think this is valid to do without holding the rangeset's
> lock in read mode (irrespective of the function return value potentially
> being stale by the time the caller gets to look at it, which is no
> different from other functions, i.e. falls in the caller's
> responsibilities).
>
>> + nr++;
>
> And then, if already abstraction is wanted, wouldn't this loop better be
> yet another helper (macro?) in xen/list.h?
>
>> + return nr;
>> +}
>
> Finally: If this is to be commonly used in several places, having such a
> helper is likely fine. As it stands, the sole caller is an __init
> function, and hence this is unreachable code post-init (which while not
> formally a Misra violation in my eyes effectively still is one). Aiui
> the same can be achieved using rangeset_report_ranges(), with a new
> (__init and static) callback function.
Moving rangeset_count_ranges() to rangeset.h might change the situation
slightly, but I'm not aware of any other potential callers so I'll just
use rangeset_report_ranges().
