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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 11.11.20 15:27, Jan Beulich wrote:

Hi Jan.

> > > > > >     }
> > > > > >
> > > > > >     #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?
Yes, it was primarily a question to Paul, but I wanted to hear your 
opinion as well. Sorry for the confusion.
--
Regards,

Oleksandr Tyshchenko