systemctl: warn when cat shows changed unit files by lucaswerkmeister · Pull Request #4493 · systemd/systemd

lucaswerkmeister · 2016-10-26T10:13:53Z

The warning is based on warn_unit_file_changed, but there are so many changes that I think it’s better to copy a bit of code than to attempt to generalize warn_unit_file_changed to fit this use case:

Comment (message starts with #)
Blue instead of normal color
stdout instead of stderr
More verbose warning message
Split up across two lines due to the more verbose warning message

Alternatively, here’s a minimal patch:

diff --cc src/systemctl/systemctl.c
index 35d5c11,35d5c11..85b3dfd
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@@ -5272,6 -5272,6 +5272,11 @@@ static int cat(int argc, char *argv[], 
                  else
                          puts("");

++                if (need_daemon_reload(bus, *name)) {
++                        fflush(stdout);
++                        warn_unit_file_changed(*name);
++                }
++
                  if (fragment_path) {
                          r = cat_file(fragment_path, false);
                          if (r < 0)

It prints the warning without any special formatting to stderr, which doesn’t break systemctl cat foo.service > foo2.service, but looks ugly in the terminal.

keszybz · 2016-10-26T12:15:45Z

src/systemctl/systemctl.c

                        puts("");

+                if (need_daemon_reload(bus, *name))
+                        printf("%s# %sWarning:%s %s changed on disk, showing current disk contents.\n"


Ha, I thought that this would be a more complicated patch. It's nice when existing functionality gets reused like that.

It prints the warning without any special formatting to stderr, which doesn’t break systemctl cat foo.service > foo2.service, but looks ugly in the terminal.

Hm, maybe we could print the warning to stdout if we were originally on the terminal (before the pager was started), and to stderr otherwise? We do a similar check in various other cases.

Why make the message blue? Shouldn't it be red as other warnings?

I'm not sure about the message:

# Warning: abrt-ccpp.service changed on disk, showing current disk contents. # Run 'systemctl daemon-reload' to reload units.

The first line basically says that you're showing the latest version ;)
There are two cases to consider:

an existing file was modified, either the main unit file, or one of the drop-ins.

a drop in was added or removed

In the first case, cat is showing good content, but the manager has an outdated view.
In the second case, cat is showing bad content, and the manager has an outdated view.
I don't think we can distinguish those cases with existing functionality, so the message should be updated to reflect both. Maybe something like "...service was changed on disk, systemd has an outdated version loaded. If drop-ins were added or removed, they are not properly reflected in this output. Run 'systmectl daemon-reload' to update to the version shown here."

Ha, I thought that this would be a more complicated patch. It's nice when existing functionality gets reused like that.

Yes, I’m very glad I didn’t have to add a new dbus method to return unit files + time loaded or something like that :)

Why make the message blue? Shouldn't it be red as other warnings?

The word “Warning” is still red, just as in systemctl status. I only changed the color of the rest of the text from default to blue, to match the file name comments.

The first line basically says that you're showing the latest version ;)

I’m not sure what you mean… systemctl cat really is cat, it boils down to open(filename) and copy_bytes(fd, STDOUT_FILENO) (see function cat_file), showing whatever is currently on disk, not what the daemon loaded. Are you saying the message sounds like it’s showing what the daemon loaded? Then I should clarify it.

There’s another condition to consider, which exhibits the same behavior as the second case.

a file was added that replaces the main unit file (/etc vs. /usr/lib, ~/.config vs. /etc, etc.)

This case is also not detected by need_daemon_reload.

I’m not sure if the message should reflect both cases – unless I’m missing something, we won’t even print the message in the second case, since we don’t know it’s happening.

keszybz · 2016-10-26T17:26:30Z

On Wed, Oct 26, 2016 at 08:51:43AM -0700, Lucas Werkmeister wrote:

The word “Warning” is still red, just as in systemctl status. I only changed the color of the rest of the text from default to blue, to match the file name comments.

I think the whole thing should be in red. I used blue for the
comments with the file name because it was supposed to not stand
out too much. A warning is different.

The first line basically says that you're showing the latest version ;)

I’m not sure what you mean…

I'm saying that 'cat' always shows the latest version (unless a file a
higher-priority path was added, as you said), so "showing the latest version"
is generally true for 'systemctl cat'. Yeah, the message should be updated.

There’s another condition to consider, which exhibits the same behavior as the second case.

a file was added that replaces the main unit file (/etc vs. /usr/lib, ~/.config vs. /etc, etc.)

This case is also not detected by need_daemon_reload.

That's nasty. It'd be great to fix it for both 'systemctl cat' and
'systemctl status'. (Separate patch). I think we can do this on the client
side, by taking the path that we got over dbus, and checking higher-priority
paths in the filesystem for an overriding unit file.

I’m not sure if the message should reflect both cases

I think it should, since we should be able to detect both kinds of cases.

lucaswerkmeister · 2016-10-26T18:14:28Z

New version pushed. Entire message is now red, and the text has been updated.

I think we can do this on the client side, by taking the path that we got over dbus, and checking higher-priority paths in the filesystem for an overriding unit file.

Why not fix the dbus property (NeedDaemonReload)? The current behavior doesn’t seem too useful to me. Or would that be a breaking change and not okay?

I think it should, since we should be able to detect both kinds of cases.

I was thinking it should only be updated when both kinds of cases are actually detected, but sure.

keszybz · 2016-10-27T00:23:22Z

Issue #4500 is for the needs-reload part.

What about this comment?

maybe we could print the warning to stdout if we were originally on the terminal (before the pager was started), and to stderr otherwise?

lucaswerkmeister · 2016-10-27T07:50:15Z

What about this comment?

Sorry, I missed that. But if we are using the pager, then both stdout and stderr go into the pager (see pager.c, lines 148-151), so wouldn’t it be better to always print to stderr?

(I have an updated patch ready for either version, just let me know which one you’d prefer.)

keszybz · 2016-10-27T12:10:21Z

Sorry, I missed that. But if we are using the pager, then both stdout and stderr go into the pager (see pager.c, lines 148-151), so wouldn’t it be better to always print to stderr?

Ah, OK, I forgot that. So yeah, stderr seems fine.

lucaswerkmeister · 2016-10-27T12:11:52Z

Alright, thanks, updated again.

@keszybz

Suggested by @keszybz in systemd#4488.

keszybz · 2016-10-27T13:28:02Z

Thanks.

lucaswerkmeister · 2016-10-28T09:58:34Z

Ouch, I just noticed that this caused CI failures, and they don’t look spurious…

logind FAIL stderr: # Warning: systemd-suspend.service changed on disk, the version systemd has loaded is outdated.

Looks like something is assuming that any stderr output means a test failure. I’ll try to fix this.

martinpitt · 2016-10-28T10:08:43Z

Looks like something is assuming that any stderr output means a test failure

Yes, as that's a good tool to notice new unexpected warnings. I can disable that behaviour in the test, or filter it out. But ATM I don't actually see failures, just the i386 and amd64 tests are still running (i. e. this was apparently merged without waiting for CI to finish).

martinpitt · 2016-10-28T10:10:46Z

Oh, I do see them now, e. g. in PR #4508 :-(

martinpitt · 2016-10-28T10:22:52Z

I moved from systemctl cat to something more robust in https://anonscm.debian.org/cgit/pkg-systemd/systemd.git/commit/?id=16d72ac45 , this should fix it.

lucaswerkmeister · 2016-10-28T10:27:01Z

Thanks, that looks like a better solution. test/networkd-test.py seems to do something similar to extract systemd-networkd’s ExecStart, perhaps that could use show as well?

martinpitt · 2016-10-28T12:57:24Z

Like that perhaps?

systemctl show --property=ExecStart --value systemd-networkd.service | sed 's/^.*argv\[\]=//; s/;.*$//'

I'll put up a PR for this.

martinpitt · 2016-10-28T12:59:05Z

... but this isn't actually important, as this only runs through systemd-run which consumes stderr anyway.

lucaswerkmeister · 2016-10-28T13:00:08Z

Oh, right, I forgot that systemctl show’s ExecStart output is different. Not sure if it’s even worth it in that case…

evverx · 2016-11-02T12:54:39Z

I see this warning always:

-bash-4.3# cat <<'EOF' >/etc/systemd/system/hola.service
[Service]
ExecStart=/bin/sh -x -c id
EOF

-bash-4.3# systemctl cat hola --no-pager
# Warning: hola.service changed on disk, the version systemd has loaded is outdated.
# This output shows the current version of the unit's original fragment and drop-in files.
# If fragments or drop-ins were added or removed, they are not properly reflected in this output.
# Run 'systemctl daemon-reload' to reload units.
# /etc/systemd/system/hola.service
[Service]
ExecStart=/bin/sh -x -c id

-bash-4.3# systemctl daemon-reload

-bash-4.3# systemctl cat hola --no-pager
# Warning: hola.service changed on disk, the version systemd has loaded is outdated.
# This output shows the current version of the unit's original fragment and drop-in files.
# If fragments or drop-ins were added or removed, they are not properly reflected in this output.
# Run 'systemctl daemon-reload' to reload units.
# /etc/systemd/system/hola.service
[Service]
ExecStart=/bin/sh -x -c id

-bash-4.3# systemctl show --value --property=NeedDaemonReload hola --no-pager
no

@evverx

Reported by @evverx in systemd#4493.

lucaswerkmeister · 2016-11-02T15:15:09Z

@evverx sorry, suggested fix is in #4535.

@evverx

Reported by @evverx in #4493.

…memory one With systemd/systemd#4493, systemctl cat now warns that the on-disk unit changed, which causes failure due to unexpected stderr. We actually intend to use the on-disk one instead of the potentially older in-memory one, so change this to ask for the unit file path and operate on that instead.

keszybz added the systemctl label Oct 26, 2016

keszybz requested changes Oct 26, 2016

View reviewed changes

lucaswerkmeister force-pushed the cat-warn-changed branch from 7dff7e8 to 01835d3 Compare October 26, 2016 18:12

lucaswerkmeister force-pushed the cat-warn-changed branch from 01835d3 to 1710476 Compare October 27, 2016 12:11

systemctl: warn when cat shows changed unit files

1710476

Suggested by @keszybz in systemd#4488.

keszybz merged commit e100155 into systemd:master Oct 27, 2016

lucaswerkmeister deleted the cat-warn-changed branch October 27, 2016 13:39

martinpitt mentioned this pull request Oct 28, 2016

tests: clarify test_path_startswith return value #4508

Merged

lucaswerkmeister added a commit to lucaswerkmeister/systemd that referenced this pull request Nov 2, 2016

systemctl: fix incorrect "need reload" on cat

10899a0

Reported by @evverx in systemd#4493.

lucaswerkmeister mentioned this pull request Nov 2, 2016

systemctl: fix incorrect "need reload" on cat #4535

Merged

poettering pushed a commit that referenced this pull request Nov 2, 2016

systemctl: fix incorrect "need reload" on cat (#4535)

5b9635d

Reported by @evverx in #4493.

Uh oh!

Conversation

lucaswerkmeister commented Oct 26, 2016

Uh oh!

keszybz Oct 26, 2016

Choose a reason for hiding this comment

Uh oh!

lucaswerkmeister Oct 26, 2016

Choose a reason for hiding this comment

Uh oh!

keszybz commented Oct 26, 2016

Uh oh!

lucaswerkmeister commented Oct 26, 2016

Uh oh!

keszybz commented Oct 27, 2016

Uh oh!

lucaswerkmeister commented Oct 27, 2016

Uh oh!

keszybz commented Oct 27, 2016

Uh oh!

lucaswerkmeister commented Oct 27, 2016

Uh oh!

keszybz commented Oct 27, 2016

Uh oh!

lucaswerkmeister commented Oct 28, 2016

Uh oh!

martinpitt commented Oct 28, 2016

Uh oh!

martinpitt commented Oct 28, 2016

Uh oh!

martinpitt commented Oct 28, 2016

Uh oh!

lucaswerkmeister commented Oct 28, 2016

Uh oh!

martinpitt commented Oct 28, 2016

Uh oh!

martinpitt commented Oct 28, 2016

Uh oh!

lucaswerkmeister commented Oct 28, 2016

Uh oh!

evverx commented Nov 2, 2016

Uh oh!

lucaswerkmeister commented Nov 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants