Fix memory protection default setting #16058
Conversation
Corrected reference to non-existent unit.
When MemoryLow= or MemoryMin= is set, it is interpretted as setting the values to infinity. This is inconsistent with the default initialization to 0. It'd be nice to interpret the empty assignment as fallback to DefaultMemory* of parent slice, however, current DBus API cannot convey such a NULL value, so stick to simply interpretting that as hard-wired default.
Currently, an empty assignment of Memory{Low,Min}= directives would be
interpretted as setting it to global default, i.e. zero. However, if we
set a runtime protection value on a unit that inherits parent's
DefaultMemory{Low,Min}=, it is not possible to revert it back to the
state where the DefaultMemory{Low,Min}= is propagated from parent
slice(s).
This patch changes the semantics of the empty assignments to explicitly
nullify any value set by the user previously. Since DBus API uses
uint64_t where 0 is a valid configuration, the patch modifies DBus API
by exploiting the variant type of property value to pass the NULL value.
|
Nice catch, thanks! Just went through the changes and they look good to me. |
|
(A test would be nice, if possible, but I won't block you on it.) |
|
I don't have an opinion on the bus semantics change... It should work, but it'll make the returned values incompatible with the api declared for introspection. @poettering ? |
|
looks ok I guess. I don't like the macro stuff though, we should rework that one day, and turn this into regular functions that just take enough parameters to deduce what they need. code generation with macros kinda sucks, and generates a lot of duplicate code... But that's for another PR, given that this just continues a scheme that already is in place. |
not sure what you mean precisely? Yes this breaks API a bit, but only in minor ways, i.e. what UINT64_MAX/0 means. But otherwise? |
|
oh, sorry, i misread the patch. I grok what you mean. Hmpf, overloading props like this with multiple is not OK I guess. We shouldn't do that... In previouses cases we just added a separate "hidden" property that we use to communicate the additional way to pass information. |
|
So I proposed #16122 now as a revert. Sorry for merging this so blindly. I apparently wasn't really awake. Either we merge #16122 or we merge a quick follow-up PR that removes the overloading of the prop (and instead introduces a new "hidden" prop with a related name that can be used to communicate that). See MemoryLimit= and MemoryLimitScale= for a blueprint for such a hidden prop. I marked the revert PR for the next milestone, so that we don't forget about this. But this doesn't mean it has to be merged, if @Werkov provides an alternative... |
|
The DBus API of |
Expected behavior
Actual behavior
When the value is unset (via
systemctl set-property) the behavior should return back to the default, i.e. no protection (or inherited from parent). See invididual commits for implementation details.Cc: @cdown