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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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 ?  Could it be bundled into some script which is
already being run ?

If it can be bundled into a script somewhere, that would be better
because it avoids duplicating the information.

> diff --git 
> a/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in 
> b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
> new file mode 100644
> index 0000000..1855719
> --- /dev/null
> +++ b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
> @@ -0,0 +1,22 @@
> +[Unit]
> +Description=qemu for xen dom0 disk backend
> +Requires=proc-xen.mount var-lib-xenstored.mount xenstored.socket
> +After=xenstored.service xenconsoled.service
> +Before=xendomains.service libvirtd.service libvirt-guests.service
> +RefuseManualStop=true
> +ConditionPathIsDirectory=/proc/xen
...
> +ExecStart=@LIBEXEC@/qemu-system-i386 -xen-domid 0 \
> +     -xen-attach -name dom0 -nographic -M xenpv -daemonize \
> +     -monitor /dev/null -serial /dev/null -parallel /dev/null \
> +     -pidfile @XEN_RUN_DIR@/qemu-dom0.pid

IMO duplicating this command line isn't acceptable.  It is likely to
have to change in the future and there needs to be one copy of it in
the source tree.

> diff --git a/tools/hotplug/Linux/systemd/xen.conf.modules-load.d.in 
> b/tools/hotplug/Linux/systemd/xen.conf.modules-load.d.in
> new file mode 100644
> index 0000000..3fbd59b
> --- /dev/null
> +++ b/tools/hotplug/Linux/systemd/xen.conf.modules-load.d.in
> @@ -0,0 +1,16 @@
> +xen-evtchn
> +xen-gntdev
> +xen-gntalloc
> +xen-blkback
> +xen-netback
> +xen-pciback
> +evtchn
> +gntdev
> +netbk
> +blkbk
> +xen-scsibk
> +usbbk
> +pciback
> +xen-acpi-processor
> +blktap2
> +blktap

This duplicates the list of modules from one of the init scripts.
Again, I think we need a common list.

(In theory we shouldn't need manual module loading, but it seems that
failure to load module bugs continue to exist in Linux.)

> 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.

> diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in 
> b/tools/hotplug/Linux/systemd/xenstored.service.in
> new file mode 100644
> index 0000000..f64dfa1
> --- /dev/null
> +++ b/tools/hotplug/Linux/systemd/xenstored.service.in
> @@ -0,0 +1,25 @@
> +[Unit]
> +Description=Xenstored - daemon managing xenstore file system

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.

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

Thanks,
Ian.

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