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

systemctl: respect [Install] section in drop-ins #7158

Merged
merged 1 commit into from Nov 8, 2017

Conversation

@johnlinp
Copy link
Contributor

commented Oct 22, 2017

Fixes: #7114

@johnlinp

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2017

The patch isn't complete yet. However, it has the following effects:

  1. WantedBy is respected in the drop-in:
# /lib/systemd/system/a.service
[Service]
ExecStart=/bin/sleep 5566
# /etc/systemd/system/a.service.d/d.conf
[Install]
WantedBy=multi-user.target

then:

[root@image ~]# systemctl enable a.service
Created symlink /etc/systemd/system/multi-user.target.wants/a.service -> /usr/lib/systemd/system/a.service.
  1. Template units too:
# /lib/systemd/system/b@.service
[Service]
ExecStart=/bin/sleep 5566
# /etc/systemd/system/b@.service.d/d.conf
[Install]
WantedBy=multi-user.target
# /etc/systemd/system/b@i1.service.d/d.conf
[Install]
WantedBy=graphica.target

then:

[root@image ~]# systemctl enable b@i1.service
Created symlink /etc/systemd/system/graphical.target.wants/b@i1.service -> /usr/lib/systemd/system/b@.service.
[root@image ~]# systemctl enable b@i2.service
Created symlink /etc/systemd/system/multi-user.target.wants/b@i2.service -> /usr/lib/systemd/system/b@.service.

The patch has the following problems:

  1. It doesn't handle the config ordering between /usr/lib, /var/run, /etc and others.
  2. Duplicate loading logic with the other sections (the ones that need systemctl daemon-reload).
  3. Maybe bad coding style.

Any comment would be appreciated. Thanks.

@keszybz
Copy link
Member

left a comment

