Re: [PATCH V2 17/23] xen/ioreq: Introduce domain_has_ioreq_server()

[Oleksandr <olekstysh@xxxxxxxxx>]

On 11.11.20 18:28, Paul Durrant wrote:

Hi Paul.

> > > > > > >         d->ioreq_server.server[id] = s;
> > > > > > > +
> > > > > > > +    if ( s )
> > > > > > > +        d->ioreq_server.nr_servers++;
> > > > > > > +    else
> > > > > > > +        d->ioreq_server.nr_servers--;
> > > > > > >     }
> > > > > > >
> > > > > > >     #define GET_IOREQ_SERVER(d, id) \
> > > > > > > diff --git a/xen/include/xen/ioreq.h b/xen/include/xen/ioreq.h
> > > > > > > index 7b03ab5..0679fef 100644
> > > > > > > --- a/xen/include/xen/ioreq.h
> > > > > > > +++ b/xen/include/xen/ioreq.h
> > > > > > > @@ -55,6 +55,20 @@ struct ioreq_server {
> > > > > > >         uint8_t                bufioreq_handling;
> > > > > > >     };
> > > > > > >
> > > > > > > +#ifdef CONFIG_IOREQ_SERVER
> > > > > > > +static inline bool domain_has_ioreq_server(const struct domain *d)
> > > > > > > +{
> > > > > > > +    ASSERT((current->domain == d) || atomic_read(&d->pause_count));
> > > > > > > +
> > > > > > This seems like an odd place to put such an assertion.
> > > > > I might miss something or interpreted incorrectly but these asserts are
> > > > > the result of how I understood the review comment on previous version [1].
> > > > >
> > > > > I will copy a comment here for the convenience:
> > > > > "This is safe only when d == current->domain and it's not paused,
> > > > > or when they're distinct and d is paused. Otherwise the result is
> > > > > stale before the caller can inspect it. This wants documenting by
> > > > > at least a comment, but perhaps better by suitable ASSERT()s."
> > > > The way his reply was worded, I think Paul was wondering about the
> > > > place where you put the assertion, not what you actually assert.
> > > Shall I put the assertion at the call sites of this helper instead?
> > Since Paul raised the question, I expect this is a question to him
> > rather than me?
> If it is indeed a question for me then yes, put the assertion where it is clear 
> why it is needed. domain_has_ioreq_server() is essentially a trivial accessor 
> function; it's not the appropriate place.
Got it. Thank you for the clarification.

--
Regards,

Oleksandr Tyshchenko