Trying to disable a unit with no install info is mostly useless, so
adding a warning like we do for enable (with the new dbus method
'DisableUnitFilesWithFlagsAndInstallInfo()'). Note that it would
still find and remove symlinks to the unit in /etc, regardless of
whether it has install info or not, just like before. And if there are
actually files to remove, we suppress the warning.
Fixes#17689
The name "def.h" originates from before the rule of "no needless abbreviations"
was established. Let's rename the file to clarify that it contains a collection
of various semi-related constants.
We treat symlinks to unit files outside of the search path differently from
symlinks to unit files *in* the search path. The former are "linked" unit
files, while the latter are enablement symlinks and such and will be removed
when disabling the unit.
The history of the check for in_search_path() is interesting: this condition
was added already in the first version of the code in
830964834f. Since the beginning, matching
arguments would simply be ignored. I think this is dubious. The man page says:
> Link a unit file that is *not* in the unit file search paths
> into the unit file search path
But for backwards-compat, let's continue to silently do nothing for files
*in* the search path.
The case of symlinks to unit files underneath the search path, but in some
subdirectory, is less clear. We didn't check for this case, so it was
implicitly allowed. But that's just an oversight, we don't want to allow people
to create additional subhierarchies under our hierarchy. Let's check for this
case and refuse.
Closes#24605.
This adds a check for an allocation error for the calls to install_changes_add()
where we're plannig to return success from the call. In cases where we're
returning failure, it doesn't matter as much: the operation will fail anyway,
and if the allocation fails, we'll just get a less descriptive error message.
The machinery to report a good error message only works if the
error was registered with install_changes_add() and a file name. Otherwise
we only get a generic "Op failed: %m" message.
In some places -EINVAL is replaced by -EUCLEAN, so that we get the proper
error message.
The function was written to only return an error from internal allocation
failures, because when using it to create a bus message, we want to distinguish
a failed operation from an allocation error when sending the reply. But it
turns out that the only caller that makes this distinction checks that the
passed-in errno value ('type') is not negative beforehand. So we can make the
function pass 'type' value through, which makes most of the callers nicer.
No functional change.
Typically the _MAX and _INVALID special enum values use the full type as
prefix, even if the actual values of the enum might not. Let's follow
this rule here too.
We can just make this an enum, as long as we ensure it has enough range,
which we can do by adding -ERRNO_MAX as one possible value (at least on
GNU C). We already do that at multiple other places, so let's do this
here too.
'systemctl unmask foo' will try to remove the symlink to /dev/null under /etc/.
But the unit may also be masked by a symlink under /run/generator, in particular
the one created by systemd-debug-generator based on systemd.mask=foo on the
kernel commandline. The unmask call cannot anything about this: even if it removed
the symlink from /run/generator, it'll be recreated on the next daemon-reload.
Thus, we can only warn about it.
Initially, I wanted to check if 'systemctl.mask' is defined on the kernel
command-line, but that's not effective, because such mask symlinks can be
created by other generators based on other conditions. Checking for runtime
mask is "dumber", but is more robust because it doesn't assume who created the
mask and why.
The handling of InstallInfo is the copied from install_info_symlink_wants().
It's pretty ugly, this whole code should be rewritten from scratch.
The message is printed, but the whole operation is still "successful". This
keep backwards compatibility: people might call unmask to remove filesystem
masks even if there's still a cmdline param in place. We allow 'systemctl
mask' to create such a mask, so 'unmask' should be able to remove it.
Fixes#22689.
We had an anonymous enum with values called UNIT_FILE_…, which could easily be
confused with UNIT_FILE_… from UnitFileFlags enum. This commit renames the enum
values and also the variables which refer to them.
This reverts commit d6c9411072.
I still think this is something that needs to be done, but we're hitting some
unexplained failures, e.g. https://github.com/systemd/systemd/issues/22920.
So let's revert this for now, so -rc2 can be released, with a plan to return
to this after a release.
Closes#22920.
We would only accept "identical" links, but having e.g. a symlink
/usr/lib/systemd/system/foo-alias.service → /usr/lib/systemd/system/foo.service
when we're trying to create /usr/lib/systemd/system/foo-alias.service →
./foo.service is OK. This fixes an issue found in ubuntuautopkg package
installation, where we'd fail when enabling systemd-resolved.service, because
the existing alias was absolute, and (with the recent patches) we were trying
to create a relative one.
A test is added.
(For .wants/.requires symlinks we were already doing OK. A test is also
added, to verify.)
When we have a symlink that goes outside of our search path, we should just
ignore the target file name. But we were verifying it, and rejecting in
the case where a symlink was created manually.
The two states are distinguished, but are treated everywhere identically,
so there is no difference in behaviour except for slighlty different log
output.
We'd say that file is enabled indirectly if we had a symlink like:
foo@.service ← bar.target.wants/foo@one.service
but not when we had
foo@one.service ← bar.target.wants/foo@one.service
The effect of both link types is the same. In fact we don't care
about the symlink target. (We'll warn if it is mismatched, but we honour
it anyway.)
So let's use the original match logic only for aliases.
For .wants/.requires we instead look for a matching source name,
or a source name that matches after stripping of instance.
This is a fairly noticable change, but I think it needs to be done.
So far we'd create an absolute symlink to the target unit file:
.wants/foo.service → /usr/lib/systemd/system/foo.service
or
alias.service → /etc/systemd/system/aliased.service.
This works reasonably well, except in one case: where the unit file
is linked. When we look at a file link, the name of the physical file
isn't used, and we only take the account the symlink source name.
(In fact, the destination filename may not even be a well-formed unit name,
so we couldn't use it, even if we wanted to.) But this means that if
a file is linked, and specifies aliases, we'd create absolute links for
those aliases, and systemd would consider each "alias" to be a separate
unit. This isn't checked by the tests here, because we don't have a running
systemd instance, but it is easy enough to check manually.
The most reasonable way to fix this is to create relative links to the
unit file:
.wants/foo.service → ../foo.service
alias.service → aliased.service.
I opted to use no prefix for aliases, both normal and 'default.target',
and to add "../" for .wants/ and .requires/. Note that the link that is
created doesn't necessarily point to the file. E.g. if we're enabling
a file under /usr/lib/systemd/system, and create a symlink in /etc/systemd/system,
it'll still be "../foo.service", not "../../usr/lib/systemd/system/foo.service".
For our unit loading logic this doesn't matter, and figuring out a path
that actually leads somewhere would be more work. Since the user is allowed
to move the unit file, or add a new unit file in a different location, and
we don't actually follow the symlink, I think it's OK to create a dangling
symlink. The prefix of "../" is useful to give a hint that the link points
to files that are conceptually "one level up" in the directory hierarchy.
With the relative symlinks, systemd knows that those are aliases.
The tests are adjusted to use the new forms. There were a few tests that
weren't really testing something useful: 'test -e x' fails if 'x' is a
a dangling symlink. Absolute links in the chroot would be dangling, even
though the target existed in the expected path, but become non-dangling
when made relative and the test fails.
This should be described in NEWS, but I'm not adding that here, because
it'd likely result in conflicts.
We use the symlink source name and destination names to decide whether to remove
the symlink. But if the source name is enough to decide to remove the symlink,
we'd still look up the destination for no good reason. This is a slow operation,
let's skip it.
I linked a file as root, so I had a symlink /root/test.service ← /etc/systemd/system/test.service.
To my surpise, when running test-systemctl-enable, it failed with a cryptic EACCES.
The previous commit made the logs a bit better. Strace shows that we
were trying to follow the symlink without taking --root into account.
It seems that this bug was introduced in 66a19d85a5:
before it, we'd do readlink_malloc(), which returned a path relative to root. But
we only used that path for checking if the path is in remove_symlinks_to set, which
contains relative paths. So if the path was relative, we'd get a false-negative
answer, but we didn't go outside of the root. (We need to canonicalize the symlink
to get a consistent answer.) But after 66a19 we use chase_symlinks(), without taking
root into account which is completely bogus.
When chase_symlinks() fails, we'd get the generic error:
Failed to disable: Permission denied.
Let's at least add the failure to changes list, so the user gets
a slightly better message. Ideally, we'd say where exactly the permission
failure occured, but chase_symlinks() is a library level function and I don't
think we should add logging there. The output looks like this now:
Failed to resolve symlink "/tmp/systemctl-test.1r7Roj/etc/systemd/system/link5alias2.service": Permission denied
Failed to resolve symlink "/tmp/systemctl-test.1r7Roj/etc/systemd/system/link5alias.service": Permission denied
Failed to disable unit, file /tmp/systemctl-test.1r7Roj/etc/systemd/system/link5alias2.service: Permission denied.
Failed to disable unit, file /tmp/systemctl-test.1r7Roj/etc/systemd/system/link5alias.service: Permission denied.
We'd create aliases and other symlinks first, and only then try to create
the main link. Since that can fail, let's do things in opposite order, and
abort immediately if we can't link the file itself.
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".
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.