This looks like a good start, but please note that dropins files in instance .d directories should have a higher priority then the identically names files in the template .d directories. (It's possible that your patch already behaves like that. It seems to me that it doesn't, but I haven't looked at the code recently). So please test that. In general we want the same semantics as for normal dropins.

STRV_FOREACH(p, paths->search_path) {
_cleanup_free_ char *path = NULL;

path = strjoin(*p, "/", info->name, ".d", NULL);

This comment has been minimized.

Copy link
@keszybz

keszybz Oct 23, 2017

Member

path_join should be used in general, it is needed for arg_root support. So you probably need to concatentate info->name and ".d" first using strjoina (on a seperate line), outside of the loop, and then call path_join here.

path = strjoin(*p, "/", info->name, ".d", NULL);
if (!path)
return -ENOMEM;
strv_extend(&dirs, path);

This comment has been minimized.

Copy link
@keszybz

keszybz Oct 23, 2017

Member

We have strv_consume for this case, and then cleanup_free is not needed. Also, the return value must be checked.

This comment has been minimized.

Copy link
@johnlinp

johnlinp Oct 24, 2017

Author Contributor

Got it. Thank you.

strv_extend(&dirs, path);

if (template) {
path = strjoin(*p, "/", template, ".d", NULL);

This comment has been minimized.

Copy link
@keszybz

keszybz Oct 23, 2017

Member

path_join and strv_consume again.


r = conf_files_list_strv(&files, ".conf", NULL, 0, (const char**) dirs);
if (r < 0) {
log_error("Failed to get list of configuration files: %s", strerror(-r));

This comment has been minimized.

Copy link
@keszybz

keszybz Oct 23, 2017

Member

log_error_errno and %m.

}

STRV_FOREACH(p, files) {
r = unit_file_load_or_readlink(c, info, *p, paths->root_dir, flags);

This comment has been minimized.

Copy link
@keszybz

keszybz Oct 23, 2017

Member

If r is ignored, do not assign it. Also, no {} around single lines.

@johnlinp

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2017

@keszybz Thank you for the comments! I am working on it.

@yuwata yuwata added the systemctl label Oct 25, 2017

@yuwata

This comment has been minimized.

Copy link
Member

commented Oct 25, 2017

I think test-install.c should be extended to support drop-in files.

@johnlinp

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2017

@yuwata I will try adding tests for it. Thank you.

@johnlinp johnlinp force-pushed the johnlinp:master branch 2 times, most recently from ea389ff to 8848486 Nov 1, 2017

@johnlinp

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2017

This is my second version. Still no tests for it, but I will add them in the next version. It deals with the priority between drop-in conf files and the units. Also, template drop-ins and instanced drop-ins priority is taken care of.

If this patch is fine, I will start work on the tests. Thank you.

@johnlinp

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2017

@yuwata I found that test-install.c is a manual test. How do you run manual tests? I mean, there are files like avahi-daemon.service and /home/lennart/test.service that are not in the git repo, how do I know the content of those testing units?

@johnlinp

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2017

@yuwata I think I am going to add tests in test-install-root.c. Thanks.

@yuwata

This comment has been minimized.

Copy link
Member

commented Nov 4, 2017

@johnlinp Yeah, you are right. Please update test-install-root.c. Thanks. Sorry for late reply.

r = strv_consume(&dirs, path);
if (r < 0) {
return r;
}

This comment has been minimized.

Copy link
@keszybz

keszybz Nov 5, 2017

Member

No braces around single-line blocks please.

This comment has been minimized.

Copy link
@johnlinp

johnlinp Nov 6, 2017

Author Contributor

Sorry, I forgot.


dropin_dir_name = strjoina(info->name, ".d");
STRV_FOREACH(p, paths->search_path) {
char *path = NULL;

This comment has been minimized.

Copy link
@keszybz

keszybz Nov 5, 2017

Member

Don't initialize this, since it get's overwritten in the next line.

This comment has been minimized.

Copy link
@johnlinp

johnlinp Nov 6, 2017

Author Contributor

Okay, I will fix it.

r = conf_files_list_strv(&files, ".conf", NULL, 0, (const char**) dirs);
if (r < 0) {
log_error_errno(r, "Failed to get list of conf files: %m");
return r;

This comment has been minimized.

Copy link
@keszybz

keszybz Nov 5, 2017

Member

log_error_errno returns r, so return log_errno_errno(r, ...);.
Also, this is a "library" function. Above, all log statements were at debug level. You need to the same in the part you add: either use log_debug or nothing at all.

This comment has been minimized.

Copy link
@johnlinp

johnlinp Nov 6, 2017

Author Contributor

Excuse me, I don't understand. I see unit_file_load() contains log_error_errno too. Does that mean unit_file_load() is not a library function?

This comment has been minimized.

Copy link
@keszybz

keszybz Nov 8, 2017

Member

The general rule is that any specific function should either always log or never log. This way we can arrange things that we always get an error message, but not more than one. Generally it's nice if the caller outputs the logs, but sometimes it's not possible because the caller does not have enough information for a truly useful error message. We are not very consistent in this unfortunately.

This comment has been minimized.

Copy link
@johnlinp

johnlinp Nov 8, 2017

Author Contributor

I see. Thanks for your helpful explanation.

@johnlinp

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2017

Hi, here is my new patch. I have the following changes:

  1. Add unit tests in test-install-root.c as @yuwata suggested.
  2. Fix what @keszybz reviewed.
  3. Remove paths->root_dir when path_join() because it's redundant there.
  4. Pass info->name as unit into parser functions so that the parser functions can check the unit names (basically for config_parse_default_instance()).

Thank you for reviewing my code.

@johnlinp johnlinp force-pushed the johnlinp:master branch from 8848486 to e616a8a Nov 6, 2017

@keszybz

keszybz approved these changes Nov 8, 2017

Copy link
Member

left a comment

Looks great.

assert(filename);
assert(lvalue);
assert(rvalue);

name = basename(filename);
type = unit_name_to_type(name);
type = unit_name_to_type(unit);

This comment has been minimized.

Copy link
@keszybz

keszybz Nov 8, 2017

Member

(Note to self: we check that the filename extension matches unit type, so this change should have no effect.)

@@ -1421,23 +1431,23 @@ static int unit_file_search(
return -ENOMEM;

r = unit_file_load_or_readlink(c, info, path, paths->root_dir, flags);

This comment has been minimized.

Copy link
@keszybz

keszybz Nov 8, 2017

Member

We like to keep the action and the handling of the result nearby. This empty line is unnecessary.

@keszybz

This comment has been minimized.

Copy link
Member

commented Nov 8, 2017

Changes to the installation procedure are tricky, but the code looks good. Let's merge this and keep an eye out for any regressions.

@keszybz keszybz merged commit 142468d into systemd:master Nov 8, 2017

4 of 5 checks passed

xenial-amd64 autopkgtest running
Details
Fedora Rawhide CI x86_64 rpm build succeeded
Details
semaphoreci The build passed on Semaphore.
Details
xenial-i386 autopkgtest finished (success)
Details
xenial-s390x autopkgtest finished (success)
Details
@johnlinp

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2017

Thank you! If there is any regression, please assign the bug to me. Thanks.

@evverx

This comment has been minimized.

Copy link
Member

commented Nov 9, 2017

I've just opened #7283. @johnlinp could you take a look?

@johnlinp

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2017

@evverx I think I found the leak. I will update in #7283.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.