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. Jan
