[systemd-devel] [PATCHv2] core: do not spawn jobs or touch other units during coldplugging

[Lennart Poettering]

On Wed, 25.02.15 21:40, Ivan Shapovalov (intelfx100 at gmail.com) wrote:

Ivan,

> Because the order of coldplugging is not defined, we can reference a
> not-yet-coldplugged unit and read its state while it has not yet been
> set to a meaningful value.
> 
> This way, already active units may get started again.

> We fix this by deferring such actions until all units have been at least
> somehow coldplugged.
> 
> Fixes https://bugs.freedesktop.org/show_bug.cgi?id=88401

Hmm, so firstly, in this case, do those two alsa services 
have RemainAfterExit=yes set? I mean, if they have not, they really
should. I they have, then queuing jobs for them a second time is not
really an issue, because the services are already running they will be
eaten up eventually.

But regarding the patch:

I am sorry, but we really should find a different way to fix this, if
there really is an issue to fix...

I really don't like the hashmap that maps units to functions. I mean,
the whole concept of unit vtables exists to encapsulate the
unit-type-specific functions, and we should not add different place
for configuring those.

Also, and more importantly, the whole coldplug function exists only to
seperate the unit loading and initial state changing into two separate
steps, so that we know that all units are fully loaded before we start
making state chnages.

Now, I see that this falls short of the issue at hand here, but I
think the right fix is really to alter the order in which we
coldplug. More specifically, I think we need to make the coldplugging
order dependency aware:

before we coldplug a unit, we should coldplug all units it might
trigger, which are those with a listed UNIT_TRIGGERS dependency, as
well as all those that retroactively_start_dependencies() and
retroactively_stop_dependencies() operates on. Of course, we should
also avoid running in loops here, but that should be easy by keeping a
per-unit coldplug boolean around.

Anyway, I reverted the patch for now, this really needs more thinking.

Lennart

-- 
Lennart Poettering, Red Hat