| Message ID | 20210111174224.24714-6-olaf@aepfle.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | leftover from 2020 | expand |
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.
Am Mon, 8 Feb 2021 16:03:29 +0000 schrieb Ian Jackson <iwj@xenproject.org>: > 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 ? Yes, this is fine. Thanks. > 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. The reason is the ordering of assignments in the file: AC_ARG_WITH comes early in the file, XEN_CONFIG_DIR= is assigned much later. It seems the assignments for CONFIG_DIR and XEN_CONFIG_DIR can be moved up, because $sysconfdir is expected to be set already. As a result the new AC_ARG_WITH= can continue to use "$XEN_CONFIG_DIR/scripts" for the default case. I assume the current ordering is to have a separate AC_ARG_WITH and AC_SUBST section. Olaf
Olaf Hering writes ("Re: [PATCH v20210111 05/39] tools: add with-xen-scriptdir configure option"):
> The reason is the ordering of assignments in the file:
> AC_ARG_WITH comes early in the file, XEN_CONFIG_DIR= is assigned much later.
Ah.
> It seems the assignments for CONFIG_DIR and XEN_CONFIG_DIR can be moved up, because $sysconfdir is expected to be set already. As a result the new AC_ARG_WITH= can continue to use "$XEN_CONFIG_DIR/scripts" for the default case.
That would be best I think.
> I assume the current ordering is to have a separate AC_ARG_WITH and AC_SUBST section.
I could be wrong, but I don't think the location of AC_SUBST matters.
Ian.
Am Mon, 8 Feb 2021 17:48:31 +0000 schrieb Ian Jackson <iwj@xenproject.org>: > > It seems the assignments for CONFIG_DIR and XEN_CONFIG_DIR can be moved up, because $sysconfdir is expected to be set already. As a result the new AC_ARG_WITH= can continue to use "$XEN_CONFIG_DIR/scripts" for the default case. > > That would be best I think. I will split this into individual changes and send a separate series. Olaf
Olaf Hering writes ("Re: [PATCH v20210111 05/39] tools: add with-xen-scriptdir configure option"):
> Am Mon, 8 Feb 2021 17:48:31 +0000
> schrieb Ian Jackson <iwj@xenproject.org>:
> > > It seems the assignments for CONFIG_DIR and XEN_CONFIG_DIR can be moved up, because $sysconfdir is expected to be set already. As a result the new AC_ARG_WITH= can continue to use "$XEN_CONFIG_DIR/scripts" for the default case.
> >
> > That would be best I think.
>
> I will split this into individual changes and send a separate series.
OK, thanks, that will help with de-risking this from a release PoV.
Ian.
diff --git a/m4/paths.m4 b/m4/paths.m4 index 89d3bb8312..0cec2bb190 100644 --- a/m4/paths.m4 +++ b/m4/paths.m4 @@ -70,6 +70,12 @@ AC_ARG_WITH([libexec-leaf-dir], [libexec_subdir=$withval], [libexec_subdir=$PACKAGE_TARNAME]) +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]) + AC_ARG_WITH([xen-dumpdir], AS_HELP_STRING([--with-xen-dumpdir=DIR], [Path to directory for domU crash dumps. [LOCALSTATEDIR/lib/xen/dump]]), @@ -137,7 +143,7 @@ AC_SUBST(INITD_DIR) XEN_CONFIG_DIR=$CONFIG_DIR/xen AC_SUBST(XEN_CONFIG_DIR) -XEN_SCRIPT_DIR=$XEN_CONFIG_DIR/scripts +XEN_SCRIPT_DIR=$xen_scriptdir_path AC_SUBST(XEN_SCRIPT_DIR) case "$host_os" in
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 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. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- m4/paths.m4 | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)