Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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 000..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
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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 000..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: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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 000..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.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [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. 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/xencommons" +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) $(I
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [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. Are you opposed to the idea to support XENSTORED_TRACE for systemd right in 4.5.0? Olaf ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 5/5] tools/hotplug: support XENSTORED_TRACE in systemd
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
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.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [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.
Signed-off-by: Olaf Hering
Cc: Ian Jackson
Cc: Stefano Stabellini
Cc: Ian Campbell
Cc: Wei Liu
---
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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
