The two states are distinguished, but are treated everywhere identically,
so there is no difference in behaviour except for slighlty different log
output.
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.
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 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.
The variable is not useful outside of the loop (it'll always be null
after the loop is finished), so we can declare it inline in the loop.
This saves one variable declaration and reduces the chances that somebody
tries to use the variable outside of the loop.
For consistency, 'de' is used everywhere for the var name.
Only accept DT_REG/DT_LNK/DT_DIR entries, ignore all others.
Only accpet DT_REG/DT_LNK for file names that are valid unit file names.
Only accept DT_DIR for filenames that are valid unit file names which
are suffixed by .d, .wants, .requires
This doesn't really fix any bugs, but tightens what we insert into the
lookup tables.
LLVM 13 introduced `-Wunused-but-set-variable` diagnostic flag, which
trips over some intentionally set-but-not-used variables or variables
attached to cleanup handlers with side effects (`_cleanup_umask_`,
`_cleanup_(notify_on_cleanup)`, `_cleanup_(restore_sigsetp)`, etc.):
```
../src/basic/process-util.c:1257:46: error: variable 'saved_ssp' set but not used [-Werror,-Wunused-but-set-variable]
_cleanup_(restore_sigsetp) sigset_t *saved_ssp = NULL;
^
1 error generated.
```
The function returns non-negative UnitNameFlags on success, and negative
errno on error. In the past we kept the return type as int because of those
negative return values. But nowadays _UNIT_NAME_INVALID == -EINVAL. And if
we tried to actually return something that doesn't fit in the return type,
the compiler would throw an error. By changing to the "real" return type,
we allow the debugger to use symbolic representation for the variables.
We had the following scenario:
under /etc/systemd/system/
- foo@.service
- bar@tty12.service → foo@tty12.service
- multi-user.target.wants/foo@tty12.service
Existing code did not "know" that foo@tty12.service has alias bar@tty12.service:
$ systemctl show -P Names foo@tty12.service
foo@tty12.service
Since multi-user.target is always loaded, we would load foo@tty12.service.
When trying to load bar@tty12.service, it would (correctly) detect that
bar@tty12.service is an alias for foo@tty12.service, and try to merge the
bar@tty12.service unit into the foo@tty12.service. This would fail, because
foo@tty12.service was already loaded, and only about-to-be-loaded units can
be merged.
With the patch we consider bar@tty12.service an alias of foo@tty12.service
immediately, so the issue does not occur:
$ systemctl show -P Names foo@tty12.service
foo@tty12.service bar@tty12.service
Fixes#19409.
This turned in a bigger rewrite. The logic add "the main name and all aliases"
was implemented twice, slightly different in both cases. I split that part out
to a new function. The result about the same length, but hopefully a bit easier
to read.
Logging output is also improved a bit. Some left-over debug logs have been
removed or cleaned up.
This is a fairly big change, but (with the addition in the following commit),
we have pretty good coverage of this logic.
Hidden and backup files cannot be valid unit name (we reject anything
starting with a dot, and we require type suffixes). So let's not iterate
over those at all.
The general idea is that when a unit file is "linked" (i.e. installed by
symlinking from outside of the search paths), the *destination* name is
irrelevant. It doesn't even have to be a valid unit name, or to match the type
or instance value. The obvious collorary is that we shouldn't look at the
symlink destination name to derive the unit name, instance value, or anything
else at all.
When building the name map, when we find a linked unit (possibly at the end
of a series of alias redirects), store the *source* of the final symlink as the
fragment path. This has two effects:
- we stop looking at the *target* file name to derive unit info, i.e. actually
implement the stuff described in the first paragraph.
- we load the unit fragment through the symlink. If someone were to remove the
symlink, we'll not load the unit. This seems like the right thing.
Fixes#18058.
Before this change, we were generally quite confused about unit alises for
linked units. Fortunately most poeple use the same symlink source and target,
so in practice we wouldn't hit this too often.
In unit_load_fragment() a comment is added to explain what we're doing there.
