Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

install: follow unit file symlinks in /usr, but not /etc when looking for [Install] data and more #1515

Merged
merged 5 commits into from Nov 12, 2015

Conversation

@poettering
Copy link
Member

poettering commented Oct 9, 2015

This replaces #1375, and adds quite a number of other fixes to the installation logic.

@msekletar please have a look, and check if this does what's necessary.

@msekletar

This comment has been minimized.

Copy link
Contributor

msekletar commented Oct 9, 2015

From a quick test it seems like new implementation doesn't handle all necessary cases. Here is a log from my test script. Let me know if you want me to share the script.

:: [   PASS   ] :: Command 'systemctl enable a' (Expected 0, got 0)
:: [   FAIL   ] :: Command 'systemctl is-enabled a' (Expected 0, got 1)
:: [   PASS   ] :: Command 'systemctl disable a' (Expected 0, got 0)
:: [   PASS   ] :: Command 'systemctl is-enabled a' (Expected 1, got 1)
:: [   PASS   ] :: Command 'systemctl reenable a' (Expected 0, got 0)
:: [   FAIL   ] :: Command 'systemctl is-enabled a' (Expected 0, got 1)
:: [   PASS   ] :: Command 'systemctl disable a' (Expected 0, got 0)
:: [   PASS   ] :: Command 'systemctl enable b' (Expected 0, got 0)
:: [   FAIL   ] :: Command 'systemctl is-enabled b' (Expected 0, got 1)
:: [   PASS   ] :: Command 'systemctl disable b' (Expected 0, got 0)
:: [   PASS   ] :: Command 'systemctl is-enabled b' (Expected 1, got 1)
:: [   PASS   ] :: Command 'systemctl reenable b' (Expected 0, got 0)
:: [   FAIL   ] :: Command 'systemctl is-enabled b' (Expected 0, got 1)
:: [   PASS   ] :: Command 'systemctl disable b' (Expected 0, got 0)
:: [   FAIL   ] :: Command 'systemctl enable  foo@tty4.service 2>&1 | grep -q Created' (Expected 0, got 1)
:: [   FAIL   ] :: Command 'systemctl is-enabled foo@tty4.service 2>&1 | grep -q enabled' (Expected 0, got 1)
:: [   FAIL   ] :: Command 'systemctl disable  foo@tty4.service 2>&1 | grep -q Removed' (Expected 0, got 1)
:: [   PASS   ] :: Command 'systemctl is-enabled foo@tty4.service' (Expected 1, got 1)
:: [   FAIL   ] :: Command 'systemctl reenable  foo@tty4.service 2>&1 | grep -q Created' (Expected 0, got 1)
:: [   FAIL   ] :: Command 'systemctl is-enabled foo@tty4.service' (Expected 0, got 1)
:: [   FAIL   ] :: Command 'systemctl disable foo@tty4.service 2>&1 | grep -q Removed' (Expected 0, got 1)
:: [   PASS   ] :: Command 'mount --bind / /mnt' (Expected 0, got 0)
:: [   PASS   ] :: Command 'systemctl --root=/mnt enable a' (Expected 0, got 0)
:: [   FAIL   ] :: Command 'systemctl --root=/mnt is-enabled a' (Expected 0, got 1)
:: [   PASS   ] :: Command 'systemctl --root=/mnt disable a' (Expected 0, got 0)
:: [   PASS   ] :: Command 'systemctl --root=/mnt is-enabled a' (Expected 1, got 1)
:: [   PASS   ] :: Command 'systemctl --root=/mnt reenable a' (Expected 0, got 0)
:: [   FAIL   ] :: Command 'systemctl --root=/mnt is-enabled a' (Expected 0, got 1)
:: [   PASS   ] :: Command 'systemctl --root=/mnt disable a' (Expected 0, got 0)
:: [   FAIL   ] :: Command 'systemctl --root=/mnt enable b' (Expected 0, got 1)
:: [   FAIL   ] :: Command 'systemctl --root=/mnt is-enabled b' (Expected 0, got 1)
:: [   PASS   ] :: Command 'systemctl --root=/mnt disable b' (Expected 0, got 0)
:: [   PASS   ] :: Command 'systemctl --root=/mnt is-enabled b' (Expected 1, got 1)
:: [   FAIL   ] :: Command 'systemctl --root=/mnt reenable b' (Expected 0, got 1)
:: [   FAIL   ] :: Command 'systemctl --root=/mnt is-enabled b' (Expected 0, got 1)
:: [   FAIL   ] :: Command 'systemctl --root=/mnt enable  foo@tty4.service 2>&1 | grep -q Created' (Expected 0, got 1)
:: [   FAIL   ] :: Command 'systemctl --root=/mnt is-enabled foo@tty4.service 2>&1 | grep -q enabled' (Expected 0, got 1)
:: [   FAIL   ] :: Command 'systemctl --root=/mnt disable  foo@tty4.service 2>&1 | grep -q Removed' (Expected 0, got 1)
:: [   PASS   ] :: Command 'systemctl --root=/mnt is-enabled foo@tty4.service' (Expected 1, got 1)
:: [   FAIL   ] :: Command 'systemctl --root=/mnt reenable  foo@tty4.service 2>&1 | grep -q 'Created symlink'' (Expected 0, got 1)
:: [   FAIL   ] :: Command 'systemctl --root=/mnt is-enabled foo@tty4.service' (Expected 0, got 1)
:: [   LOG    ] :: Duration: 2s
:: [   LOG    ] :: Assertions: 20 good, 21 bad
:: [   FAIL   ] :: RESULT: Enable unit in /usr via symlink


