[PATCH 0/5] tools/hotplug: systemd changes for 4.5

[PATCH 0/5] tools/hotplug: systemd changes for 4.5

[Olaf Hering]

Konrad and Michael Young reported SELinux related failures in
var-lib-xenstored.mount. The first patch tries to address this by
makeing it easier to change the value of XENSTORED_MOUNT_CTX.

Its not clear to me if the mount option "context=" should be
adjustable by configure --with-selinux-mount-context=VAL to simplify
building via "make rpmball" for example. Looks like every new make
install or "rpm -U --force dist/xen.rpm" would require a readjustment of
XENSTORED_MOUNT_CTX without such new configure option.

The remaining patches are a result of reviewing the service files.
They reference non-existant sysconfig files. We should fix this before
the release of 4.5 to avoid stale sysconfig files if someone wants to
adjust the values.

I have tested this series on openSUSE 13.1.

Please review and apply for 4.5.


Olaf

Olaf Hering (5):
  tools/hotplug: move XENSTORED_MOUNT_CTX to sysconfig.xencommons
  tools/hotplug: use existing sysconfig file for xenconsoled
  tools/hotplug: remove EnvironmentFile from
    xen-qemu-dom0-disk-backend.service
  tools/hotplug: remove XENSTORED_ROOTDIR from service file
  tools/hotplug: support XENSTORED_TRACE in systemd

 tools/hotplug/Linux/init.d/sysconfig.xencommons.in | 24 ++++++++++++++++++++--
 tools/hotplug/Linux/init.d/xencommons.in           |  5 +++--
 .../Linux/systemd/var-lib-xenstored.mount.in       |  3 +--
 .../systemd/xen-qemu-dom0-disk-backend.service.in  |  1 -
 tools/hotplug/Linux/systemd/xenconsoled.service.in |  7 ++-----
 tools/hotplug/Linux/systemd/xenstored.service.in   |  3 +--
 6 files changed, 29 insertions(+), 14 deletions(-)

[PATCH 1/5] tools/hotplug: move XENSTORED_MOUNT_CTX to sysconfig.xencommons

[Olaf Hering]

On a non-SELinux system the mount option "context=none" works fine. But
with SELinux enabled a proper value has to be defined. To simplify the
required adjustment move XENSTORED_MOUNT_CTX from the service file to
the sysconfig file.

There is no need to require the creation of a new sysconfig file, just
reuse the existing /etc/sysconfig/xencommons file.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/Linux/init.d/sysconfig.xencommons.in     | 7 +++++++
 tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in | 3 +--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
index c12fc8a..3a34b33 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
+++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
@@ -40,3 +40,10 @@
 
 # qemu path
 #QEMU_XEN=@LIBEXEC_BIN@/qemu-system-i386
+
+## Type: string
+## Default: "none"
+#
+# SELinux context for @XEN_LIB_STORED@ mount point.
+# see mount(8) for the meaning of the "context=" option
+XENSTORED_MOUNT_CTX=none
diff --git a/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in b/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
index d5e04db..65e0b79 100644
--- a/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
+++ b/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
@@ -6,8 +6,7 @@ ConditionPathExists=/proc/xen/capabilities
 RefuseManualStop=true
 
 [Mount]
-Environment=XENSTORED_MOUNT_CTX=none
-EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xenstored
+EnvironmentFile=@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
 What=xenstore
 Where=@XEN_LIB_STORED@
 Type=tmpfs

Re: [PATCH 1/5] tools/hotplug: move XENSTORED_MOUNT_CTX to sysconfig.xencommons

[Ian Jackson]

Olaf Hering writes ("[PATCH 1/5] tools/hotplug: move XENSTORED_MOUNT_CTX to sysconfig.xencommons"):
> On a non-SELinux system the mount option "context=none" works fine. But
> with SELinux enabled a proper value has to be defined. To simplify the
> required adjustment move XENSTORED_MOUNT_CTX from the service file to
> the sysconfig file.

This patch looks like just the hook.  It seems to be missing the part
where the actual selinux context is defined and plumbed through.


> There is no need to require the creation of a new sysconfig file, just
> reuse the existing /etc/sysconfig/xencommons file.

This seems to be an unrelated change ?  If not I confess I don't see
the connection.

> --- a/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
> +++ b/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
...
>  [Mount]
> -Environment=XENSTORED_MOUNT_CTX=none
> -EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xenstored
> +EnvironmentFile=@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons

And won't this break existing systems which have an
/etc/{default,sysconfig}/xenstored ?

Ian.

Re: [PATCH 1/5] tools/hotplug: move XENSTORED_MOUNT_CTX to sysconfig.xencommons

[Olaf Hering]

On Fri, Dec 05, Ian Jackson wrote:

> Olaf Hering writes ("[PATCH 1/5] tools/hotplug: move XENSTORED_MOUNT_CTX to sysconfig.xencommons"):
> > On a non-SELinux system the mount option "context=none" works fine. But
> > with SELinux enabled a proper value has to be defined. To simplify the
> > required adjustment move XENSTORED_MOUNT_CTX from the service file to
> > the sysconfig file.
> 
> This patch looks like just the hook.  It seems to be missing the part
> where the actual selinux context is defined and plumbed through.

The context in xen source is "none". As asked in the cover letter (which
unfortunately got send to just Konrad and xen-devel, no idea how to fix
that) a configure --with-something may be the way to inject it into the
sources, if required.

> > There is no need to require the creation of a new sysconfig file, just
> > reuse the existing /etc/sysconfig/xencommons file.
> 
> This seems to be an unrelated change ?  If not I confess I don't see
> the connection.

The context has to be defined somewhere. And that place is
sysconfig/xencommons.

> > --- a/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
> > +++ b/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
> ...
> >  [Mount]
> > -Environment=XENSTORED_MOUNT_CTX=none
> > -EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xenstored
> > +EnvironmentFile=@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
> 
> And won't this break existing systems which have an
> /etc/{default,sysconfig}/xenstored ?

Which systems would that be? That file is new in 4.5.

Olaf

Re: [PATCH 1/5] tools/hotplug: move XENSTORED_MOUNT_CTX to sysconfig.xencommons

[Olaf Hering]

On Fri, Dec 05, Olaf Hering wrote:

> On Fri, Dec 05, Ian Jackson wrote:
> > And won't this break existing systems which have an
> > /etc/{default,sysconfig}/xenstored ?
> Which systems would that be? That file is new in 4.5.

... Not the file itself but the usage of a to-be-created file ...

Olaf

Re: [PATCH 1/5] tools/hotplug: move XENSTORED_MOUNT_CTX to sysconfig.xencommons

[Ian Jackson]

Olaf Hering writes ("Re: [PATCH 1/5] tools/hotplug: move XENSTORED_MOUNT_CTX to sysconfig.xencommons"):
> On Fri, Dec 05, Ian Jackson wrote:
> > This patch looks like just the hook.  It seems to be missing the part
> > where the actual selinux context is defined and plumbed through.
> 
> The context in xen source is "none". As asked in the cover letter (which
> unfortunately got send to just Konrad and xen-devel, no idea how to fix
> that) a configure --with-something may be the way to inject it into the
> sources, if required.

I confess I don't know very much about selinux, but shouldn't we be
providing a reasonable default policy, rather than leaving it to the
distro or user to pass special options to configure ?  Or are things
in the selinux world so fragmented or fast-moving that such a generic
policy couldn't be written ?

