[PATCH] Fix install.sh for systemd

[PATCH] Fix install.sh for systemd

[Jason Andryuk]

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 when systemd is being used.  systemd
expects an empty /run on boot and works properly without one.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 tools/Makefile | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/Makefile b/tools/Makefile
index 4906fdbc23..32c8b0a2a2 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -58,9 +58,11 @@ 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)
+ifneq ($(CONFIG_SYSTEMD),y)
+	$(INSTALL_DIR) $(DESTDIR)$(XEN_RUN_DIR)
 	$(INSTALL_DIR) $(DESTDIR)$(XEN_RUN_STORED)
+endif
 	$(INSTALL_DIR) $(DESTDIR)$(PKG_INSTALLDIR)
 	$(MAKE) subdirs-install
 
-- 
2.40.0

[PATCH] libxl: device_backend_callback() print rc on error

[Jason Andryuk]

Print the rc when an error is found in device_backend_callback() so the
user can have some idea of why things went wrong.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 tools/libs/light/libxl_device.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/libs/light/libxl_device.c b/tools/libs/light/libxl_device.c
index a75c21d433..13da6e0573 100644
--- a/tools/libs/light/libxl_device.c
+++ b/tools/libs/light/libxl_device.c
@@ -1160,9 +1160,10 @@ static void device_backend_callback(libxl__egc *egc, libxl__ev_devstate *ds,
     }
 
     if (rc) {
-        LOGD(ERROR, aodev->dev->domid, "unable to %s device with path %s",
+        LOGD(ERROR, aodev->dev->domid,
+                    "unable to %s device with path %s - rc %d",
                     libxl__device_action_to_string(aodev->action),
-                    libxl__device_backend_path(gc, aodev->dev));
+                    libxl__device_backend_path(gc, aodev->dev), rc);
         goto out;
     }
 
-- 
2.40.0

[PATCH] libxl: Print device_kind as a string

[Jason Andryuk]

Printing the integer isn't particularly informative.  Switch to a
human-readable string when printing the device_kind in
libxl__get_hotplug_script_info().

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
 tools/libs/light/libxl_linux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libs/light/libxl_linux.c b/tools/libs/light/libxl_linux.c
index 27f2bce718..f7c92ba562 100644
--- a/tools/libs/light/libxl_linux.c
+++ b/tools/libs/light/libxl_linux.c
@@ -231,8 +231,8 @@ int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
         break;
     default:
         /* No need to execute any hotplug scripts */
-        LOGD(DEBUG, dev->domid,
-             "backend_kind %d, no need to execute scripts", dev->backend_kind);
+        LOGD(DEBUG, dev->domid, "backend_kind %s, no need to execute scripts",
+             libxl__device_kind_to_string(dev->backend_kind));
         rc = 0;
         break;
     }
-- 
2.40.0

Re: [PATCH] libxl: device_backend_callback() print rc on error

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 289 bytes --]

On 25.04.23 21:46, Jason Andryuk wrote:
> Print the rc when an error is found in device_backend_callback() so the
> user can have some idea of why things went wrong.
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] libxl: Print device_kind as a string

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 325 bytes --]

On 25.04.23 21:46, Jason Andryuk wrote:
> Printing the integer isn't particularly informative.  Switch to a
> human-readable string when printing the device_kind in
> libxl__get_hotplug_script_info().
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] Fix install.sh for systemd

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 248 bytes --]

Tue, 25 Apr 2023 15:46:20 -0400 Jason Andryuk <jandryuk@gmail.com>:

> Skip populating /var/run/xen when systemd is being used.

It was wrong to do it like that from day one.
Such directory has to be populated on demand at runtime.


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fix install.sh for systemd

[Jan Beulich]

On 26.04.2023 09:15, Olaf Hering wrote:
> Tue, 25 Apr 2023 15:46:20 -0400 Jason Andryuk <jandryuk@gmail.com>:
> 
>> Skip populating /var/run/xen when systemd is being used.
> 
> It was wrong to do it like that from day one.
> Such directory has to be populated on demand at runtime.

Is this to be translated to Reviewed-by: <you>?

Jan

Re: [PATCH] libxl: device_backend_callback() print rc on error

[Jan Beulich]

On 25.04.2023 21:46, Jason Andryuk wrote:
> Print the rc when an error is found in device_backend_callback() so the
> user can have some idea of why things went wrong.
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> ---
>  tools/libs/light/libxl_device.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

While patches which are part of a series should be sent as replies to the
cover letter, may I ask that you do not send individual patches as replies
to other (unrelated) patches (or, in general, really as replies to anything,
i.e. also not as replies to e.g. an earlier discussion)?

