[PATCH 0/2] tools/hotplug/Linux changes

[PATCH 0/2] tools/hotplug/Linux changes

[Olaf Hering]

As suggested by IanC:

With these changes the existing XENSTORED_TRACE= sysconfig variable is
removed, in favor of a new XENSTORED_ARGS= variable. This variable is
already used in the code, but it was never put into the sysconfig files.

All this is to remove the illusion that XENSTORED_TRACE= will work with
systemd service files.

Note: it seems that XENSTORED_TRACE=yes will not work even today with
oxenstored and sysv runlevel script because oxenstored and xenstored do
not handle the same set of command line options. Thats not my fault.

I suggest to consider this for staging and staging-4.5.

Olaf

Olaf Hering (2):
  tools/hotplug: introduce XENSTORED_ARGS= in sysconfig file.
  tools/hotplug: remove usage of XENSTORED_TRACE boolean

 tools/hotplug/Linux/init.d/sysconfig.xencommons.in | 8 +++++---
 tools/hotplug/Linux/init.d/xencommons.in           | 1 -
 2 files changed, 5 insertions(+), 4 deletions(-)

[PATCH 1/2] tools/hotplug: introduce XENSTORED_ARGS= in sysconfig file.

[Olaf Hering]

It is already used in the runlevel script and the service file.
It is supposed to replace XENSTORED_TRACE= boolean, which cant be easily
supported in the xenstored.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/init.d/sysconfig.xencommons.in | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
index c12fc8a..f0fa98d 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
+++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
@@ -21,6 +21,14 @@
 #XENSTORED=@XENSTORED@
 
 ## Type: string
+## Default: ""
+#
+# Additional commandline arguments to start xenstored,
+# like "--trace-file /var/log/xen/xenstored-trace.log"
+# See "@sbindir@/xenstored --help" for possible options.
+XENSTORED_ARGS=
+
+## Type: string
 ## Default: Not defined, tracing off
 #
 # Log xenstored messages

Re: [PATCH 1/2] tools/hotplug: introduce XENSTORED_ARGS= in sysconfig file.

[Ian Campbell]

On Wed, 2015-01-07 at 17:37 +0100, Olaf Hering wrote:
> It is already used in the runlevel script and the service file.
> It is supposed to replace XENSTORED_TRACE= boolean, which cant be easily
> supported in the xenstored.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/init.d/sysconfig.xencommons.in | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
> index c12fc8a..f0fa98d 100644
> --- a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
> +++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
> @@ -21,6 +21,14 @@
>  #XENSTORED=@XENSTORED@
>  
>  ## Type: string
> +## Default: ""
> +#
> +# Additional commandline arguments to start xenstored,
> +# like "--trace-file /var/log/xen/xenstored-trace.log"
> +# See "@sbindir@/xenstored --help" for possible options.

I think you want $XENSTORED here (back reference to the above) to cope
with the possible use of oxenstored.

Alternatively @XENSTORED@ would cause this to refer to the default
xenstored implementation, but I don't think that's quite right.

(TBH, saying "see foo --help for options" almost doesn't need saying)

> +XENSTORED_ARGS=
> +
> +## Type: string
>  ## Default: Not defined, tracing off
>  #
>  # Log xenstored messages
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 1/2] tools/hotplug: introduce XENSTORED_ARGS= in sysconfig file.

[Ian Jackson]

Olaf Hering writes ("[PATCH 1/2] tools/hotplug: introduce XENSTORED_ARGS= in sysconfig file."):
> It is already used in the runlevel script and the service file.
> It is supposed to replace XENSTORED_TRACE= boolean, which cant be easily
> supported in the xenstored.service file.

I don't think it is right to desupport XENSTORED_TRACE= which has been
supported in sysconfig.xencommons since at least Xen 4.1.

Certainly removing this feature this late in the 4.5 release cycle is
not appropriate.

Sorry,
Ian.

Re: [PATCH 1/2] tools/hotplug: introduce XENSTORED_ARGS= in sysconfig file.

[Ian Campbell]

On Wed, 2015-01-07 at 16:49 +0000, Ian Jackson wrote:
> Certainly removing this feature this late in the 4.5 release cycle is
> not appropriate.

I agree that faffing around with the initscripts/systemd units at the
eleventh hour seem liable to leave us with a release where xenstored
doesn't start or something.

Ian.

Re: [PATCH 1/2] tools/hotplug: introduce XENSTORED_ARGS= in sysconfig file.

[Olaf Hering]

On Wed, Jan 07, Ian Campbell wrote:

> On Wed, 2015-01-07 at 16:49 +0000, Ian Jackson wrote:
> > Certainly removing this feature this late in the 4.5 release cycle is
> > not appropriate.
> 
> I agree that faffing around with the initscripts/systemd units at the
> eleventh hour seem liable to leave us with a release where xenstored
> doesn't start or something.

What about staging, is that how it is supposed to look like in 4.6?

Or should I rather work on the wrapper script so that XENSTORED_TRACE
has to be used?

I would like to see patch #1 in 4.5 as the proper way to pass additional
options to xenstored.

Olaf

Re: [PATCH 1/2] tools/hotplug: introduce XENSTORED_ARGS= in sysconfig file.

[Konrad Rzeszutek Wilk]

On Thu, Jan 08, 2015 at 08:51:51AM +0100, Olaf Hering wrote:
> On Wed, Jan 07, Ian Campbell wrote:
> 
> > On Wed, 2015-01-07 at 16:49 +0000, Ian Jackson wrote:
> > > Certainly removing this feature this late in the 4.5 release cycle is
> > > not appropriate.
> > 
> > I agree that faffing around with the initscripts/systemd units at the
> > eleventh hour seem liable to leave us with a release where xenstored
> > doesn't start or something.
> 
> What about staging, is that how it is supposed to look like in 4.6?
> 
> Or should I rather work on the wrapper script so that XENSTORED_TRACE
> has to be used?
> 
> I would like to see patch #1 in 4.5 as the proper way to pass additional
> options to xenstored.

Too late. Yesterday was the commit cutoff :-(

> 
> Olaf

[PATCH 2/2] tools/hotplug: remove usage of XENSTORED_TRACE boolean

[Olaf Hering]

The existing XENSTORED_TRACE= boolean cant be easily implemented in the
xenstored.service file. All additional options should be added to the
XENSTORED_ARGS= variable.

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 | 6 ------
 tools/hotplug/Linux/init.d/xencommons.in           | 1 -
 2 files changed, 7 deletions(-)

diff --git a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
index f0fa98d..52d1843 100644
--- a/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
+++ b/tools/hotplug/Linux/init.d/sysconfig.xencommons.in
@@ -29,12 +29,6 @@
 XENSTORED_ARGS=
 
 ## Type: string
-## Default: Not defined, tracing off
-#
-# Log xenstored messages
-#XENSTORED_TRACE=[yes|on|1]
-
-## Type: string
 ## Default: "@XEN_LIB_STORED@"
 #
 # Running xenstored on XENSTORED_ROOTDIR
diff --git a/tools/hotplug/Linux/init.d/xencommons.in b/tools/hotplug/Linux/init.d/xencommons.in
index a1095c2..6af689e 100644
--- a/tools/hotplug/Linux/init.d/xencommons.in
+++ b/tools/hotplug/Linux/init.d/xencommons.in
@@ -66,7 +66,6 @@ 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...