CP-39894: Move most of the daemon management code to the service module and make pid locations explicit

[psafont]

The PR is better reviewed commit-by-commit. The biggest change (10c0291) should only move around the code and only modify the callers' side.

Xenops and the daemons until now have used 2 ways of communicating the pid to the former: a xenstore key and optionally a file. We want to move to a model where we can use only a pidfile for some daemons (SWTPM, mainly).

Previously the path pidfile was implicit and the code to fetch it was shared among all the daemons meaning that even if the daemon always provided a pidfile, their clients always had to take into account the case where there was no file to process.

These changes force all the damon modules to declare the locations where the pidfile is expected to be, and only expose the pidfile in the way the daemon actually uses it.

Having all this logic in a separate module also help us to maintain an interface and not allow clients to access all the internal workings, facilitating future refactors (on how to wait for a daemon, for example)

vTPM suite: SR 170394 green
ring3 BVT+BST: SR170395 green

[lindig]

I'd use "removing" as cleaning implies something like flushing.

[lindig]

Is this guaranteed behavior? I understand it works with the kill(1) command but does this work with the system call as well?

[psafont]

The man page for kill(2) is missing from centos7 hosts (and CH), but on my dev machine seems to confirm this behaviour:

If sig is 0, then no signal is sent, but existence and permission checks are still performed; this can be used to check for the existence of a process ID or process group ID that the caller is permitted to signal.

[psafont]

Sorry, I missed a lot of commits here

[robhoes]

This generally looks good to me, and a useful exercise to consolidate the code for all of these mechanisms. A lot of churn though, so it needs careful testing, including upgrade scenarios with running VMs on the host.

[edwintorok]

Reviewing this PR on github is next to impossible: github won't show whether all the moved code is the same or not.
Locally with git I think it is possible with these git config settings:

[diff]
    algorithm = histogram
    colorMoved = dimmed_zebra
    renames = copies

Then identically moved lines will be kind of greyish (i.e. easy to visually filter out on a black background as they're barely visible, but still there for context if needed).

And then use something like this (after having added paus as a remote and fetched it), ignoring whitespace is a must otherwise indentation changes won't detect the moved code as identical:
git log --reverse --color-moved-ws=ignore-all-space -w -p origin/feature/vtpm..psafont/private/paus/pid-wait

[edwintorok]

I think some of the code moves got mixed together with also reformatting the moved code, e.g. git shows the vnc port path and pid path lines as having changed even if they're functionally the same.

Not sure whether we want all the arg building code moved out of device, that seems to belong there (it tells you how to launch the device, and is specific to it, e.g. varstored and qemu) and results in a lot of code churn.

I think we may be overcomplicating service launch detection here: the usual way a Unix process signals that it is "ready" is to open all sockets and everything it needs, fork off a child to run the actual daemon and then exit the parent, and this exit is detected by systemd to transition it from starting to started state. In more complex situations sd_notify can be used (though read the manpage there is also an sd_notify_barrier to avoid race conditions with the former, because systemd may just ignore the former completely and still end up with launch race conditions).
Also looking at the various places in the code that create the pidfile or the xenstore entry they might be created too soon (i.e. the parent can still fail for other reasons, e.g. wrong cmdline args).

I haven't checked whether all our processes fork off a child, but perhaps that'd be a lot simpler way: no need to wait for xenstore or a pidfile, just wait for the forked process from forkexecd to exit successfully, which already has a function and would be a single mechanism instead of two. We could also delete quite a lot of this code then.

If not all processes would support this "forking" detection method of launch we could perhaps add a few small patches implementing that to the daemons, which overall would be less complex code to maintain. (There might be a performance justification for avoiding the extra fork, especially in a PV Dom0 forking is expensive as it triggers a lot page map copies, but I don't think the added complexity here would justify the tiny perf advantage we might have by avoiding the fork, setting up the inotify is probably not exactly free either.)
Although of course introducing a fork into a code that never forked before can also lead to bugs, e.g. if you initialized some state in the parent, and you forked that state may not necessarily be valid in the child (especialy if it is related to threads, you can't fork if you already created threads, etc.)

[edwintorok]

I can't spot anything particularly wrong with the changes here though, but it'll be difficult to merge this to master without also merging the vTPM code itself (since it depends on it).

[edwintorok]

We should probably merge the xenopsd/ changes of feature/vtpm to master fairly soon, we've diverged quite a lot and if xenopsd changes on master we'll have lots of conflicts to solve. The swtpm daemon is not yet part of rt-next and if we don't merge the xapi side of the code this code should be mostly inactive, so we should just do some regression testing on a branch with ocaml/xenopsd commits cherry-picked.

[psafont]

We should probably merge the xenopsd/ changes of feature/vtpm to master fairly soon

We'll probably need some retouches around the interface for this, or be conservative about the changes brought into master