Re: [PATCH v5 12/14] autoconf: xen: enable explicit preference option for xenstored preference

["Luis R. Rodriguez" <mcgrof@suse.com>]

On Wed, May 21, 2014 at 04:44:00PM +0100, Ian Campbell wrote:
> On Tue, 2014-05-20 at 05:31 -0700, Luis R. Rodriguez wrote:
> > diff --git a/README b/README
> > index 079e6a9..4183b34 100644
> > --- a/README
> > +++ b/README
> 
> > +If you enable oxenstored the cxenstored will still be built and installed,
> > +the xenstored used can be changed through the configuration file:
> > +
> > +/etc/xen/scripts/hotplugpath.sh
> 
> That's an unconventional place for such a configuration. Today it is
> in /etc/{sysconfig,default}/xencommons, I think, which is more expected
> IMHO. That comes from tools/hotplug/Linux/init.d/sysconfig.xencommons.

Yeah good point, I had not seen that.

> It might be reasonable for hotplugpath.sh to define
> XENSTORE_xenstored=/path/to/xenstored
> XENSTORE_oxenstored=/path/to/oxenstored
> 
> and use the existing XENSTORED variable in sysconfig to select which.

Ah but but hotplugpath.sh is one of those automatically generated files
and after my last patch in this series they are all now shared in one
master file, config/xen-environment-scripts.in, and since the we want
to keep script file Shell / Python agnostic checking which one is set
on sysconfig is not something reasonable to do on that master file.

So here's the thing:

My series of patches already deals with the old init and xencommons script to
start xenstore, it used the hotplugpath.sh for deducing the xenstore
daemon it should use by default and switching this to rely on the sysconfig
xencommons should be easy if it wasn't already dealt with there already.

For systemd though we can't use varibles on ExecStart for the process, we
however can use environment variables. We have a few options. Fedora's
approach was to use two service unit files which conflicted with each other,
although I'm sure we can work it out by using a target unit and let folks
flip it seems rather silly to me to maintain two service unit files with
identical entries. Therefore in my new approach usres would have to manually
edit the service file with the differnt path / binary. Another possibility
is to have a central launcher binary that just does getenv() and execve()
on the desired binary. If this is agreeable I can work on it and it could
even be shared by the old init as well.

> This would also avoid the user having to update the name and the path
> (as would the use of basename mind you)

Agreed.

> > diff --git a/m4/expand_config.m4 b/m4/expand_config.m4
> > index 717fcd1..40d9707 100644
> > --- a/m4/expand_config.m4
> > +++ b/m4/expand_config.m4
> > @@ -58,4 +58,65 @@ AC_SUBST(XEN_RUN_DIR)
> >  
> 
> I'd say that most of this belongs in something which is handled from
> tools/configure.ac not the top level. m4/xenstored.m4 seems logical.

Sure.

> >  XEN_PAGING_DIR=/var/lib/xen/xenpaging
> >  AC_SUBST(XEN_PAGING_DIR)
> > +
> > +AC_DEFUN([AX_XEN_OCAML_XENSTORE_CHECK], [
> > +	AC_PROG_OCAML
> > +	AC_PROG_FINDLIB
> > +	AS_IF([test "x$OCAMLC" = "xno" || test "x$OCAMLFIND" = "xno"], [
> 
> THen you could rely on m4/ocaml.m4 to have checked all this already.

Just including m4/ocaml.m4 won't get us much, the above logic assumes
that file was included so there is a bit of a trick we need to adjust
in order to avoid multiple calls to the same checks.

> 
> > +		AC_MSG_ERROR([Missing ocaml dependencies for oxenstored, try installing ocaml ocaml-compiler-libs ocaml-runtime ocaml-findlib])
> > +	])
> > +])
> > +
> > +AC_DEFUN([AX_XEN_OCAML_XENSTORE_DEFAULTS], [
> 
> Why is this separate from CHECK?

Its not a check, I should remove the double calls to AC_PROG_OCAML
and AC_PROG_OCAML, what this does is it sets the defaults based
on your build system without considering any flags passed.

> Also there can't be any need to call
> AC_PROG_{OCAML,FINDLIB} quite as many times as you do, surely.

We can surely move the AC_PROG_OCAML and AC_PROG_FINDLIB calls outside
so that the check can be done just once. We want to support not having
to pass the flag for specifying the xenstore preference.

> > +	xenstore="oxenstored"
> > +	xenstored=$SBINDIR/oxenstored
> > +	AC_PROG_OCAML
> > +	AC_PROG_FINDLIB
> > +	AS_IF([test "x$OCAMLC" = "xno" || test "x$OCAMLFIND" = "xno"], [
> > +		xenstore="cxenstored"
> > +		xenstored=$SBINDIR/xenstored
> > +	])
> > +])
> > +
> > +AS_IF([test "x$XENSTORE" = "x"], [
> > +AC_ARG_WITH([xenstored],
> > +	AS_HELP_STRING([--with-xenstored@<:@=cxenstored|cxenstored@:>@],
> > +		[This lets you choose which xenstore daemon you want, you have
> > +		two options: the original xenstored written in C (cxenstored)
> > +		or the newer and robust one written in Ocaml (oxenstored).
> > +		The oxenstored daemon is the default but will but can only
> > +		be used if you have ocaml library / build dependencies solved,
> > +		if you have not specified a preference and do not have ocaml
> > +		dependencies resolved we'll enable the C xenstored for you. If
> > +		you ask for oxenstored we'll complain until you resolve those
> > +		dependencies]),
> 
> Does this not end up with an overly verbose configure --help output?

Its fine by me, check and let me know if its too much.

> > +	[
> > +		AS_IF([test "x$withval" = "xcxenstored"], [
> > +			xenstore="cxenstored"
> > +			xenstored=$SBINDIR/xenstored
> > +		])
> > +		AS_IF([test "x$withval" = "xoxenstored"], [
> > +			xenstore="oxenstored"
> > +			xenstored=$SBINDIR/oxenstored
> > +			AX_XEN_OCAML_XENSTORE_CHECK()
> > +		])
> 
> xenstore=$withval

OK

> if $xenstore = ocaml && ocaml not detected ; 
>   AC_ERROR[].
> fi

That's what AX_XEN_OCAML_XENSTORE_CHECK() does, but I agree this
can be simplified. Will work on that.

> 
> > +		AS_IF([test "x$withval" != "xoxenstored" && test "x$withval" != "xcxenstored"], [
> > +			AC_MSG_ERROR([Unsupported xenstored specified, supported types: oxenstored cxenstored])
> > +		])
> > +	],
> > +	[
> > +		AX_XEN_OCAML_XENSTORE_DEFAULTS()
> > +	])
> > +])
> > +
> > +XENSTORE=$xenstore
> > +AC_SUBST(XENSTORE)
> > +
> > +AS_IF([test "x$XENSTORED" = "x"], [
> > +	XENSTORED=$xenstored
> > +])
> > +AC_SUBST(XENSTORED)
> > +
> > +AS_IF([test "x$XENSTORE" != "xcxenstored" && test "x$XENSTORE" != "xoxenstored"],
> > +	[AC_MSG_ERROR([Invalid xenstore: $XENSTORE])])
> 
> Didn't this already get checked above?

Yeah I went trigger happy with checks, will remove.

  Luis