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

["Luis R. Rodriguez" <mcgrof@xxxxxxxxxxxxxxxx>]

On Mon, May 12, 2014 at 7:11 AM, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> 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 ?  Could it be bundled into some script which is
> already being run ?

I don't see why, systemd wants to know exactly what it needs to be set
up and this is component based, mounts are one type of component,
stuffing it into a shell scripts hides an important system component.

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

One of the objectives of systemd is to help enable a different way to
think about initializing a system and scripts don't really contribute
any component information to the system that let systemd use to help
ensure things are done efficiently. Using scripts was what got us into
the position we are in today with the legacy init, doing away with
that requires components to be split up and for logic of requirements
to be dealt with through systemd, in this case service files.

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

Xen is already a complicated boat that tries to accommodate many
Operating Systems, supporting two different init systems for one
Operating System likely incurs a duplicate or sharing effort penalty
cost. I do agree trying to share is better for this case and will try
it out somehow in my next series.

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

Sure.

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

Once I'm done with systemd I'm going to jump on the kernel and would
like to know exactly which modules there is an issue for on this. Do
you have pointers?

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

Not sure what you mean. Systemd already knows of the PID file, this is
just to help forward port the old behavior and expectations. Are you
providing some hints as to what you wish for systemd to do ? If so
consider any users of the PID file.

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

Feel free to provide a better alternative description that suits
anyone's likings.

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

Sure that makes sense and this is pretty important information. I
consider this an area that could likely be enhanced upstream on
systemd in the future to help making this crystal clear as part of the
design of the system, but that's homework for later.

  Luis

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