Hi,
On 22/12/2020 07:50, Jan Beulich wrote:
On 21.12.2020 19:45, Andrew Cooper wrote:
On 21/12/2020 18:36, Julien Grall wrote:
@@ -553,6 +606,9 @@ struct domain *domain_create(domid_t domid,
if ( init_status & INIT_watchdog )
watchdog_domain_destroy(d);
+ /* Must not hit a continuation in this context. */
+ ASSERT(domain_teardown(d) == 0);
The ASSERT() will become a NOP in production build, so
domain_teardown_down() will not be called.
Urgh - its not really a nop, but it's evaluation isn't symmetric between
debug and release builds. I'll need an extra local variable.
Or use ASSERT_UNREACHABLE(). (I admit I don't really like the
resulting constructs, and would like to propose an alternative,
even if I fear it'll be controversial.)
However, I think it would be better if we pass an extra argument to
indicated wheter the code is allowed to preempt. This would make the
preemption check more obvious in evtchn_destroy() compare to the
current d->is_dying != DOMDYING_dead.
We can have a predicate if you'd prefer, but plumbing an extra parameter
is wasteful, and can only cause confusion if it is out of sync with
d->is_dying.
I agree here - it wasn't so long ago that event_channel.c gained
a DOMDYING_dead check, and I don't see why we shouldn't extend
this approach to here and elsewhere.
I think the d->is_dying != DOMYING_dead is difficult to understand even
with the comment on top. This was ok in one place, but now it will
spread everywhere. So at least, I would suggest to introduce a wrapper
that is better named.
There is also a futureproof concern. At the moment, we are considering
the preemption will not be needed in domain_create(). I am ready to bet
that the assumption is going to be broken sooner or later.
As we don't have an extra parameter, we would need to investigate all
the callers in helpers to see where we need to drop the d->is_dying !=
DOMYING_dead. I am sure you will agree that there is a risk to screw up.
With an extra parameter, it is easier to re-enable preemption when needed.
Anyway, both of you seem to agree on the approach here. If that's what
you want, then so it be, but that's not something I will feel confident
to Ack. Although, I will not Nack it.
Cheers,
--
Julien Grall