@poettering

This comment has been minimized.

Copy link
Member Author

poettering commented Oct 9, 2015

What are "all necessary cases"? Can you please write down what happens what should not happen or what doesn't happen that should happen? Something I can reproduce easily...

@msekletar

This comment has been minimized.

Copy link
Contributor

msekletar commented Oct 10, 2015

Just from top of my head here is the list of things I deem are necessary to implement correctly,

  • enable must work for symlink which is absolute (/usr/lib/systemd/system/mdns.service -> /usr/lib/systemd/system/avahi-daemon.service)
  • enablemust work for relative symlink (/usr/lib/systemd/system/mdns-relative.service -> avahi-daemon.service)
  • it must be possible to enable instance via symlink to template (foo@.service -> getty@.service)
  • is-enabled must return correct status for all cases
  • disable must do the right thing for all cases
  • all verbs must work correctly when code is invoked from systemctl --root
  • code should be robust to handle misconfiguration, e.g.
foobar.service
[Install]
WantedBy=multi-user.target
Also=baz.service

and baz.service -> foobar.service
systemd must not enter some busy loop or crash

@poettering poettering force-pushed the poettering:install-symlink branch from 523da9d to 2a231b0 Oct 31, 2015
@poettering

This comment has been minimized.

Copy link
Member Author

poettering commented Oct 31, 2015

I force-pushed a new version now. Note that it's based on #1737, which is hence included in this PR, but please ignore this here, and look at #1737 for that. That PR should be merged before this one.

@msekletar I think I fixed everything you pointed out now. I also added a proper test that tests all that, so that we can verify that all is good. Please check out if everything works for you now, and if not, let me know specifically which is missing so that we can add testcases for that too.

@msekletar

This comment has been minimized.

Copy link
Contributor

msekletar commented Nov 2, 2015

@poettering If you enable instance via symlink then you can't disable it. There is no error but systemd doesn't do anything. Looks OK, otherwise.

-bash-4.3# systemctl enable foo@tty4.service
Created symlink from /etc/systemd/system/getty.target.wants/getty@tty4.service to /usr/lib/systemd/system/getty@.service.
-bash-4.3# systemctl disable foo@tty4.service
-bash-4.3#
poettering added 5 commits Oct 8, 2015
Instead, let the caller do that. Fix this by moving masked unit messages
into the caller, by returning a clear error code (ESHUTDOWN) by which
this may be detected.
… for [Install] data

Some distributions use alias unit files via symlinks in /usr to cover
for legacy service names. With this change we'll allow "systemctl
enable" on such aliases.

Previously, our rule was that symlinks are user configuration that
"systemctl enable" + "systemctl disable" creates and removes, while unit
files is where the instructions to do so are store. As a result of the
rule we'd never read install information through symlinks, since that
would mix enablement state with installation instructions.

Now, the new rule is that only symlinks inside of /etc are
configuration. Unit files, and symlinks in /usr are now valid for
installation instructions.

This patch is quite a rework of the whole install logic, and makes the
following addional changes:

- Adds a complete test "test-instal-root" that tests the install logic
  pretty comprehensively.

- Never uses canonicalize_file_name(), because that's incompatible with
  operation relative to a specific root directory.

- unit_file_get_state() is reworked to return a proper error, and
  returns the state in a call-by-ref parameter. This cleans up confusion
  between the enum type and errno-like errors.

- The new logic puts a limit on how long to follow unit file symlinks:
  it will do so only for 64 steps at max.

- The InstallContext object's fields are renamed to will_process and
  has_processed (will_install and has_installed) since they are also
  used for deinstallation and all kinds of other operations.

- The root directory is always verified before use.

- install.c is reordered to place the exported functions together.

- Stricter rules are followed when traversing symlinks: the unit suffix
  must say identical, and it's not allowed to link between regular units
  and templated units.

- Various modernizations

- The "invalid" unit file state has been renamed to "bad", in order to
  avoid confusion between UNIT_FILE_INVALID and
  _UNIT_FILE_STATE_INVALID. Given that the state should normally not be
  seen and is not documented this should not be a problematic change.
  The new name is now documented however.