Thanks, Jan

Re: [PATCH] Fix install.sh for systemd

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

Wed, 26 Apr 2023 10:28:38 +0200 Jan Beulich <jbeulich@suse.com>:

> Is this to be translated to Reviewed-by: <you>?

It was a Nack, in case such thing exists, and if it has any meaning if sent by me.

There are a few places already which do a mkdir -p XEN_RUN_DIR prior usage.
But a few are missing.
XEN_RUN_DIR and most likely also XEN_RUN_STORED have to be removed from make install.


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Fix install.sh for systemd

[Jan Beulich]

On 26.04.2023 10:47, Olaf Hering wrote:
> Wed, 26 Apr 2023 10:28:38 +0200 Jan Beulich <jbeulich@suse.com>:
> 
>> Is this to be translated to Reviewed-by: <you>?
> 
> It was a Nack, in case such thing exists, and if it has any meaning if sent by me.

Now I'm confused: Your reply didn't read like a nack at all, at least to
me. Furthermore ...

> There are a few places already which do a mkdir -p XEN_RUN_DIR prior usage.
> But a few are missing.
> XEN_RUN_DIR and most likely also XEN_RUN_STORED have to be removed from make install.

... this suggests to me that you really mean the change doesn't go far
enough, but that's then different from nack-ing a change. Can you please
clarify this for me (and maybe also for Jason, depending on how he has
read your replies)?

Jan

Re: [PATCH] Fix install.sh for systemd

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 3196 bytes --]

Wed, 26 Apr 2023 11:07:17 +0200 Jan Beulich <jbeulich@suse.com>:

> On 26.04.2023 10:47, Olaf Hering wrote:
> > XEN_RUN_DIR and most likely also XEN_RUN_STORED have to be removed from make install.  
> ... this suggests to me that you really mean the change doesn't go far
> enough, but that's then different from nack-ing a change. Can you please
> clarify this for me (and maybe also for Jason, depending on how he has
> read your replies)?

I think the change should look like this, the runtime directories have to be created at runtime.

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

--- 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
 
--- 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."
--- 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"
--- 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 () {
--- 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]
--- 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()


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] libxl: device_backend_callback() print rc on error

[Jason Andryuk]

On Wed, Apr 26, 2023 at 4:39 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 25.04.2023 21:46, Jason Andryuk wrote:
> > Print the rc when an error is found in device_backend_callback() so the
> > user can have some idea of why things went wrong.
> >
> > Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> > ---
> >  tools/libs/light/libxl_device.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
>
> While patches which are part of a series should be sent as replies to the
> cover letter, may I ask that you do not send individual patches as replies
> to other (unrelated) patches (or, in general, really as replies to anything,
> i.e. also not as replies to e.g. an earlier discussion)?

Certainly.  Sorry about that.  I formatted the patches individually,
but sent them with a single git send-email command.  Looks like I
should have added --no-thread to have them sent individually.

Regards,
Jason

Re: [PATCH] Fix install.sh for systemd

[Jason Andryuk]

On Wed, Apr 26, 2023 at 6:40 AM Olaf Hering <olaf@aepfle.de> wrote:
>
> Wed, 26 Apr 2023 11:07:17 +0200 Jan Beulich <jbeulich@suse.com>:
>
> > On 26.04.2023 10:47, Olaf Hering wrote:
> > > XEN_RUN_DIR and most likely also XEN_RUN_STORED have to be removed from make install.
> > ... this suggests to me that you really mean the change doesn't go far
> > enough, but that's then different from nack-ing a change. Can you please
> > clarify this for me (and maybe also for Jason, depending on how he has
> > read your replies)?
>
> I think the change should look like this, the runtime directories have to be created at runtime.

Thanks, Olaf.  Yes, I think your approach is better.  Will you submit
it as a formal patch?  I'm happy to test it.

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

This one should be "@XEN_RUN_DIR@"?

Thanks,
Jason

Re: [PATCH] Fix install.sh for systemd

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 591 bytes --]

Wed, 26 Apr 2023 09:31:44 -0400 Jason Andryuk <jandryuk@gmail.com>:

> On Wed, Apr 26, 2023 at 6:40 AM Olaf Hering <olaf@aepfle.de> wrote:
> > +++ 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}  
> This one should be "@XEN_RUN_DIR@"?

I changed it, it was a result of copy&paste from another file.

Since hotplug.sh is included in many scripts, all such users could be converted to a shell variable. But this would be a separate change.


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]