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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 01.02.22 19:33, Julien Grall wrote:
>
>
> On 01/02/2022 17:14, Oleksandr Andrushchenko wrote:
>> On 01.02.22 19:05, Julien Grall wrote:
>>> 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.
>
> Right, but that change of behavior may not be noticed during development. So 
> I think we want to avoid BUG_ON() when this is possible.
>
>>
>> I can turn this into WARN_ON instead, but this may lead to memory leaks
>> or some other errors not handled.
>
> IMHO, this is a bit better but not by much. Looking a rangeset_destroy(), you 
> should be able to do it without any of the issues you described here. 
> Something like:
>
>     if ( r == NULL )
>       return;
>
>     while ( (x = first_range(r)) != NULL )
>         destroy_range(r, x);
>
Yes, this is actually what Roger suggested to me privately on IRC.
Ok, so I'll re-work the patch as above

Thank you,
Oleksandr