> > > There is no need to require the creation of a new sysconfig file, just
> > > reuse the existing /etc/sysconfig/xencommons file.
> > 
> > This seems to be an unrelated change ?  If not I confess I don't see
> > the connection.
> 
> The context has to be defined somewhere. And that place is
> sysconfig/xencommons.

Oh, I see.  I think you should do this change as a pre-patch, along
with the abolition of
  /etc/{default,sysconfig}/{xenconsoled,xenstored}

Your patch 2/5 involving xenconsoled has a mixture of code motion and
other semantic changes, which makes it hard to review.

> > And won't this break existing systems which have an
> > /etc/{default,sysconfig}/xenstored ?
> 
> Which systems would that be? That file is new in 4.5.

Oh, good.  In that case we should abolish these ASAP - before 4.5.

Thanks,
Ian.

Re: [PATCH 1/5] tools/hotplug: move XENSTORED_MOUNT_CTX to sysconfig.xencommons

[Olaf Hering]

On Fri, Dec 05, Ian Jackson wrote:

> Olaf Hering writes ("Re: [PATCH 1/5] tools/hotplug: move XENSTORED_MOUNT_CTX to sysconfig.xencommons"):
> > On Fri, Dec 05, Ian Jackson wrote:
> > > This patch looks like just the hook.  It seems to be missing the part
> > > where the actual selinux context is defined and plumbed through.
> > 
> > The context in xen source is "none". As asked in the cover letter (which
> > unfortunately got send to just Konrad and xen-devel, no idea how to fix
> > that) a configure --with-something may be the way to inject it into the
> > sources, if required.
> 
> I confess I don't know very much about selinux, but shouldn't we be
> providing a reasonable default policy, rather than leaving it to the
> distro or user to pass special options to configure ?  Or are things
> in the selinux world so fragmented or fast-moving that such a generic
> policy couldn't be written ?

I know nothing about SELinux.  Not sure why a context= is required
anyway.  But I can find out next week if noone else has an idea how to
deal with SELinux.

Olaf

Re: [PATCH 1/5] tools/hotplug: move XENSTORED_MOUNT_CTX to sysconfig.xencommons

[Ian Jackson]

Olaf Hering writes ("Re: [PATCH 1/5] tools/hotplug: move XENSTORED_MOUNT_CTX to sysconfig.xencommons"):
> On Fri, Dec 05, Ian Jackson wrote:
> > I confess I don't know very much about selinux, but shouldn't we be
> > providing a reasonable default policy, rather than leaving it to the
> > distro or user to pass special options to configure ?  Or are things
> > in the selinux world so fragmented or fast-moving that such a generic
> > policy couldn't be written ?
> 
> I know nothing about SELinux.  Not sure why a context= is required
> anyway.  But I can find out next week if noone else has an idea how to
> deal with SELinux.

OK, thanks.

Anyway, I don't think this question should stand in the way of this
hunk of your patch, which is IMO obviously a move in the right
direction.

So if you shuffle things about as I suggested I will ack this hunk in
your next version of the series.

Thanks,
Ian.

Re: [PATCH 1/5] tools/hotplug: move XENSTORED_MOUNT_CTX to sysconfig.xencommons

[Anthony PERARD]

On Fri, Dec 05, 2014 at 01:05:48PM +0100, Olaf Hering wrote:
> On a non-SELinux system the mount option "context=none" works fine.

That's not true. I've tested 'context=none' on ArchLinux which have no
support (or limited) for selinux, and the option give this error:
"tmpfs: Bad mount option context"

-- 
Anthony PERARD

Re: [PATCH 1/5] tools/hotplug: move XENSTORED_MOUNT_CTX to sysconfig.xencommons

[Olaf Hering]

On Fri, Dec 05, Anthony PERARD wrote:

> On Fri, Dec 05, 2014 at 01:05:48PM +0100, Olaf Hering wrote:
> > On a non-SELinux system the mount option "context=none" works fine.
> 
> That's not true. I've tested 'context=none' on ArchLinux which have no
> support (or limited) for selinux, and the option give this error:
> "tmpfs: Bad mount option context"

Appears to work for me, at least on SLE12. No idea how much SELinux they
put in there. My old 11.4 behaves the same. Perhaps your kernel lacks
certain functionality? I assome the error msg comes from the kernel.

root@bax:~ # mount -vt tmpfs xxx -o context=foo /mnt/
mount: xxx mounted on /mnt.
root@bax:~ # grep mnt /proc/mounts 
xxx /mnt tmpfs rw,relatime 0 0

Olaf

Re: [PATCH 1/5] tools/hotplug: move XENSTORED_MOUNT_CTX to sysconfig.xencommons

[Anthony PERARD]

On Fri, Dec 05, 2014 at 04:51:49PM +0100, Olaf Hering wrote:
> On Fri, Dec 05, Anthony PERARD wrote:
> 
> > On Fri, Dec 05, 2014 at 01:05:48PM +0100, Olaf Hering wrote:
> > > On a non-SELinux system the mount option "context=none" works fine.
> > 
> > That's not true. I've tested 'context=none' on ArchLinux which have no
> > support (or limited) for selinux, and the option give this error:
> > "tmpfs: Bad mount option context"
> 
> Appears to work for me, at least on SLE12. No idea how much SELinux they
> put in there. My old 11.4 behaves the same. Perhaps your kernel lacks
> certain functionality? I assome the error msg comes from the kernel.

Yes, the message comes from the kernel. I don't know what functionality
is needed to for the kernel to handle context=none, so I can't tell.
At least, there is not CONFIG_SECURITY_SELINUX in the config, but
CONFIG_SECURITY=y is set.

In anyway, I'll continue to edit the systemd unit to remove the context
option, that's not a big deal.

> root@bax:~ # mount -vt tmpfs xxx -o context=foo /mnt/
> mount: xxx mounted on /mnt.

$ sudo mount -vt tmpfs xxx -o context=foo /tmp/tmp.lhw79USQUe 
mount: wrong fs type, bad option, bad superblock on xxx,
       missing codepage or helper program, or other error

       In some cases useful info is found in syslog - try
       dmesg | tail or so.
$ dmesg | tail -1
[1569927.987083] tmpfs: Bad mount option context

-- 
Anthony PERARD

[PATCH 2/5] tools/hotplug: use existing sysconfig file for xenconsoled

[Olaf Hering]

There is no need to require the creation of a new sysconfig file to
pass options to xenconsoled in the systemd service file. Reuse the
existing xencommons file. This file already contains the variable
XENCONSOLED_TRACE, which is used in the sysv runlevel script.

- Adjust systemd service file to use XENCONSOLED_TRACE instead of
  XENCONSOLED_LOG
- Move XENCONSOLED_ARGS and XENCONSOLED_LOG_DIR to the sysconfig file.
- Enable XENCONSOLED_TRACE and set its value to "none" to have a value
  for --log in the service file.
- Adjust the runlevel script to recognize also XENCONSOLED_ARGS and
  XENCONSOLED_LOG_DIR
