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

[Jacek Konieczny <jajcus@jajcus.net>]

On 05/12/14 16:11, Ian Jackson wrote:
> Thanks for pursuing this.  I have some comments:
> 
> Luis R. Rodriguez writes ("[PATCH v4 13/15] systemd: add xen systemd service and module files"):
> ...
>> diff --git a/tools/hotplug/Linux/systemd/proc-xen.mount.in b/tools/hotplug/Linux/systemd/proc-xen.mount.in
>> new file mode 100644
>> index 0000000..6eb61b2
>> --- /dev/null
>> +++ b/tools/hotplug/Linux/systemd/proc-xen.mount.in
>> @@ -0,0 +1,9 @@
>> +[Unit]
>> +Description=Mount /proc/xen files
>> +ConditionPathIsDirectory=/proc/xen
>> +RefuseManualStop=true
>> +
>> +[Mount]
>> +What=xenfs
>> +Where=/proc/xen
>> +Type=xenfs
> 
> Blimey.  All of this to replace one line in a shell script.
> 
> Can you explain why this needs to be broken out into a separate
> systemd service ?

Because that is how mount points are configured in systemd.

One could drop /etc/fstab all together and use a shell script to mount
every file system. Would that be better?

In fact mounting /proc/xen from within a script instead of using fstab
entry is hack required because /etc/fstab cannot be reliably updated on
Xen install or activation.

The systemd mount units seem verbose, but they contain not much more
than an /etc/fstab line and allow more flexibility (conditions and
ordering).

> Could it be bundled into some script which is already being run ?

No. Scripts are bad way for configuring mounts. Doing it in the script
is the necessary evil in systems without drop-in configuration like
systemd provides.

>> diff --git a/tools/hotplug/Linux/systemd/xenconsoled.service.in b/tools/hotplug/Linux/systemd/xenconsoled.service.in
>> +[Service]
>> +Type=simple
>> +Environment=XENCONSOLED_ARGS=
>> +Environment=XENCONSOLED_LOG=none
>> +Environment=XENCONSOLED_LOG_DIR=@XEN_LOG_DIR@/console
>> +EnvironmentFile=-/etc/default/xenconsoled
>> +EnvironmentFile=-/etc/sysconfig/xenconsoled
>> +PIDFile=@XEN_RUN_DIR@/xenconsoled.pid
>> +ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
>> +ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@> +ExecStart=@SBINDIR@/xenconsoled --pid-file @XEN_RUN_DIR@/xenconsoled.pid --log=${XENCONSOLED_LOG} --log-dir=${XENCONSOLED_LOG_DIR} $XENCONSOLED_ARGS
> 
> This should use a non-forking startup protocol, rather than pid files.

I would say more: it seems weird to use both 'Type=simple' and
'PIDFile=' – PIDFile seems unnecessary here. sd_notify support
(Type=notify) would be the best option here, but it must be implemented
in the daemon.

> xenstore isn't a file system.  The /var/run area it uses is just
> something it needs for its private data.
> 
>> diff --git a/tools/hotplug/Linux/systemd/xenstored.socket.in b/tools/hotplug/Linux/systemd/xenstored.socket.in
>> +[Socket]
>> +ListenStream=/var/run/xenstored/socket
>> +ListenStream=/var/run/xenstored/socket_ro
> 
> AIUI the systemd socket activation protocol requires the systemd
> config and the daemon code/configuration to correspond.

The daemon may have no own configuration when the systemd-provided
sockets are used – the daemon does not need to know the paths, just that
the first socked is read-write, and the other is read-only.

> There should be a comment explaining where the corresponding
> configuration in xenstored is (ie referring to the code in both
> daemons listing the sockets).

Comment about the expected sockets would not hurt, I guess.

Jacek

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel