[v2] Fix install.sh for systemd

[PATCH v2] Fix install.sh for systemd

Posted by Olaf Hering 12 months ago

On a fedora system, if you run `sudo sh install.sh` you break your
system.  The installation clobbers /var/run, a symlink to /run.  A
subsequent boot fails when /var/run and /run are different since
accesses through /var/run can't find items that now only exist in /run
and vice-versa.

Skip populating /var/run/xen during make install.
The directory is already created by some scripts. Adjust all remaining
scripts to create XEN_RUN_DIR at runtime.

XEN_RUN_STORED is covered by XEN_RUN_DIR because xenstored is usually
started afterwards.

Reported-by: Jason Andryuk <jandryuk@gmail.com>
Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 tools/Makefile                                     | 2 --
 tools/hotplug/FreeBSD/rc.d/xencommons.in           | 1 +
 tools/hotplug/FreeBSD/rc.d/xendriverdomain.in      | 1 +
 tools/hotplug/Linux/init.d/xendriverdomain.in      | 1 +
 tools/hotplug/Linux/systemd/xenconsoled.service.in | 2 +-
 tools/hotplug/NetBSD/rc.d/xendriverdomain.in       | 2 +-
 6 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/Makefile b/tools/Makefile
index 4906fdbc23..1ff90ddfa0 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -58,9 +58,7 @@ build all: subdirs-all
 install:
 	$(INSTALL_DIR) -m 700 $(DESTDIR)$(XEN_DUMP_DIR)
 	$(INSTALL_DIR) $(DESTDIR)$(XEN_LOG_DIR)
-	$(INSTALL_DIR) $(DESTDIR)$(XEN_RUN_DIR)
 	$(INSTALL_DIR) $(DESTDIR)$(XEN_LIB_DIR)
-	$(INSTALL_DIR) $(DESTDIR)$(XEN_RUN_STORED)
 	$(INSTALL_DIR) $(DESTDIR)$(PKG_INSTALLDIR)
 	$(MAKE) subdirs-install
 
diff --git a/tools/hotplug/FreeBSD/rc.d/xencommons.in b/tools/hotplug/FreeBSD/rc.d/xencommons.in
index 7f7cda289f..1cf217d418 100644
--- a/tools/hotplug/FreeBSD/rc.d/xencommons.in
+++ b/tools/hotplug/FreeBSD/rc.d/xencommons.in
@@ -34,6 +34,7 @@ xen_startcmd()
 	local time=0
 	local timeout=30
 
+	mkdir -p "@XEN_RUN_DIR@"
 	xenstored_pid=$(check_pidfile ${XENSTORED_PIDFILE} ${XENSTORED})
 	if test -z "$xenstored_pid"; then
 		printf "Starting xenservices: xenstored, xenconsoled."
