Re: [PATCH v20210111 05/39] tools: add with-xen-scriptdir configure option

From: Ian Jackson <iwj@xenproject.org>
To: Olaf Hering <olaf@aepfle.de>
Cc: xen-devel@lists.xenproject.org, Wei Liu <wl@xen.org>
Subject: Re: [PATCH v20210111 05/39] tools: add with-xen-scriptdir configure option
Date: Mon, 8 Feb 2021 16:03:29 +0000	[thread overview]
Message-ID: <24609.24785.791060.128298@mariner.uk.xensource.com> (raw)
In-Reply-To: <20210111174224.24714-6-olaf@aepfle.de>

Olaf Hering writes ("[PATCH v20210111 05/39] tools: add with-xen-scriptdir configure option"):
> In the near future all fresh installations will have an empty /etc.
> The content of this directory will not be controlled by the package
> manager anymore. One of the reasons for this move is to make snapshots
> more robust.

As I wrote in September 2019 I don't agree with it, if it's intended
as a claim about all systems that Xen will run on.

However, this seems to be an accidental retention in the commit
message, since the content of the commit is now what I asked for,
which is to make this directory configurable.

So I approve of the contents of this patch in principle.

> As a first step into this direction, add a knob to configure to allow
> storing the hotplug scripts to libexec because they are not exactly
> configuration. The current default is unchanged, which is
> /etc/xen/scripts.

I suggest this commit message as a compromise:

  Some distros plan for fresh installations will have an empty /etc,
  whose content will not be controlled by the package manager
  anymore.

  To make this possible, add a knob to configure to allow storing the
  hotplug scripts to libexec instead of /etc/xen/scripts.

Olaf, would that be OK with you ?

As for detailed review:

> diff --git a/m4/paths.m4 b/m4/paths.m4
> index 89d3bb8312..0cec2bb190 100644
> --- a/m4/paths.m4
> +++ b/m4/paths.m4
...
> +AC_ARG_WITH([xen-scriptdir],
> +    AS_HELP_STRING([--with-xen-scriptdir=DIR],
> +    [Path to directory for dom0 hotplug scripts. [SYSCONFDIR/xen/scripts]]),
> +    [xen_scriptdir_path=$withval],
> +    [xen_scriptdir_path=$sysconfdir/xen/scripts])
...
> -XEN_SCRIPT_DIR=$XEN_CONFIG_DIR/scripts
> +XEN_SCRIPT_DIR=$xen_scriptdir_path
>  AC_SUBST(XEN_SCRIPT_DIR)

It is not clear to me why the deefault is changed from
"$XEN_CONFIG_DIR/scripts" to "$sysconfdir/xen/scripts" and there isn't
any explanation for this in the commit message.  I think this may make
no difference but an explanation is called for.

As for the release ack question: there is a risk that this patch would
break the build, by causing the scripts to go somewhere wrong.  This
risk seems fairly low and osstest would catch it.

However, I find it difficult to analyse the consequence of the changed
way the default is calculated.

So, a conditional release ack:

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

subject to changing this patch to make the actual implemented default
to still be $XEN_CONFIG_DIR/scripts.

Ian.