We had a check that was done in unit_file_resolve_symlink(). Let's move
the check to unit_validate_alias_symlink_or_warn(), which makes it available
to the code in install.c.
With this, unit_file_resolve_symlink() behaves almost the same. The warning
about "suspicious symlink" is done a bit later. I think this should be OK.
Some calls to lookup_path_init() were not followed by any log emission.
E.g.:
$ SYSTEMD_LOG_LEVEL=debug systemctl --root=/missing enable unit; echo $?
1
Let's add a helper function and use it in various places.
$ SYSTEMD_LOG_LEVEL=debug build/systemctl --root=/missing enable unit; echo $?
Failed to initialize unit search paths for root directory /missing: No such file or directory
1
$ SYSTEMCTL_SKIP_SYSV=1 build/systemctl --root=/missing enable unit; echo $?
Failed to initialize unit search paths for root directory /missing: No such file or directory
Failed to enable: No such file or directory.
1
The repeated error in the second case is not very nice, but this is a niche
case and I don't think it's worth the trouble to trying to avoid it.
So far we'd issue a warning (before this series, just in the logs on the server
side, and before this commit, on stderr on the caller's side), but return
success. It seems that successfull return was introduced by mistake in
aa0f357fd8 (my fault :( ), which was supposed to
be a refactoring without a functional change. I think it's better to fail,
because if enablement fails, the user will most likely want to diagnose the
issue.
Note that we still do partial enablement, as far as that is possible. So if
e.g. we have [Install] Alias=foo.service foobar, we'll create the symlink
'foo.service', but not 'foobar', since that's not a valid unit name. We'll
print info about the action taken, and about 'foobar' being invalid, and return
failure.
If an invalid arg appears in [Install] Alias=, WantedBy=, RequiredBy=,
we'd warn in the logs, but not propagate this information to the caller,
and in particular not over dbus. But if we call "systemctl enable" on a
unit, and the config if invalid, this information is quite important.
We would resolve those specifiers to the calling user/group. This is mostly OK
when done in the manager, because the manager generally operates as root
in system mode, and a non-root in user mode. It would still be wrong if
called with --test though. But in systemctl, this would be generally wrong,
since we can call 'systemctl --system' as a normal user, either for testing
or even for actual operation with '--root=…'.
When operating in --global mode, %u/%U/%g/%G should return an error.
The information whether we're operating in system mode, user mode, or global
mode is passed as the data pointer to specifier_group_name(), specifier_user_name(),
specifier_group_id(), specifier_user_id(). We can't use userdata, because
it's already used for other things.
This makes it easier to pass it around in preparation for future changes.
While at it, let's rename InstallContext c → ctx, and InstallInfo i → info.
'c' and 'i' are bad names for variables that are passed through multiple layers
of functions calls. It's easier to follow what is happening with a meaningful
variable names.
ENOENT is easily confused with the file that we're working on not being
present, e.g. when the file contains %o or something else that requires
os-release to be present. Let's use -EUNATCH instead to reduce that chances of
confusion if the context of the error is lost.
And once we have pinpointed the reason, let's provide a proper error message:
+ build/systemctl --root=/tmp/systemctl-test.TO7Mcb enable some-some-link6@.socket
/tmp/systemctl-test.TO7Mcb/etc/systemd/system/some-some-link6@.socket: Failed to resolve alias "target@A:%A.socket": Protocol driver not attached
Failed to enable unit, cannot resolve specifiers in "target@A:%A.socket".
In systemd.unit we document that unset fields resolve to "". But we didn't
directly test this, so let's do that. Also, we return -ENOENT if the file
is missing, which we didn't document or test.
We didn't actually say that keys should not be repeated. At least the
examples in docs (both python and shell) would do that, and any simple
parser that builds a dictionary would most likely behave the same way.
But let's document this expectation, but also say how to deal with malformed
files.
The test for the variable is added in test-systemctl-enable because there we
can do it almost for free, and the variable is most likely to be used with
'systemctl enable --root' anyway.
This test has overlap with test-install-root, but it tests things at a
different level, so I think it's useful to add. It immediately shows various
bugs which will be fixed in later patches.
When we are printing a valid unit name, quoting isn't necessary, because
unit names cannot contain whitespace or other confusing characters. In particular
if the unit name is prefixed by " unit " or something else that clearly
identifies the string as a unit name, quoting would just add unnecessary
noise. But when we're printing paths or invalid names, it's better to add
quotes for clarity.
We save a few lines, but the important thing is that we don't have two
different implementations with slightly different rules used for enablement
and loading. Fixes#22000.
Tested with:
- the report in #22000, it now says:
$ SYSTEMD_LOG_LEVEL=debug systemctl --root=/ enable test.service
Suspicious symlink /etc/systemd/system/test.service→/etc/systemd/system/myown.d/test.service, treating as alias.
unit_file_resolve_symlink: self-alias: /etc/systemd/system/test.service → test.service, ignoring.
running_in_chroot(): Permission denied
Suspicious symlink /etc/systemd/system/test.service→/etc/systemd/system/myown.d/test.service, treating as alias.
unit_file_resolve_symlink: self-alias: /etc/systemd/system/test.service → test.service, ignoring.
Failed to enable unit, refusing to operate on linked unit file test.service
- a symlink to /dev/null:
...
unit_file_resolve_symlink: linked unit file: /etc/systemd/system/test3.service → /dev/null
Failed to enable unit, unit /etc/systemd/system/test3.service is masked.
- the same from the host:
...
unit_file_resolve_symlink: linked unit file: /var/lib/machines/rawhide/etc/systemd/system/test3.service → /var/lib/machines/rawhide/dev/null
Failed to enable unit, unit /var/lib/machines/rawhide/etc/systemd/system/test3.service is masked.
- through the manager:
$ sudo systemctl enable test.service
Failed to enable unit: Refusing to operate on alias name or linked unit file: test.service
$ sudo systemctl enable test3.service
Failed to enable unit: Unit file /etc/systemd/system/test3.service is masked.
As seen in the first example, the warning is repeated. This is because we call
the lookup logic twice: first for sysv-compat, and then again for real. I think
that since this is only for broken setups, and when sysv-compat is enabled, and
in an infrequent manual operation, at debug level, this is OK.
The old logs used __func__, but this doesn't make sense now, because the
low-level function will be used in other places. So those are adjusted to be
more generic.
Same idea as 03677889f0.
No functional change intended. The type of the iterator is generally changed to
be 'const char*' instead of 'char*'. Despite the type commonly used, modifying
the string was not allowed.
I adjusted the naming of some short variables for clarity and reduced the scope
of some variable declarations in code that was being touched anyway.
When we do mkdir, we should just use 0o777 and let the umask take care of the
rest. Specifying an explicit mode is inappropriate. And when touching the code,
let's replace black madness with normal python style.