diff --git a/tools/hotplug/FreeBSD/rc.d/xendriverdomain.in b/tools/hotplug/FreeBSD/rc.d/xendriverdomain.in
index a032822e33..030d104024 100644
--- a/tools/hotplug/FreeBSD/rc.d/xendriverdomain.in
+++ b/tools/hotplug/FreeBSD/rc.d/xendriverdomain.in
@@ -27,6 +27,7 @@ xendriverdomain_start()
 {
 	printf "Starting xenservices: xl devd."
 
+	mkdir -p "@XEN_RUN_DIR@"
 	PATH="${bindir}:${sbindir}:$PATH" ${sbindir}/xl devd --pidfile ${XLDEVD_PIDFILE} ${XLDEVD_ARGS}
 
 	printf "\n"
diff --git a/tools/hotplug/Linux/init.d/xendriverdomain.in b/tools/hotplug/Linux/init.d/xendriverdomain.in
index c63060f62a..1055d0b942 100644
--- a/tools/hotplug/Linux/init.d/xendriverdomain.in
+++ b/tools/hotplug/Linux/init.d/xendriverdomain.in
@@ -49,6 +49,7 @@ fi
 
 do_start () {
 	echo Starting xl devd...
+	mkdir -m700 -p @XEN_RUN_DIR@
 	${sbindir}/xl devd --pidfile=$XLDEVD_PIDFILE $XLDEVD_ARGS
 }
 do_stop () {
diff --git a/tools/hotplug/Linux/systemd/xenconsoled.service.in b/tools/hotplug/Linux/systemd/xenconsoled.service.in
index 1f03de9041..d84c09aa9c 100644
--- a/tools/hotplug/Linux/systemd/xenconsoled.service.in
+++ b/tools/hotplug/Linux/systemd/xenconsoled.service.in
@@ -11,7 +11,7 @@ Environment=XENCONSOLED_TRACE=none
 Environment=XENCONSOLED_LOG_DIR=@XEN_LOG_DIR@/console
 EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
-ExecStartPre=/bin/mkdir -p ${XENCONSOLED_LOG_DIR}
+ExecStartPre=/bin/mkdir -p ${XENCONSOLED_LOG_DIR} @XEN_RUN_DIR@
 ExecStart=@sbindir@/xenconsoled -i --log=${XENCONSOLED_TRACE} --log-dir=${XENCONSOLED_LOG_DIR} $XENCONSOLED_ARGS
 
 [Install]
diff --git a/tools/hotplug/NetBSD/rc.d/xendriverdomain.in b/tools/hotplug/NetBSD/rc.d/xendriverdomain.in
index f47b0b189c..23a5352502 100644
--- a/tools/hotplug/NetBSD/rc.d/xendriverdomain.in
+++ b/tools/hotplug/NetBSD/rc.d/xendriverdomain.in
@@ -23,7 +23,7 @@ XLDEVD_PIDFILE="@XEN_RUN_DIR@/xldevd.pid"
 
 xendriverdomain_precmd()
 {
-	:
+	mkdir -p "@XEN_RUN_DIR@"
 }
 
 xendriverdomain_startcmd()

Re: [PATCH v2] Fix install.sh for systemd

Posted by Jason Andryuk 11 months, 4 weeks ago

On Mon, May 8, 2023 at 1:14 PM Olaf Hering <olaf@aepfle.de> wrote:
>
> On a fedora system, if you run `sudo sh install.sh` you break your
> system.  The installation clobbers /var/run, a symlink to /run.  A
> subsequent boot fails when /var/run and /run are different since
> accesses through /var/run can't find items that now only exist in /run
> and vice-versa.
>
> Skip populating /var/run/xen during make install.
> The directory is already created by some scripts. Adjust all remaining
> scripts to create XEN_RUN_DIR at runtime.
>
> XEN_RUN_STORED is covered by XEN_RUN_DIR because xenstored is usually
> started afterwards.
>
> Reported-by: Jason Andryuk <jandryuk@gmail.com>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

Tested-by: Jason Andryuk <jandryuk@gmail.com>

I tested with Fedora/systemd.

Thanks, Olaf.

Regards,
Jason

Re: [PATCH v2] Fix install.sh for systemd

Posted by Andrew Cooper 12 months ago

On 08/05/2023 6:14 pm, Olaf Hering wrote:
> On a fedora system, if you run `sudo sh install.sh` you break your
> system.  The installation clobbers /var/run, a symlink to /run.  A
> subsequent boot fails when /var/run and /run are different since
> accesses through /var/run can't find items that now only exist in /run
> and vice-versa.
>
> Skip populating /var/run/xen during make install.
> The directory is already created by some scripts. Adjust all remaining
> scripts to create XEN_RUN_DIR at runtime.
>
> XEN_RUN_STORED is covered by XEN_RUN_DIR because xenstored is usually
> started afterwards.
>
> Reported-by: Jason Andryuk <jandryuk@gmail.com>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>

TBH, I think this goes to show how many people do `make install` by hand
on a system, rather than using a packaged version.

One query...

> diff --git a/tools/hotplug/Linux/init.d/xendriverdomain.in b/tools/hotplug/Linux/init.d/xendriverdomain.in
> index c63060f62a..1055d0b942 100644
> --- a/tools/hotplug/Linux/init.d/xendriverdomain.in
> +++ b/tools/hotplug/Linux/init.d/xendriverdomain.in
> @@ -49,6 +49,7 @@ fi
>  
>  do_start () {
>  	echo Starting xl devd...
> +	mkdir -m700 -p @XEN_RUN_DIR@

Why is this 700, and the others just using regular perms?

Also, doesn't it want quoting like the other examples too?

~Andrew

Re: [PATCH v2] Fix install.sh for systemd

Posted by Olaf Hering 11 months, 3 weeks ago

Tue, 9 May 2023 13:47:11 +0100 Andrew Cooper <andrew.cooper3@citrix.com>:

> Why is this 700, and the others just using regular perms?
> Also, doesn't it want quoting like the other examples too?

It is not clear why there is a single mkdir -m 0700 in the tree.
Most likely it will not give any extra security.

The scripts source hotplug.sh, which defines a variable XEN_RUN_DIR.
I think it is better to use the shell variable instead of hardcoded paths.

Regarding quoting: there are many paths used without quoting.
For the beauty an additional (huge) change could be done to quote
everything. Not sure if it is worth the effort...

I will post a v3 with this relative change:

--- a/tools/hotplug/FreeBSD/rc.d/xencommons.in
+++ b/tools/hotplug/FreeBSD/rc.d/xencommons.in
@@ -34,7 +34,7 @@ xen_startcmd()
 	local time=0
 	local timeout=30
 
-	mkdir -p "@XEN_RUN_DIR@"
+	mkdir -p "${XEN_RUN_DIR}"
 	xenstored_pid=$(check_pidfile ${XENSTORED_PIDFILE} ${XENSTORED})
 	if test -z "$xenstored_pid"; then
 		printf "Starting xenservices: xenstored, xenconsoled."
--- a/tools/hotplug/FreeBSD/rc.d/xendriverdomain.in
+++ b/tools/hotplug/FreeBSD/rc.d/xendriverdomain.in
@@ -27,7 +27,7 @@ xendriverdomain_start()
 {
 	printf "Starting xenservices: xl devd."
 
-	mkdir -p "@XEN_RUN_DIR@"
+	mkdir -p "${XEN_RUN_DIR}"
 	PATH="${bindir}:${sbindir}:$PATH" ${sbindir}/xl devd --pidfile ${XLDEVD_PIDFILE} ${XLDEVD_ARGS}
 
 	printf "\n"
--- a/tools/hotplug/Linux/init.d/xendriverdomain.in
+++ b/tools/hotplug/Linux/init.d/xendriverdomain.in
@@ -49,7 +49,7 @@ fi
 
 do_start () {
 	echo Starting xl devd...
-	mkdir -m700 -p @XEN_RUN_DIR@
+	mkdir -p "${XEN_RUN_DIR}"
 	${sbindir}/xl devd --pidfile=$XLDEVD_PIDFILE $XLDEVD_ARGS
 }
 do_stop () {
--- a/tools/hotplug/NetBSD/rc.d/xendriverdomain.in
+++ b/tools/hotplug/NetBSD/rc.d/xendriverdomain.in
@@ -23,7 +23,7 @@ XLDEVD_PIDFILE="@XEN_RUN_DIR@/xldevd.pid"
 
 xendriverdomain_precmd()
 {
-	mkdir -p "@XEN_RUN_DIR@"
+	mkdir -p "${XEN_RUN_DIR}"
 }
 
 xendriverdomain_startcmd()

Re: [PATCH v2] Fix install.sh for systemd

Posted by Andrew Cooper 11 months, 3 weeks ago

On 12/05/2023 12:18 pm, Olaf Hering wrote:
> Tue, 9 May 2023 13:47:11 +0100 Andrew Cooper <andrew.cooper3@citrix.com>:
>
>> Why is this 700, and the others just using regular perms?
>> Also, doesn't it want quoting like the other examples too?
> It is not clear why there is a single mkdir -m 0700 in the tree.
> Most likely it will not give any extra security.

I agree.  It's weird and doesn't have a good reason for being different.

> The scripts source hotplug.sh, which defines a variable XEN_RUN_DIR.
> I think it is better to use the shell variable instead of hardcoded paths.

Sounds good.  Does this allow for making any of these files no longer
preprocessed by ./configure ?  (i.e. cease being .in files)

> Regarding quoting: there are many paths used without quoting.
> For the beauty an additional (huge) change could be done to quote
> everything. Not sure if it is worth the effort...

Perhaps, but variables should always be quoted.  At least make sure that
new additions (and edits) leave things quoted.

Thanks,

~Andrew

Re: [PATCH v2] Fix install.sh for systemd

Posted by Olaf Hering 11 months, 3 weeks ago

Fri, 12 May 2023 12:22:08 +0100 Andrew Cooper <andrew.cooper3@citrix.com>:

> Does this allow for making any of these files no longer
> preprocessed by ./configure ?  (i.e. cease being .in files)

The path to hotplugpath.sh is variable, so each consumer needs to be a .in file.


Olaf

Re: [PATCH v2] Fix install.sh for systemd

Posted by Olaf Hering 12 months ago

Tue, 9 May 2023 13:47:11 +0100 Andrew Cooper <andrew.cooper3@citrix.com>:

> > +++ b/tools/hotplug/Linux/init.d/xendriverdomain.in
> > @@ -49,6 +49,7 @@ fi
> >  
> >  do_start () {
> >  	echo Starting xl devd...
> > +	mkdir -m700 -p @XEN_RUN_DIR@  
> 
> Why is this 700, and the others just using regular perms?

I think the permissions are just copy&paste from elsewhere.
I have to check why -m700 is used anyway, and why it is not used consistently.

> Also, doesn't it want quoting like the other examples too?

There is a mix of quoting and non-quoting. Not sure if having spaces in any of the path names does actually work today.

I will double check this detail as well.

Olaf