- Adjust the runlevel script to handle XENCONSOLED_TRACE properly. If
  an old sysconfig file exist the XENCONSOLED_TRACE will remain empty.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/Linux/init.d/sysconfig.xencommons.in | 17 +++++++++++++++--
 tools/hotplug/Linux/init.d/xencommons.in           |  5 +++--
 tools/hotplug/Linux/systemd/xenconsoled.service.in |  7 ++-----
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
index 3a34b33..6271c3e 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
+++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
@@ -2,8 +2,21 @@
 ## Type: string
 ## Default: "none"
 #
-# Log xenconsoled messages (cf xl dmesg)
-#XENCONSOLED_TRACE=[none|guest|hv|all]
+# Log xenconsoled messages (cf xl dmesg
+# This can be [none|guest|hv|all]
+XENCONSOLED_TRACE=none
+
+## Type: string
+## Default: ""
+#
+# Additional command line arguments for xenconsoled
+XENCONSOLED_ARGS=
+
+## Type: string
+## Default: "@XEN_LOG_DIR@/console"
+#
+# Output directory for xenconsoled logfiles.
+XENCONSOLED_LOG_DIR=@XEN_LOG_DIR@/console
 
 ## Type: string
 ## Default: xenstored
diff --git a/tools/hotplug/Linux/init.d/xencommons.in b/tools/hotplug/Linux/init.d/xencommons.in
index a1095c2..ddc8daa 100644
--- a/tools/hotplug/Linux/init.d/xencommons.in
+++ b/tools/hotplug/Linux/init.d/xencommons.in
@@ -95,8 +95,9 @@ do_start () {
 	fi
 
 	echo Starting xenconsoled...
-	test -z "$XENCONSOLED_TRACE" || XENCONSOLED_ARGS=" --log=$XENCONSOLED_TRACE"
-	${SBINDIR}/xenconsoled --pid-file=$XENCONSOLED_PIDFILE $XENCONSOLED_ARGS
+	test -z "$XENCONSOLED_LOG_DIR" || XENCONSOLED_LOG_DIR="--log-dir=${XENCONSOLED_LOG_DIR}"
+	test -z "$XENCONSOLED_TRACE" || XENCONSOLED_TRACE=" --log=$XENCONSOLED_TRACE"
+	${SBINDIR}/xenconsoled --pid-file=$XENCONSOLED_PIDFILE ${XENCONSOLED_LOG_DIR} ${XENCONSOLED_TRACE} $XENCONSOLED_ARGS
 	echo Starting QEMU as disk backend for dom0
 	test -z "$QEMU_XEN" && QEMU_XEN="${LIBEXEC_BIN}/qemu-system-i386"
 	$QEMU_XEN -xen-domid 0 -xen-attach -name dom0 -nographic -M xenpv -daemonize \
diff --git a/tools/hotplug/Linux/systemd/xenconsoled.service.in b/tools/hotplug/Linux/systemd/xenconsoled.service.in
index cb44cd6..9f533ff 100644
--- a/tools/hotplug/Linux/systemd/xenconsoled.service.in
+++ b/tools/hotplug/Linux/systemd/xenconsoled.service.in
@@ -6,14 +6,11 @@ ConditionPathExists=/proc/xen/capabilities
 
 [Service]
 Type=simple
-Environment=XENCONSOLED_ARGS=
-Environment=XENCONSOLED_LOG=none
-Environment=XENCONSOLED_LOG_DIR=@XEN_LOG_DIR@/console
-EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xenconsoled
+EnvironmentFile=@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
 PIDFile=@XEN_RUN_DIR@/xenconsoled.pid
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
 ExecStartPre=/bin/mkdir -p ${XENCONSOLED_LOG_DIR}
-ExecStart=@sbindir@/xenconsoled --pid-file @XEN_RUN_DIR@/xenconsoled.pid --log=${XENCONSOLED_LOG} --log-dir=${XENCONSOLED_LOG_DIR} $XENCONSOLED_ARGS
+ExecStart=@sbindir@/xenconsoled --pid-file @XEN_RUN_DIR@/xenconsoled.pid --log=${XENCONSOLED_TRACE} --log-dir=${XENCONSOLED_LOG_DIR} $XENCONSOLED_ARGS
 
 [Install]
 WantedBy=multi-user.target

[PATCH 3/5] tools/hotplug: remove EnvironmentFile from xen-qemu-dom0-disk-backend.service

[Olaf Hering]

The references Environment file does not exist, and the service file
does not make use of variables anyway.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in | 1 -
 1 file changed, 1 deletion(-)

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
index 0a5807a..274cec0 100644
--- a/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
+++ b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
@@ -8,7 +8,6 @@ ConditionPathExists=/proc/xen/capabilities
 
 [Service]
 Type=simple
-EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xenstored
 PIDFile=@XEN_RUN_DIR@/qemu-dom0.pid
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
 ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@

[PATCH 4/5] tools/hotplug: remove XENSTORED_ROOTDIR from service file

[Olaf Hering]

There is no need to export XENSTORED_ROOTDIR. This variable can be
enabled in sysconfig/xencommons. If the variable is unset xenstored
will automatically use @XEN_LIB_STORED@.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/Linux/systemd/xenstored.service.in | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
index 780bdd6..0f0ac58 100644
--- a/tools/hotplug/Linux/systemd/xenstored.service.in
+++ b/tools/hotplug/Linux/systemd/xenstored.service.in
@@ -9,7 +9,6 @@ ConditionPathExists=/proc/xen/capabilities
 [Service]
 Type=notify
 Environment=XENSTORED_ARGS=
-Environment=XENSTORED_ROOTDIR=@XEN_LIB_STORED@
 Environment=XENSTORED=@XENSTORED@
 EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities

Re: [PATCH 4/5] tools/hotplug: remove XENSTORED_ROOTDIR from service file

[Ian Jackson]

Olaf Hering writes ("[PATCH 4/5] tools/hotplug: remove XENSTORED_ROOTDIR from service file"):
> There is no need to export XENSTORED_ROOTDIR. This variable can be
> enabled in sysconfig/xencommons. If the variable is unset xenstored
> will automatically use @XEN_LIB_STORED@.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Olaf Hering]

The sysv runlevel script handles the boolean variable XENSTORED_TRACE
from sysconfig.xencommons to enable tracing. Recognize this also to
the systemd service file.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/Linux/systemd/xenstored.service.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
index 0f0ac58..7e55f4f 100644
--- a/tools/hotplug/Linux/systemd/xenstored.service.in
+++ b/tools/hotplug/Linux/systemd/xenstored.service.in
@@ -14,7 +14,7 @@ EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
 ExecStartPre=-/bin/rm -f @XEN_LIB_STORED@/tdb*
 ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@
-ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS"
+ExecStart=/bin/sh -c 'if test -n "${XENSTORED_TRACE}" ; then XENSTORED_ARGS="-T /var/log/xen/xenstored-trace.log" ; fi ; exec $XENSTORED --no-fork $$XENSTORED_ARGS'
 
 [Install]
 WantedBy=multi-user.target

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Ian Jackson]

Olaf Hering writes ("[PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd"):
> The sysv runlevel script handles the boolean variable XENSTORED_TRACE
> from sysconfig.xencommons to enable tracing. Recognize this also to
> the systemd service file.
...
> -ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS"
> +ExecStart=/bin/sh -c 'if test -n "${XENSTORED_TRACE}" ; then XENSTORED_ARGS="-T /var/log/xen/xenstored-trace.log" ; fi ; exec $XENSTORED --no-fork $$XENSTORED_ARGS'

