core: propagate start limit hit from triggered unit to path unit #17031
Conversation
keszybz
left a comment
There was a problem hiding this comment.
Looks like a good fix, but I think the check in the first patch looks like something that grew organically over time and should be refactored now.
src/core/automount.c
Outdated
| UNIT_NOT_FOUND, | ||
| UNIT_BAD_SETTING, | ||
| UNIT_ERROR, | ||
| UNIT_MASKED)|| other->type != UNIT_MOUNT) |
There was a problem hiding this comment.
Hmm, the states that are left are UNIT_STUB and UNIT_MERGED. I think it'd be cleaner at this point to invert the condition (and maybe even emit a warning if we hit that path. Wouldn't that mean some fuckup in the unit handling logic if a stub unit or a merged unit emitted a notification?).
Also, space before || is missing.
In 4c2ef32 we enabled propagating triggered unit state to the triggering unit for service units in more load states, so that we don't accidentally stop tracking state correctly. Do the same for our other triggering unit states: automounts, paths, and timers. Also, make this an assertion rather than a simple test. After all it should never happen that we get called for half-loaded units or units of the wrong type. The load routines should already have made this impossible.
We already do this for socket and automount units, do it for path units too: if the triggered service keeps hitting the start limit, then fail the triggering unit too, so that we don#t busy loop forever. (Note that this leaves only timer units out in the cold for this kind of protection, but it shouldn't matter there, as they are naturally protected against busy loops: they are scheduled by time anyway). Fixes: systemd#16669
This is implied in C and we generally don't bother with this, so don't bother with this here either.
de85fbf to
22f401b
Compare
|
Force pushed a new version, which upgrades these tests to assert() and turns them around. PTAL. I think they should be asserts since it shouldn't be possible we get into the state where state changes are triggered on UNIT_STUB or UNIT_MERGED unit load states. |
|
Looks reasonable. I guess that if we ever get the logic wrong, at least it'll be easy to notice with all the asserts. |
|
bionic-i386 timed out, and semaphoreci says something about lxc. Looks unrelated. |
Fixes: #16669