Fixes #1375, #1718, #1706
Previously, the %u, %U, %s and %h specifiers would resolve to the user
name, numeric user ID, shell and home directory of the user configured
in the User= setting of a unit file, or the user of the manager instance
if no User= setting was configured. That at least was the theory. In
real-life this was not ever actually useful:

- For the systemd --user instance it made no sense to ever set User=,
  since the instance runs in user context after all, and hence the
  privileges to change user IDs don't even exist. The four specifiers
  were actually not useful at all in this case.

- For the systemd --system instance we did not allow any resolving that
  would require NSS. Hence, %s and %h were not supported, unless
  User=root was set, in which case they would be hardcoded to /bin/sh
  and /root, to avoid NSS. Then, %u would actually resolve to whatever
  was set with User=, but %U would only resolve to the numeric UID of
  that setting if the User= was specified in numeric form, or happened
  to be root (in which case 0 was hardcoded as mapping). Two of the
  specifiers are entirely useless in this case, one is realistically
  also useless, and one is pretty pointless.

- Resolving of these settings would only happen if User= was actually
  set *before* the specifiers where resolved. This behaviour was
  undocumented and is really ugly, as specifiers should actually be
  considered something that applies to the whole file equally,
  independently of order...

With this change, %u, %U, %s and %h are drastically simplified: they now
always refer to the user that is running the service instance, and the
user configured in the unit file is irrelevant. For the system instance
of systemd this means they always resolve to "root", "0", "/bin/sh" and
"/root", thus avoiding NSS. For the user instance, to the data for the
specific user.

The new behaviour is identical to the old behaviour in all --user cases
and for all units that have no User= set (or set to "0" or "root").
@poettering poettering force-pushed the poettering:install-symlink branch from c7e96bf to 79413b6 Nov 12, 2015
@poettering

This comment has been minimized.

Copy link
Member Author

poettering commented Nov 12, 2015

OK, I force-pushed a new version. One that hopefully is liked by semaphore too. It fixes the disable issue you pointed out, too, @msekletar.

If semaphore likes it, I'd really like to see this one commited, before we do any further fixing on it. With the tests this includes we have a verification that it does what it is supposed to do better than we ever had...

@poettering

This comment has been minimized.

Copy link
Member Author

poettering commented Nov 12, 2015

Nice, semaphore likes it too now. Yay!

@zonque

This comment has been minimized.

Copy link

zonque commented on src/shared/install.c in d25e100 Nov 12, 2015

This also iterates over '.' and '..', right? Without the hidden_file() check, how to you prevent looking at those? Do I miss anything?

This comment has been minimized.

Copy link
Owner Author

poettering replied Nov 12, 2015

FOREACH_DIRENT() already includes that check. If you really want to traverse even through the weird stuff like "." and "..", use FOREACH_DIRENT_ALL() instead...

This comment has been minimized.

Copy link

zonque replied Nov 12, 2015

Ah, right. Thanks!

@zonque

This comment has been minimized.

Copy link

zonque commented on src/shared/install.c in d25e100 Nov 12, 2015

Dito

This comment has been minimized.

Copy link
Owner Author

poettering replied Nov 12, 2015

Dito too! ;-)

@zonque

This comment has been minimized.

Copy link
Member

zonque commented Nov 12, 2015

Ok, this has been around for a while and it looks all sane to me, so I'll just merge it now. @msekletar, if you spot any other things, please open a new PR and also extend the test suite along with the fix :)

zonque added a commit that referenced this pull request Nov 12, 2015
install: follow unit file symlinks in /usr, but not /etc when looking for [Install] data and more
@zonque zonque merged commit f1637bd into systemd:master Nov 12, 2015
1 check passed
1 check passed
semaphoreci The build passed on Semaphore.
Details
@poettering

This comment has been minimized.

Copy link
Member Author

poettering commented Nov 12, 2015

AWESOME! Thanks for merging!

@msekletar yes, please give this a whirl. If you find anything, would be cool to prep some new tests for test-install-root that fail, but which should not, and pass those to me, and I'll look into them. For me it's much easier if whatever is left now we can just fix as it comes up, and always immediately stabilize things with tests.

@msekletar

This comment has been minimized.

Copy link
Contributor

msekletar commented Nov 13, 2015

@poettering everything seems to work as expected.

@poettering

This comment has been minimized.

Copy link
Member Author

poettering commented Nov 13, 2015

Excellent! Thanks for testing! And thanks for prepping the initial patches. In the end adding this turned out to be a much more complex job than I initially thought, hence I hope it's OK I took this over.

@msekletar

This comment has been minimized.

Copy link
Contributor

msekletar commented Nov 13, 2015

Your help with this was much appreciated. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.