I'm afraid I'm not happy with the way that this duplicates logic
already found in /etc/init.d/xencommons.

Nacked-by: Ian Jackson <ian.jackson@eu.citrix.com>

I think the only way to make this work properly is to factor the
necessary parts out of init.d/xencommons into a new script which can
be used by both xencommons and systemd.  I'm not sure such a patch
would be appropriate for 4.5 at this stage.

Ian.

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Olaf Hering]

On Fri, Dec 05, Ian Jackson wrote:

> I think the only way to make this work properly is to factor the
> necessary parts out of init.d/xencommons into a new script which can
> be used by both xencommons and systemd.  I'm not sure such a patch
> would be appropriate for 4.5 at this stage.

Yes, a helper script to launch just xenstored would help. But which part
would do the final "exec"? Perhaps the sysv script has to fork a shell
like its done above. I will have a look at this. 

Are you opposed to the idea to support XENSTORED_TRACE for systemd right
in 4.5.0?

Olaf

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Ian Jackson]

Olaf Hering writes ("Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd"):
> On Fri, Dec 05, Ian Jackson wrote:
> > I think the only way to make this work properly is to factor the
> > necessary parts out of init.d/xencommons into a new script which can
> > be used by both xencommons and systemd.  I'm not sure such a patch
> > would be appropriate for 4.5 at this stage.
> 
> Yes, a helper script to launch just xenstored would help. But which part
> would do the final "exec"? Perhaps the sysv script has to fork a shell
> like its done above. I will have a look at this. 

If there's no other way to do it, you could have the helper script
take an argument (or a named (environment) parameter) to discover
whether to call exec.

> Are you opposed to the idea to support XENSTORED_TRACE for systemd right
> in 4.5.0?

Ideally I would like to support XENSTORED_TRACE for systemd in 4.5.0.

But I do not want to duplicate the functionality at all.  systemd
seems to make it difficult to support XENSTORED_TRACE without either
duplicating functionality or refactoring the existing init.d script.
(Indeed the very fact that XENSTORED_TRACE does not work with systemd
right now is due to the systemd startup of xenstored being decoupled
from the init script code which handles XENSTORED_TRACE.  I seem to
remember making some comments about this kind of thing at the time...)

And I am currently unconvinced that refactoring things at this stage
of the 4.5 release is appropriate.  But others may have a different
view.

Can systemd not launch these daemons by running the existing
xencommons et al init scripts ?  Obviously that won't give you all of
systemd's shiny features but IMO it ought to work.

Ian.

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Olaf Hering]

On Fri, Dec 05, Ian Jackson wrote:

> Can systemd not launch these daemons by running the existing
> xencommons et al init scripts ?  Obviously that won't give you all of
> systemd's shiny features but IMO it ought to work.

I think the point was to let systemd pass the file descriptors. Thats why
the service file does the "exec xenstored".

Olaf

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Olaf Hering]

On Fri, Dec 05, Ian Jackson wrote:

> I think the only way to make this work properly is to factor the
> necessary parts out of init.d/xencommons into a new script which can
> be used by both xencommons and systemd.  I'm not sure such a patch
> would be appropriate for 4.5 at this stage.

I came up with this, it appears to work in my testing. Will do more
testing later today.

Olaf
---
 .gitignore                                       |  1 +
 tools/configure                                  |  3 +-
 tools/configure.ac                               |  1 +
 tools/hotplug/Linux/Makefile                     |  2 ++
 tools/hotplug/Linux/init.d/xencommons.in         | 24 +------------
 tools/hotplug/Linux/systemd/xenstored.service.in |  7 +---
 tools/hotplug/Linux/xenstored.sh.in              | 44 ++++++++++++++++++++++++
 7 files changed, 52 insertions(+), 30 deletions(-)
 create mode 100644 tools/hotplug/Linux/xenstored.sh.in

diff --git a/.gitignore b/.gitignore
index 8c8c06f..7e6884a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -153,6 +153,7 @@ tools/hotplug/Linux/vif-setup
 tools/hotplug/Linux/xen-backend.rules
 tools/hotplug/Linux/xen-hotplug-common.sh
 tools/hotplug/Linux/xendomains
