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

[Julien Grall <julien@xxxxxxx>]

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);

--
Julien Grall