Re: [PATCH 2/4] rangeset: add rangeset_reset helper function

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Julien!

On 01.02.22 19:05, Julien Grall wrote:
> Hi,
>
> On 01/02/2022 16:25, Oleksandr Andrushchenko wrote:
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>
>> This helper destroys all the ranges of the rangeset given.
>> Please note, that it uses rangeset_remove_range which returns an error
>> code on failure. This error code can be ignored as while destroying all
>> the ranges no memory allocation is expected, so in this case it must not
>> fail.
>>
>> To make sure this remains valid use BUG_ON if that changes in the future.
>>
>> Suggested-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>> ---
>>   xen/common/rangeset.c      | 6 ++++++
>>   xen/include/xen/rangeset.h | 3 +++
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
>> index ea27d651723b..9ca2b06cff22 100644
>> --- a/xen/common/rangeset.c
>> +++ b/xen/common/rangeset.c
>> @@ -525,6 +525,12 @@ void rangeset_swap(struct rangeset *a, struct rangeset 
>> *b)
>>       write_unlock(&b->lock);
>>   }
>>   +void rangeset_reset(struct rangeset *r)
>> +{
>> +    /* This doesn't allocate anything and must not fail. */
>> +    BUG_ON(rangeset_remove_range(r, 0, ~0ULL));
>
> I vaguely recall some discussion in the past (not related to this series) 
> that we wanted to avoid calling function have side-effect in a BUG_ON(). So 
> if we decide to remove at compile-time BUG_ON(), there would be no issue.
>
> But then I am not sure about the use of BUG_ON(). Can you outline why 
> crashing the hypervisor is better than continuing (e.g. use WARN_ON() or 
> ASSERT())?
Non-zero value will indicate we were not able to complete the operation
which must not fail because of the concrete use-case: we remove all the
ranges and it is not expected that this may fail.
Just to make sure this behavior does not change I use BUG_ON here which
if triggered clearly indicates that the behavior has changed and there is
a need in code change.

I can turn this into WARN_ON instead, but this may lead to memory leaks
or some other errors not handled.

>
> Cheers,
>
Thank you,
Oleksandr