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

allow variable substition within Exec functions #4818

Closed
wants to merge 3 commits into from

Conversation

james-lawrence
Copy link

@james-lawrence james-lawrence commented Dec 4, 2016

implements #3061 interested in getting first pass eyes on this.

open questions

  • tests suggestions, I see test/test-execute, do I just define service files in there? how does one setup the user session?
  • how does setting User=soandso impact the expansion when run by init? what other settings may impact this?
  • are there other functions like config_parse_exec I should do as part of this? are there different methods for ExecStop, ExecRestart, etc?
  • documentation updates

things I noticed

  • building the image periodically fails on the docbook-xsl/xml transformations for the arch image. highly annoying, required uninstall-reinstall of docbook-xsl, docbook-xml. is something corrupting those packages file on the local system?
  • the following comment in the code snippet below says it supports unescaping, implying that unit_name_printf does not. however the code looks identical besides the additional codes.
int unit_full_printf(Unit *u, const char *format, char **ret) {

        /* This is similar to unit_name_printf() but also supports
         * unescaping. Also, adds a couple of additional codes:
         *
         * %f the instance if set, otherwise the id
         * %c cgroup path of unit
         * %r where units in this slice are placed in the cgroup tree
         * %R the root of this systemd's instance tree
         * %t the runtime directory to place sockets in (e.g. "/run" or $XDG_RUNTIME_DIR)
         * %U the UID of the running user
         * %u the username of the running user
         * %h the homedir of the running user
         * %s the shell of the running user
         * %m the machine ID of the running system
         * %H the host name of the running system
         * %b the boot ID of the running system
         * %v `uname -r` of the running system
         */

Examples

  • sample unit files and their output.
[Unit]
Description=echo service

[Service]
Type=simple
ExecStart=%h/.local/bin/echo %p %P %m %H %u %U %s %h %n %N %f %c %r %R %t %v %%U

[Install]
WantedBy=default.target
echo echo bf3ef719c51644f1bf658596d29d6a3a arch.raw archie 1000 /bin/bash /home/archie echo.service echo /echo /user.slice/user-1000.slice/user@1000.service/echo.service /user.slice/user-1000.slice/user@1000.service /user.slice/user-1000.slice/user@1000.service /run/user/1000 4.8.11-1-ARCH %U
[Unit]
Description=echo service

[Service]
Type=simple
ExecStart=%m/.local/bin/echo %p %P %m %H %u %U %s %h %n %N %f %c %r %R %t %v

[Install]
WantedBy=default.target
Dec 03 19:43:25 arch.raw systemd[28]: [/home/archie/.local/share/systemd/user/echo.service:7] Executable path is not absolute, ignoring: %m/.local/bin/echo %p %P %m %H %u %U %s %h %n %N %f %c %r %R %t %v

@spiette, @rektide, @DrYak would any of you be interested in helping test out this patch since you also seemed interested in the functionality?

Copy link
Member

@evverx evverx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, but I've already reverted such commit: bd1b973

$ sudo ./libtool --mode=execute ./test-execute 2>&1
...
+ perl -e 'exit(!(qq{0} eq qq{\x25U}))'
exec-spec-interpolation.service: Main process exited, code=exited, status=1/FAILURE
exec-spec-interpolation.service: Unit entered failed state.
exec-spec-interpolation.service: Failed with result 'exit-code'.
        PID: 11570
        Start Timestamp: Sun 2016-12-04 21:09:08 UTC
        Exit Timestamp: Sun 2016-12-04 21:09:08 UTC
        Exit Code: exited
        Exit Status: 1
Assertion 'service->main_exec_status.status == status_expected' failed at src/test/test-execute.c:69, function check(). Aborting.
Aborted

@james-lawrence
Copy link
Author

@evverx any good resources for running the test suite? I'm still working my way through the codebase while I look at this. from the test just seems like it can't be applied to later portions of the command because they get processed again later.

@evverx
Copy link
Member

evverx commented Dec 4, 2016

@evverx any good resources for running the test suite?

Not sure I understand the question.
Some tests (for example test-execute) require a root privileges. So, you need to run make check as root.

Also, you can run our tests under valgrind: sudo make valgrind-tests. This helps to find the memory leak:

==18164== 5,018 bytes in 67 blocks are definitely lost in loss record 3 of 5
==18164==    at 0x4C2DB9D: malloc (vg_replace_malloc.c:299)
==18164==    by 0x4EF4925: malloc_multiply (alloc-util.h:70)
==18164==    by 0x4EF49FC: specifier_printf (specifier.c:52)
==18164==    by 0x1EFFAF: unit_full_printf (unit-printf.c:271)
==18164==    by 0x15EA99: config_parse_exec (load-fragment.c:602)
==18164==    by 0x4EDFB53: next_assignment (conf-parser.c:151)
==18164==    by 0x4EE0372: parse_line (conf-parser.c:270)
==18164==    by 0x4EE0910: config_parse (conf-parser.c:373)
==18164==    by 0x16DEC4: load_from_path (load-fragment.c:4219)
==18164==    by 0x16E0C5: unit_load_fragment (load-fragment.c:4258)
==18164==    by 0x1717EC: service_load (service.c:689)
==18164==    by 0x1387A0: unit_load (unit.c:1290)
==18164==

@james-lawrence
Copy link
Author

basically I havent had a chance to figure out how the mkosi stuff works in conjunction with the compilation process. Ideally I want to run the tests inside a container. atm the process is pretty slow because i'm rebuilding the image each time to manually test the code. a more incremental approach with be ideal. the hacking file has minimal details on that setup.

@james-lawrence
Copy link
Author

and thanks for the example for valgrand, I knew there was a memory leak in initial pr. I'm more interested in getting the logic correct atm. ensure memory cleanup will come after =)

@martinpitt
Copy link
Contributor

Fedora tests are broken, rest is green.

@poettering
Copy link
Member

I am pretty sure we should resolve specifiers exclusively when parsing unit files, not when we are about to activate. Hence I figure we should remove any invocation of unit_full_printf_strv() from service.c and socket.c.

@poettering
Copy link
Member

Hmm, so given that moving the stuff away from socket.c/service.c was a bit more complex I prepped a new PR now in #4835 that should replace this one. Please have a look. I am taking the liberty to close this one in favour of the new one. I hope that's OK.

@poettering poettering closed this Dec 5, 2016
@james-lawrence
Copy link
Author

haha, of course its okay. I will check out the new pr and give it a run.

@evverx
Copy link
Member

evverx commented Dec 5, 2016

havent had a chance to figure out how the mkosi stuff works in conjunction with the compilation process

Well, mkosi simplifies the manual testing. But it doesn't run any tests.

Ideally I want to run the tests inside a container

So, you should copy all tests to the container manually. There is no automatic way to do that.

@poettering
Copy link
Member

Well, mkosi simplifies the manual testing. But it doesn't run any tests.

We should fix that and run "make check" as part of the build...

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

Successfully merging this pull request may close these issues.

None yet

4 participants