Re: [PATCH v6 13/13] systemd: add xen systemd service and module files

["Luis R. Rodriguez" <mcgrof@do-not-panic.com>]

On Thu, Jun 19, 2014 at 12:47 PM, Bastian Blank <bastian@waldi.eu.org> wrote:
> On Thu, Jun 12, 2014 at 06:18:51PM -0700, Luis R. Rodriguez wrote:
>> This adds the systemd xen service / module files. All of this
>> is disabled for now, and won't install / build until we hook
>> up proper support for systemd into the build system.
>
> Why?

As I noted in my follow up the statement that says that this goes
disabled is not accurate, I folded in the autoconf support here so
ignore that part. I think I was asked to fold them together in
previous reviews and just forgot to update that paragraph.

>> --- /dev/null
>> +++ b/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
>> @@ -0,0 +1,13 @@
>> +[Unit]
>> +Description=mount xenstore file system
>> +ConditionVirtualization=xen
>
> Move the whole shit to /run, as anyone else does.

That's a separate atomic change from existing behavior and I'd prefer
that change be done separately.

>> --- /dev/null
>> +++ b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
>> @@ -0,0 +1,22 @@
>> +[Service]
>> +Type=simple
>> +EnvironmentFile=-/etc/default/xenstored
>> +EnvironmentFile=-/etc/sysconfig/xenstored
>> +PIDFile=@XEN_RUN_DIR@/qemu-dom0.pid
>> +ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
>
> This can fail and will move the unit into failed state.  Please show
> that this stuff does not make failed units on non-control domains.

This was in the legacy init scripts, as such its valid and should not
fail if this is dom0, for non xen dom0 builds the
ConditionVirtualization will prevent this unit from even being
considered and as such will not produced a failed unit.

>> +ExecStartPre=/bin/mkdir -p /var/run/xen
>
> This is referenced nowhere.
>
>> +ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS"
>> +ExecStartPost=-@BINDIR@/xenstore-write "/local/domain/0/name" "Domain-0"
>> +ExecStartPost=-@BINDIR@/xenstore-write "/local/domain/0/domid" 0
>
> Are you sure this is not racey?

As per the documentation ExecStartPost are executed one after the
other, serially [0]

[0] http://www.freedesktop.org/software/systemd/man/systemd.service.html

 Luis