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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-05-20 at 05:31 -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@xxxxxxxx>
> 
> As it stands oxenstored will be used by default if ocaml tools are
> found, the init system will also try to use oxenstored first if its
> found otherwise the cxenstored will be used. Lets simplify the init
> script and let users be explicit about the preference through configure.
> 
> This adds support to let you be explicit about the xenstored preference,
> you can only use one of these two options:
> 
> ./configure --with-xenstored=cxenstored
> ./configure --with-xenstored=oxenstored
> 
> We continue with the old behaviour and default oxenstored will be used
> but only if you have ocaml dependencies. Since the xenstored preference
> is explicit now we can move it to the shared top level config and use
> this to simplify both the legacy init script and eventually our systemd
> service files. This should also make it clear on how you can still
> enable usage of cxenstored but still also use ocaml tools. If you have
> both oxenstored and cxenstored installed you can change the xenstore by
> a simple flip on the system configuration file.
> 
> In order to help with documentation we update the README with some
> details on configure usage refer to the wiki [0] [1] [2] for more elaborate
> details.
> 
> [0] http://wiki.xen.org/wiki/Xenstored
> [1] http://wiki.xen.org/wiki/XenStore
> [2] http://wiki.xen.org/wiki/XenStoreReference
> 
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Signed-off-by: Luis R. Rodriguez <mcgrof@xxxxxxxx>
> ---
> 
> Please run ./autgen.sh after this patch is applied.
> 
>  README                                | 34 +++++++++++++++++++
>  config/Toplevel.mk.in                 |  3 ++
>  config/xen-environment-header.in      |  3 ++
>  config/xen-environment-scripts.in     |  3 ++
>  configure.ac                          |  1 +
>  m4/expand_config.m4                   | 61 
> +++++++++++++++++++++++++++++++++++
>  stubdom/configure.ac                  |  1 +
>  tools/hotplug/Linux/init.d/xencommons |  6 ----
>  8 files changed, 106 insertions(+), 6 deletions(-)
> 
> diff --git a/README b/README
> index 079e6a9..4183b34 100644
> --- a/README
> +++ b/README
> @@ -145,6 +145,40 @@ Further documentation can be found on the wiki:
>  
> http://wiki.xen.org/wiki/Category:Host_Configuration#System_wide_xen_configuration
>  
> +xenstored: cxenstored and oxenstored
> +====================================
> +
> +Xen uses a xenstore [0] to upkeep configuration and status information shared

s/a xenstore/xenstore/

or uses a configuration database (xenstore[0]).

"upkeep" is not the word you are looking for either. ...to store... is
probably correct, or maintain.

> +between domains. A daemon is provided to help respond to queries from dom0
> +and guests,

Well, the daemon is provided because it is the thing which implemented
the xenstore. Saying it is provided to respond to queries is a strange
way to phrase it.

>  details for the xenstored can be found on the wiki's xenstored [2]
> +page. Two xenstored daemons are supported and you can choose which xenstored
> +you want to enable on a system through configure:
                     ^ by default (I hope, it should be possible to
change my mind by editing the configuration without rebuilding)

> +
> +     ./configure --with-xenstored=cxenstored
> +     ./configure --with-xenstored=oxenstored
> +
> +By defalut oxenstored will be used if the ocaml development tools are found.

"default"

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

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.

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

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

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

> +             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? Also there can't be any need to call
AC_PROG_{OCAML,FINDLIB} quite as many times as you do, surely.

> +     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?

> +     [
> +             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
if $xenstore = ocaml && ocaml not detected ; 
  AC_ERROR[].
fi


> +             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?

>  ])

Ian


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