Re: [PATCH v2 4/6] rangeset: introduce rangeset_subtract

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.05.2025 19:01, Stewart Hildebrand wrote:
> On 5/13/25 11:39, Jan Beulich wrote:
>> On 08.05.2025 15:20, Stewart Hildebrand wrote:
>>> --- a/xen/common/rangeset.c
>>> +++ b/xen/common/rangeset.c
>>> @@ -397,6 +397,18 @@ int rangeset_merge(struct rangeset *r1, struct 
>>> rangeset *r2)
>>>      return rangeset_report_ranges(r2, 0, ~0UL, merge, r1);
>>>  }
>>>  
>>> +static int cf_check subtract(unsigned long s, unsigned long e, void *data)
>>> +{
>>> +    struct rangeset *r = data;
>>> +
>>> +    return rangeset_remove_range(r, s, e);
>>> +}
>>> +
>>> +int rangeset_subtract(struct rangeset *r1, struct rangeset *r2)
>>> +{
>>> +    return rangeset_report_ranges(r2, 0, ~0UL, subtract, r1);
>>> +}
>>
>> I understand this was committed already, but I don't understand why: This
>> introduces a Misra rule 2.1 violation aiui. The rule isn't tagged as clean
>> yet, but it was accepted and hence I thought we would strive towards not
>> introducing new violations. What's the deal?
> 
> The very next patch (also committed) makes use of the function, so the
> series as a whole did not introduce a violation. Our code review
> guidelines still say to organize new independent helper functions into
> logically separate patches [0]. To be clear, and for future reference,
> would your expectation be to squash the introduction of the helper
> function into the patch where it's used?

Well, it's not so much my than Misra's expectation. With a small helper
like the one here folding certainly wouldn't have caused much of a
headache, yet I agree things can be different when the helper is quite
a bit larger; some re-arrangements may be necessary to make in such a
situation. And yes, imo ...

> Perhaps we ought to finally
> update the code review guidelines...
> 
> [0] https://xenbits.xenproject.org/governance/code-review-guide.html

... the guidelines better wouldn't be in conflict with Misra requirements.

Jan