+tools/hotplug/Linux/xenstored.sh
 tools/hotplug/NetBSD/rc.d/xencommons
 tools/include/xen/*
 tools/include/xen-foreign/*.(c|h|size)
diff --git a/tools/configure b/tools/configure
index b0aea0a..e72876c 100755
--- a/tools/configure
+++ b/tools/configure
@@ -2276,7 +2276,7 @@ ac_compiler_gnu=$ac_cv_c_compiler_gnu
 
 
 
-ac_config_files="$ac_config_files ../config/Tools.mk hotplug/FreeBSD/rc.d/xencommons hotplug/Linux/init.d/sysconfig.xencommons hotplug/Linux/init.d/xen-watchdog hotplug/Linux/init.d/xencommons hotplug/Linux/init.d/xendomains hotplug/Linux/systemd/proc-xen.mount hotplug/Linux/systemd/var-lib-xenstored.mount hotplug/Linux/systemd/xen-init-dom0.service hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service hotplug/Linux/systemd/xen-watchdog.service hotplug/Linux/systemd/xenconsoled.service hotplug/Linux/systemd/xendomains.service hotplug/Linux/systemd/xenstored.service hotplug/Linux/systemd/xenstored.socket hotplug/Linux/systemd/xenstored_ro.socket hotplug/Linux/vif-setup hotplug/Linux/xen-backend.rules hotplug/Linux/xen-hotplug-common.sh hotplug/Linux/xendomains hotplug/NetBSD/rc.d/xencom
 mons"
+ac_config_files="$ac_config_files ../config/Tools.mk hotplug/FreeBSD/rc.d/xencommons hotplug/Linux/init.d/sysconfig.xencommons hotplug/Linux/init.d/xen-watchdog hotplug/Linux/init.d/xencommons hotplug/Linux/init.d/xendomains hotplug/Linux/systemd/proc-xen.mount hotplug/Linux/systemd/var-lib-xenstored.mount hotplug/Linux/systemd/xen-init-dom0.service hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service hotplug/Linux/systemd/xen-watchdog.service hotplug/Linux/systemd/xenconsoled.service hotplug/Linux/systemd/xendomains.service hotplug/Linux/systemd/xenstored.service hotplug/Linux/systemd/xenstored.socket hotplug/Linux/systemd/xenstored_ro.socket hotplug/Linux/vif-setup hotplug/Linux/xen-backend.rules hotplug/Linux/xen-hotplug-common.sh hotplug/Linux/xendomains hotplug/Linux/xenstored.sh
  hotplug/NetBSD/rc.d/xencommons"
 
 ac_config_headers="$ac_config_headers config.h"
 
@@ -9585,6 +9585,7 @@ do
     "hotplug/Linux/xen-backend.rules") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/xen-backend.rules" ;;
     "hotplug/Linux/xen-hotplug-common.sh") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/xen-hotplug-common.sh" ;;
     "hotplug/Linux/xendomains") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/xendomains" ;;
+    "hotplug/Linux/xenstored.sh") CONFIG_FILES="$CONFIG_FILES hotplug/Linux/xenstored.sh" ;;
     "hotplug/NetBSD/rc.d/xencommons") CONFIG_FILES="$CONFIG_FILES hotplug/NetBSD/rc.d/xencommons" ;;
     "config.h") CONFIG_HEADERS="$CONFIG_HEADERS config.h" ;;
 
diff --git a/tools/configure.ac b/tools/configure.ac
index 1ac63a3..8f198e8 100644
--- a/tools/configure.ac
+++ b/tools/configure.ac
@@ -26,6 +26,7 @@ hotplug/Linux/vif-setup
 hotplug/Linux/xen-backend.rules
 hotplug/Linux/xen-hotplug-common.sh
 hotplug/Linux/xendomains
+hotplug/Linux/xenstored.sh
 hotplug/NetBSD/rc.d/xencommons
 ])
 AC_CONFIG_HEADERS([config.h])
diff --git a/tools/hotplug/Linux/Makefile b/tools/hotplug/Linux/Makefile
index 1706c05..e9a1ef0 100644
--- a/tools/hotplug/Linux/Makefile
+++ b/tools/hotplug/Linux/Makefile
@@ -2,6 +2,7 @@ XEN_ROOT = $(CURDIR)/../../..
 include $(XEN_ROOT)/tools/Rules.mk
 
 # Init scripts.
+XENSTORED_LIBEXEC = xenstored.sh
 XENDOMAINS_INITD = init.d/xendomains
 XENDOMAINS_LIBEXEC = xendomains
 XENDOMAINS_SYSCONFIG = init.d/sysconfig.xendomains
@@ -51,6 +52,7 @@ install-initd:
 	[ -d $(DESTDIR)$(INITD_DIR) ] || $(INSTALL_DIR) $(DESTDIR)$(INITD_DIR)
 	[ -d $(DESTDIR)$(SYSCONFIG_DIR) ] || $(INSTALL_DIR) $(DESTDIR)$(SYSCONFIG_DIR)
 	[ -d $(DESTDIR)$(LIBEXEC_BIN) ] || $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN)
+	$(INSTALL_PROG) $(XENSTORED_LIBEXEC) $(DESTDIR)$(LIBEXEC_BIN)
 	$(INSTALL_PROG) $(XENDOMAINS_LIBEXEC) $(DESTDIR)$(LIBEXEC_BIN)
 	$(INSTALL_PROG) $(XENDOMAINS_INITD) $(DESTDIR)$(INITD_DIR)
 	$(INSTALL_DATA) $(XENDOMAINS_SYSCONFIG) $(DESTDIR)$(SYSCONFIG_DIR)/xendomains
diff --git a/tools/hotplug/Linux/init.d/xencommons.in b/tools/hotplug/Linux/init.d/xencommons.in
index a1095c2..d03dd59 100644
--- a/tools/hotplug/Linux/init.d/xencommons.in
+++ b/tools/hotplug/Linux/init.d/xencommons.in
@@ -18,7 +18,6 @@
 # Description:       Starts and stops the daemons neeeded for xl/xend
 ### END INIT INFO
 
-XENSTORED=@XENSTORED@
 BACKEND_MODULES="@LINUX_BACKEND_MODULES@"
 
 . @XEN_SCRIPT_DIR@/hotplugpath.sh
@@ -64,28 +63,7 @@ do_start () {
 
 	if ! `${BINDIR}/xenstore-read -s / >/dev/null 2>&1`
 	then
-		test -z "$XENSTORED_ROOTDIR" && XENSTORED_ROOTDIR="@XEN_LIB_STORED@"
-		rm -f "$XENSTORED_ROOTDIR"/tdb* &>/dev/null
-		test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log"
-
-		if [ -n "$XENSTORED" ] ; then
-		    echo -n Starting $XENSTORED...
-		    $XENSTORED --pid-file /var/run/xenstored.pid $XENSTORED_ARGS
-		else
-		    echo "No xenstored found"
-		    exit 1
-		fi
-
-		# Wait for xenstored to actually come up, timing out after 30 seconds
-                while [ $time -lt $timeout ] && ! `${BINDIR}/xenstore-read -s / >/dev/null 2>&1` ; do
-                    echo -n .
-		    time=$(($time+1))
-                    sleep 1
-                done
-		echo
-
-		# Exit if we timed out
-		if ! [ $time -lt $timeout ] ; then
+		if ! ${LIBEXEC_BIN}/xenstored.sh --opt --pid-file --opt /var/run/xenstored.pid ; then
 		    echo Could not start xenstored
 		    exit 1
 		fi
diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
index 0f0ac58..01f5726 100644
--- a/tools/hotplug/Linux/systemd/xenstored.service.in
+++ b/tools/hotplug/Linux/systemd/xenstored.service.in
@@ -8,13 +8,8 @@ ConditionPathExists=/proc/xen/capabilities
 
 [Service]
 Type=notify
-Environment=XENSTORED_ARGS=
-Environment=XENSTORED=@XENSTORED@
-EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
-ExecStartPre=-/bin/rm -f @XEN_LIB_STORED@/tdb*
-ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@
-ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS"
+ExecStart=-@LIBEXEC_BIN@/xenstored.sh --exec --opt "--no-fork"
 
 [Install]
 WantedBy=multi-user.target
diff --git a/tools/hotplug/Linux/xenstored.sh.in b/tools/hotplug/Linux/xenstored.sh.in
new file mode 100644
index 0000000..11caf25
--- /dev/null
+++ b/tools/hotplug/Linux/xenstored.sh.in
@@ -0,0 +1,44 @@
+#!/bin/bash
+do_exec=
+ret=1
+declare -a opts
+while test $# -gt 0
+do
+    case "$1" in
+    --exec)
+        do_exec="exec"
+    ;;
+    --opt)
+        opts=(${opts[@]} "$2")
+        shift
+    ;;
+    esac
+    shift
+done
+
+. @XEN_SCRIPT_DIR@/hotplugpath.sh
+
+XENSTORED=@XENSTORED@
+XENSTORED_ROOTDIR="@XEN_LIB_STORED@"
+
+xencommons_config=@CONFIG_DIR@/@CONFIG_LEAF_DIR@
+test -f $xencommons_config/xencommons && . $xencommons_config/xencommons
+
+rm -f "$XENSTORED_ROOTDIR"/tdb* &>/dev/null
+test -z "$XENSTORED_TRACE" || XENSTORED_TRACE_ARGS=" -T /var/log/xen/xenstored-trace.log"
+
+echo -n Starting $XENSTORED...
+if $do_exec $XENSTORED ${opts[@]} $XENSTORED_TRACE_ARGS $XENSTORED_ARGS
+then
+    # Wait for xenstored to actually come up, timing out after 30 seconds
+    while [ $time -lt $timeout ] && ! `${BINDIR}/xenstore-read -s / >/dev/null 2>&1` ; do
+        echo -n .
+        time=$(($time+1))
+        sleep 1
+    done
+    echo
+    if [ $time -lt $timeout ]; then
+        ret=0
+    fi
+fi
+exit ${ret}

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Ian Jackson]

Olaf Hering writes ("Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd"):
> On Fri, Dec 05, Ian Jackson wrote:
> > I think the only way to make this work properly is to factor the
> > necessary parts out of init.d/xencommons into a new script which can
> > be used by both xencommons and systemd.  I'm not sure such a patch
> > would be appropriate for 4.5 at this stage.
> 
> I came up with this, it appears to work in my testing. Will do more
> testing later today.

Thanks.  I think this is going in roughly the right direction.

But: I think the script is rather over-engineered, and that it ought
to be in /etc.

> +	$(INSTALL_PROG) $(XENSTORED_LIBEXEC) $(DESTDIR)$(LIBEXEC_BIN)

Sysadmins might want to edit the script to do something we haven't
thought of.  So it should be in /etc where they can do so.

> diff --git a/tools/hotplug/Linux/xenstored.sh.in b/tools/hotplug/Linux/xenstored.sh.in
> new file mode 100644
> index 0000000..11caf25
> --- /dev/null
> +++ b/tools/hotplug/Linux/xenstored.sh.in
...
> +    case "$1" in
> +    --exec)
> +        do_exec="exec"
> +    ;;
> +    --opt)
> +        opts=(${opts[@]} "$2")
> +        shift
> +    ;;
> +    esac
> +    shift
> +done

I don't think this script wants to contain an option parser!

> +. @XEN_SCRIPT_DIR@/hotplugpath.sh
...
> +test -f $xencommons_config/xencommons && . $xencommons_config/xencommons
...
> +    # Wait for xenstored to actually come up, timing out after 30 seconds
> +    while [ $time -lt $timeout ] && ! `${BINDIR}/xenstore-read -s / >/dev/null 2>&1` ; do

I would have expected this new script to contain only the
functionality which was previously both in (a) the systemd unit and
(b) the traditional init script, and which you are removing from both
those places.  So I think you should be able to say "no ultimate
functional change" in your commit message.

The systemd unit doesn't currently contain anything messing about with
xenstore-read to detect when xenstored is working.  Is that a bug that
was previously in the systemd unit, or is it a mistake in your patch
that this is added here ?

Thanks,
Ian.

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Olaf Hering]

On Tue, Dec 09, Ian Jackson wrote:

> Olaf Hering writes ("Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd"):
> > On Fri, Dec 05, Ian Jackson wrote:
> > > I think the only way to make this work properly is to factor the
> > > necessary parts out of init.d/xencommons into a new script which can
> > > be used by both xencommons and systemd.  I'm not sure such a patch
> > > would be appropriate for 4.5 at this stage.
> > 
> > I came up with this, it appears to work in my testing. Will do more
> > testing later today.
> 
> Thanks.  I think this is going in roughly the right direction.
> 
> But: I think the script is rather over-engineered, and that it ought
> to be in /etc.

Why should the wrapper be in /etc?!
xendomains isnt in /etc either.

> > +	$(INSTALL_PROG) $(XENSTORED_LIBEXEC) $(DESTDIR)$(LIBEXEC_BIN)
> 
> Sysadmins might want to edit the script to do something we haven't
> thought of.  So it should be in /etc where they can do so.

So they should mail here if they find substantial functionality missing.
Until then they continue to not enable xencommons and run their own
startup script.

> I don't think this script wants to contain an option parser!

How should it handle exec vs. no-exec? Just a single yes/no knob, so
essentially sysv vs systemd?

> The systemd unit doesn't currently contain anything messing about with
> xenstore-read to detect when xenstored is working.  Is that a bug that
> was previously in the systemd unit, or is it a mistake in your patch
> that this is added here ?

No idea how long it takes to have a functional xenstored after running
it. Perhaps the forking has some overhead and it returns earlier than it
can process requests. If thats the reason why the loop exists in the
sysv runlevel script then that loop should be used only without --no-fork.


Olaf

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Ian Jackson]

Olaf Hering writes ("Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd"):
> On Tue, Dec 09, Ian Jackson wrote:
> > But: I think the script is rather over-engineered, and that it ought
> > to be in /etc.
> 
> Why should the wrapper be in /etc?!

Because it is the last chance the admin has to adjust how xenstored is
run, which they ought to be able to do.

> xendomains isnt in /etc either.

xendomains is rather different.

> > > +	$(INSTALL_PROG) $(XENSTORED_LIBEXEC) $(DESTDIR)$(LIBEXEC_BIN)
> > 
> > Sysadmins might want to edit the script to do something we haven't
> > thought of.  So it should be in /etc where they can do so.
> 
> So they should mail here if they find substantial functionality missing.

That is no good because it doesn't allow them to make the change
immediately on their own system, in a way that won't be overwritten by
their system's package manager.

> Until then they continue to not enable xencommons and run their own
> startup script.

That is not a practical suggestion.

Bottom line: as relevant maintainer, I'm afraid I'm going to insist
that this script be in /etc.


> > I don't think this script wants to contain an option parser!
> 
> How should it handle exec vs. no-exec? Just a single yes/no knob, so
> essentially sysv vs systemd?

I was imagining a "named parameter" as SuS calls them.  One or both
the sites which run this wrapper script would pass an environment
variable.  Something like this in the script:

  $xenstored_do_exec $XENSTORED "$@" $XENSTORED_TRACE_ARGS $XENSTORED_ARGS

where the systemd unit simply sets `xenstored_do_exec=exec'.


> > The systemd unit doesn't currently contain anything messing about with
> > xenstore-read to detect when xenstored is working.  Is that a bug that
> > was previously in the systemd unit, or is it a mistake in your patch
> > that this is added here ?
> 
> No idea how long it takes to have a functional xenstored after running
> it. Perhaps the forking has some overhead and it returns earlier than it
> can process requests. If thats the reason why the loop exists in the
> sysv runlevel script then that loop should be used only without --no-fork.

I think it is up to you as the person proposing a change to explain
what the situation is and why your change is justified.  It's not
really satisfactory for a maintainer or reviewer to ask questions of a
submitter and get simply `I'm not sure ... perhaps ...' without some
kind of acknowledgement that more information (and perhaps research)
is needed before we can establish how the code should look.

In this case that means I think you should find out whether the lack
of this xenstore-read polling loop is actually a bug in the existing
systemd unit.  If (as I suspect) this is not a bug, then your code
motion should not change this aspect of the operation under systemd.

Thanks,
Ian.

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Olaf Hering]

On Tue, Dec 09, Ian Jackson wrote:

> Bottom line: as relevant maintainer, I'm afraid I'm going to insist
> that this script be in /etc.

I dont agree with the reasoning, but to get this done:
Which place would that be, XEN_SCRIPT_DIR?

> > > I don't think this script wants to contain an option parser!
> > 
> > How should it handle exec vs. no-exec? Just a single yes/no knob, so
> > essentially sysv vs systemd?
> 
> I was imagining a "named parameter" as SuS calls them.  One or both
> the sites which run this wrapper script would pass an environment
> variable.  Something like this in the script:
> 
>   $xenstored_do_exec $XENSTORED "$@" $XENSTORED_TRACE_ARGS $XENSTORED_ARGS
> 
> where the systemd unit simply sets `xenstored_do_exec=exec'.

Ok, I will implement this.

> In this case that means I think you should find out whether the lack
> of this xenstore-read polling loop is actually a bug in the existing
> systemd unit.  If (as I suspect) this is not a bug, then your code
> motion should not change this aspect of the operation under systemd.

I think any daemon needs some time until its operational. In case of
xenstored there are users which expect its functionality right away,
such as xen-init-dom0 and the other tools launched by xencommons. Thats
likely the reason why the loop is there, and the commit msg of f706d9e9a
confirms that. With systemd we can only hope that it handles this
properly with its socket passing functionality. If not, the startup code
could be done like I did in my patch do avoid code duplication in a
to-be-added ExecStartPost=.

And the "is xenstored ready?" check in my current implementation would
not work anyway because once the shell does exec it will not run the
following code which does the "xenstore-read -s".

Olaf

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Ian Campbell]

On Wed, 2014-12-10 at 10:15 +0100, Olaf Hering wrote:
> > I was imagining a "named parameter" as SuS calls them.  One or both
> > the sites which run this wrapper script would pass an environment
> > variable.  Something like this in the script:
> > 
> >   $xenstored_do_exec $XENSTORED "$@" $XENSTORED_TRACE_ARGS $XENSTORED_ARGS
> > 
> > where the systemd unit simply sets `xenstored_do_exec=exec'.
> 
> Ok, I will implement this.

I'm not sure why we don't want to exec in both cases, making this whole
problem moot.

> > In this case that means I think you should find out whether the lack
> > of this xenstore-read polling loop is actually a bug in the existing
> > systemd unit.  If (as I suspect) this is not a bug, then your code
> > motion should not change this aspect of the operation under systemd.
> 
> I think any daemon needs some time until its operational. In case of
> xenstored there are users which expect its functionality right away,
> such as xen-init-dom0 and the other tools launched by xencommons. Thats
> likely the reason why the loop is there, and the commit msg of f706d9e9a
> confirms that. With systemd we can only hope that it handles this
> properly with its socket passing functionality.

I think we should assume that socket activation is working properly, or
else what is the point of socket activation? If it is buggy then we
should fix it, not add hacks to the wrapper or unit files.

That results in a wrapper which unconditionally execs, the systemd unit
just calls that while the sysv script runs the wrapper and then does the
xenstore-read -s loop. 

>  If not, the startup code
> could be done like I did in my patch do avoid code duplication in a
> to-be-added ExecStartPost=.
> 
> And the "is xenstored ready?" check in my current implementation would
> not work anyway because once the shell does exec it will not run the
> following code which does the "xenstore-read -s".

Separately from the above I wonder if it might be worth moving the
xenstore readiness check into the xen-init-dom0 helper and having most
things which currently depend on xenstore actually depend on the
"dom0-is-ready" unit, which itself depends on xenstored, qemu-dom0 and
whatever else is supposed to be ready in a dom0?

Ian.

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Olaf Hering]

On Wed, Dec 10, Ian Campbell wrote:

> I'm not sure why we don't want to exec in both cases, making this whole
> problem moot.

Good point!

That will probably work because sysv lets xenstored do the fork, so the
script will return to its caller. In systemd case --no-fork is passed so
the started process will become xenstored, which is expected by systemd.

Olaf

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Olaf Hering]

On Wed, Dec 10, Ian Campbell wrote:

> That results in a wrapper which unconditionally execs, the systemd unit
> just calls that while the sysv script runs the wrapper and then does the
> xenstore-read -s loop. 

Since systemd handles the socket there is already a listener.
http://lists.freedesktop.org/archives/systemd-devel/2014-December/026157.html


I came up with this, works in my testing with SLE11 sysv and Factory
systemd. The beef looks like shown below.  Let me know if thats good
enough to handle XENSTORED_TRACE also in systemd. I will add some
comments to the wrapper why it is there.

Olaf

diff --git a/tools/hotplug/Linux/init.d/xencommons.in b/tools/hotplug/Linux/init.d/xencommons.in
index a1095c2..f57bfd3 100644
--- a/tools/hotplug/Linux/init.d/xencommons.in
+++ b/tools/hotplug/Linux/init.d/xencommons.in
@@ -66,11 +66,13 @@ do_start () {
 	then
 		test -z "$XENSTORED_ROOTDIR" && XENSTORED_ROOTDIR="@XEN_LIB_STORED@"
 		rm -f "$XENSTORED_ROOTDIR"/tdb* &>/dev/null
-		test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log"
 
 		if [ -n "$XENSTORED" ] ; then
 		    echo -n Starting $XENSTORED...
-		    $XENSTORED --pid-file /var/run/xenstored.pid $XENSTORED_ARGS
+		    XENSTORED=$XENSTORED \
+		    XENSTORED_TRACE=$XENSTORED_TRACE \
+		    XENSTORED_ARGS=$XENSTORED_ARGS \
+		    ${LIBEXEC_BIN}/xenstored.sh --pid-file /var/run/xenstored.pid
 		else
 		    echo "No xenstored found"
 		    exit 1
diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
index 0f0ac58..d906ea2 100644
--- a/tools/hotplug/Linux/systemd/xenstored.service.in
+++ b/tools/hotplug/Linux/systemd/xenstored.service.in
@@ -8,13 +8,12 @@ ConditionPathExists=/proc/xen/capabilities
 
 [Service]
 Type=notify
-Environment=XENSTORED_ARGS=
 Environment=XENSTORED=@XENSTORED@
-EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
+EnvironmentFile=@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
 ExecStartPre=-/bin/rm -f @XEN_LIB_STORED@/tdb*
 ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@
-ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS"
+ExecStart=-@LIBEXEC_BIN@/xenstored.sh --no-fork
 
 [Install]
 WantedBy=multi-user.target
diff --git a/tools/hotplug/Linux/xenstored.sh.in b/tools/hotplug/Linux/xenstored.sh.in
new file mode 100644
index 0000000..dc806ee
--- /dev/null
+++ b/tools/hotplug/Linux/xenstored.sh.in
@@ -0,0 +1,6 @@
+#!/bin/sh
+if test -n "$XENSTORED_TRACE"
+then
+	XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log"
+fi
+exec $XENSTORED $@ $XENSTORED_ARGS

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Ian Campbell]

On Wed, 2014-12-10 at 18:52 +0100, Olaf Hering wrote:
> On Wed, Dec 10, Ian Campbell wrote:
> 
> > That results in a wrapper which unconditionally execs, the systemd unit
> > just calls that while the sysv script runs the wrapper and then does the
> > xenstore-read -s loop. 
> 
> Since systemd handles the socket there is already a listener.
> http://lists.freedesktop.org/archives/systemd-devel/2014-December/026157.html

Not sure I understand what any of that means, but I'll assume it's
good ;-)

> I came up with this, works in my testing with SLE11 sysv and Factory
> systemd. The beef looks like shown below.  Let me know if thats good
> enough to handle XENSTORED_TRACE also in systemd. I will add some
> comments to the wrapper why it is there.

Seems ok. I wonder if the wrapper ought to source
@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons to obtain XENSTORED_* itself
rather than relying on the initscript and unit file to do so. Especially
in the initscript case it looks a bit ugly to have to manually propagate
things.

> 
> Olaf
> 
> diff --git a/tools/hotplug/Linux/init.d/xencommons.in b/tools/hotplug/Linux/init.d/xencommons.in
> index a1095c2..f57bfd3 100644
> --- a/tools/hotplug/Linux/init.d/xencommons.in
> +++ b/tools/hotplug/Linux/init.d/xencommons.in
> @@ -66,11 +66,13 @@ do_start () {
>  	then
>  		test -z "$XENSTORED_ROOTDIR" && XENSTORED_ROOTDIR="@XEN_LIB_STORED@"
>  		rm -f "$XENSTORED_ROOTDIR"/tdb* &>/dev/null
> -		test -z "$XENSTORED_TRACE" || XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log"
>  
>  		if [ -n "$XENSTORED" ] ; then
>  		    echo -n Starting $XENSTORED...
> -		    $XENSTORED --pid-file /var/run/xenstored.pid $XENSTORED_ARGS
> +		    XENSTORED=$XENSTORED \
> +		    XENSTORED_TRACE=$XENSTORED_TRACE \
> +		    XENSTORED_ARGS=$XENSTORED_ARGS \
> +		    ${LIBEXEC_BIN}/xenstored.sh --pid-file /var/run/xenstored.pid
>  		else
>  		    echo "No xenstored found"
>  		    exit 1
> diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
> index 0f0ac58..d906ea2 100644
> --- a/tools/hotplug/Linux/systemd/xenstored.service.in
> +++ b/tools/hotplug/Linux/systemd/xenstored.service.in
> @@ -8,13 +8,12 @@ ConditionPathExists=/proc/xen/capabilities
>  
>  [Service]
>  Type=notify
> -Environment=XENSTORED_ARGS=
>  Environment=XENSTORED=@XENSTORED@
> -EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
> +EnvironmentFile=@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
>  ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
>  ExecStartPre=-/bin/rm -f @XEN_LIB_STORED@/tdb*
>  ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@
> -ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS"
> +ExecStart=-@LIBEXEC_BIN@/xenstored.sh --no-fork
>  
>  [Install]
>  WantedBy=multi-user.target
> diff --git a/tools/hotplug/Linux/xenstored.sh.in b/tools/hotplug/Linux/xenstored.sh.in
> new file mode 100644
> index 0000000..dc806ee
> --- /dev/null
> +++ b/tools/hotplug/Linux/xenstored.sh.in
> @@ -0,0 +1,6 @@
> +#!/bin/sh
> +if test -n "$XENSTORED_TRACE"
> +then
> +	XENSTORED_ARGS=" -T /var/log/xen/xenstored-trace.log"
> +fi
> +exec $XENSTORED $@ $XENSTORED_ARGS
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Olaf Hering]

On Fri, Dec 12, Ian Campbell wrote:

> Seems ok. I wonder if the wrapper ought to source
> @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons to obtain XENSTORED_* itself
> rather than relying on the initscript and unit file to do so. Especially
> in the initscript case it looks a bit ugly to have to manually propagate
> things.

It seems all that wrapping is of no use because SELinux can not deal
with it. I will see if "ExecStart=/bin/ary --no-fork $ENVVAR" can be
used to pass additional arguments. If so, the current XENSTORED_TRACE
handling has to be removed in favour of XENSTORED_ARGS=.

Olaf

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Ian Campbell]

On Fri, 2014-12-12 at 12:37 +0100, Olaf Hering wrote:
> On Fri, Dec 12, Ian Campbell wrote:
> 
> > Seems ok. I wonder if the wrapper ought to source
> > @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons to obtain XENSTORED_* itself
> > rather than relying on the initscript and unit file to do so. Especially
> > in the initscript case it looks a bit ugly to have to manually propagate
> > things.
> 
> It seems all that wrapping is of no use because SELinux can not deal
> with it.

I suppose you mean "the current SELinux policies". Surely SELinux in
general can cope with execing things...

>  I will see if "ExecStart=/bin/ary --no-fork $ENVVAR" can be
> used to pass additional arguments. If so, the current XENSTORED_TRACE
> handling has to be removed in favour of XENSTORED_ARGS=.
> 
> Olaf

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[M A Young]

On Fri, 12 Dec 2014, Ian Campbell wrote:

> On Fri, 2014-12-12 at 12:37 +0100, Olaf Hering wrote:
>> On Fri, Dec 12, Ian Campbell wrote:
>>
>>> Seems ok. I wonder if the wrapper ought to source
>>> @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons to obtain XENSTORED_* itself
>>> rather than relying on the initscript and unit file to do so. Especially
>>> in the initscript case it looks a bit ugly to have to manually propagate
>>> things.
>>
>> It seems all that wrapping is of no use because SELinux can not deal
>> with it.
>
> I suppose you mean "the current SELinux policies". Surely SELinux in
> general can cope with execing things...

I suspect it is more how systemd implements selinux. xenstored does get 
the right permissions eventually, but too late to connect to the sockets.

 	Michael Young

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Olaf Hering]

On Fri, Dec 12, Olaf Hering wrote:

> On Fri, Dec 12, Ian Campbell wrote:
> 
> > Seems ok. I wonder if the wrapper ought to source
> > @CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons to obtain XENSTORED_* itself
> > rather than relying on the initscript and unit file to do so. Especially
> > in the initscript case it looks a bit ugly to have to manually propagate
> > things.
> 
> It seems all that wrapping is of no use because SELinux can not deal
> with it. I will see if "ExecStart=/bin/ary --no-fork $ENVVAR" can be
> used to pass additional arguments. If so, the current XENSTORED_TRACE
> handling has to be removed in favour of XENSTORED_ARGS=.

This works:
ExecStart=@XENSTORED@ --no-fork $XENSTORED_ARGS

This fails:
ExecStart=$XENSTORED --no-fork $XENSTORED_ARGS
Dez 12 13:06:21 optiplex systemd[1]: [/usr/lib/systemd/system/xenstored.service:16] Executable path is not absolute, ignoring: $XENSTORED --no-fork $XENSTORED_ARGS

Looks like variables are not expanded for the executable itself. If that
really has to be supported a wrapper is required.

Maybe that new wrapper script just needs some special SELinux handling?
No idea.

Olaf

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Olaf Hering]

On Fri, Dec 12, Olaf Hering wrote:

> This works:
> ExecStart=@XENSTORED@ --no-fork $XENSTORED_ARGS
> 
> This fails:
> ExecStart=$XENSTORED --no-fork $XENSTORED_ARGS

But this will likely work:
ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS


Let me know how to proceed...

Olaf

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Olaf Hering]

On Wed, Dec 10, Ian Campbell wrote:

> Separately from the above I wonder if it might be worth moving the
> xenstore readiness check into the xen-init-dom0 helper and having most
> things which currently depend on xenstore actually depend on the
> "dom0-is-ready" unit, which itself depends on xenstored, qemu-dom0 and
> whatever else is supposed to be ready in a dom0?

You mean something like this chain of depencencies?

 xenstored
 xen-init-dom0
 xenconsoled | qemu-dom0
 xendomains

Olaf

Re: [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd

[Ian Campbell]

On Wed, 2014-12-10 at 19:01 +0100, Olaf Hering wrote:
> On Wed, Dec 10, Ian Campbell wrote:
> 
> > Separately from the above I wonder if it might be worth moving the
> > xenstore readiness check into the xen-init-dom0 helper and having most
> > things which currently depend on xenstore actually depend on the
> > "dom0-is-ready" unit, which itself depends on xenstored, qemu-dom0 and
> > whatever else is supposed to be ready in a dom0?
> 
> You mean something like this chain of depencencies?
> 
>  xenstored
>  xen-init-dom0
>  xenconsoled | qemu-dom0
>  xendomains

If you mean | to be "in parallel" rather than "or" then yes, I think
that's what I meant, or something like it.

Ian.