libct/cg/sd/v1: fix freezeBeforeSet #3148
Conversation
kolyshkin
force-pushed
the
fix-freeze-before-set
branch
from
5837118 to
9e2a01a
Compare
|
Ah, pushed the old version. Updated now. |
kolyshkin
force-pushed
the
fix-freeze-before-set
branch
from
9e2a01a to
b7df794
Compare
kolyshkin
force-pushed
the
fix-freeze-before-set
branch
from
b7df794 to
f9a6457
Compare
| return | ||
| unitType := getUnitType(unitName) | ||
| devPolicy, e := getUnitTypeProperty(m.dbus, unitName, unitType, "DevicePolicy") | ||
| if e == nil && devPolicy.Value.String() == `"auto"` { |
There was a problem hiding this comment.
interesting ... comparing with "auto" instead of auto
There was a problem hiding this comment.
This is because (dbus.Variant).String uses strconv.Quote to format strings.
I am not quite sure whether the old or the new way to compare is better... Old one is working fine (for devPolicy, that is), so maybe leave it as is.
Add a test for freezeBeforeSet, checking various scenarios including those that were failing before the fix in the previous commit. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
force-pushed
the
fix-freeze-before-set
branch
from
f9a6457 to
a133fbb
Compare
| t.Error(err) | ||
| } | ||
| } | ||
| // Destroy() do not kill processes in cgroup, so we should. |
mrunalp
left a comment
mrunalp
left a comment
There was a problem hiding this comment.
Should we move the check of the current freezer state before we do the SkipDevices check since we do that check always anyways and sometimes we could exit early avoiding the calls in SkipDevices?
|
Left a couple of comments, otherwise looks fine. |
Nope, we don't do the freezer state check in case SkipDevices is set and the device rules are "allow all".
I was thinking about that while working on #3082 but decided against it, since a frozen cgroup is a rare case. Wait a minute, I got a better idea! |
PTAL #3151 (faster/simpler check, even more test cases) |
3 participants
this is an alternative to #3143; the biggest difference is the test case.
Commit f2db879 (PR #3082) did not do the right job getting unit
properties, and the deviceAllow property comparison was not working
either. Fix both issues.
The test case added by #3082 was not adequate either -- it only checked
that the freeze is performed if necessary, but did not check that the freeze
is avoided if possible.
Add a test case specific to freezeBeforeSet.
Fixes: f2db879
Co-authored-by: Odin Ugedal odin@uged.al
Signed-off-by: Kir Kolyshkin kolyshkin@gmail.com