1. Patchew
  2. Xen
  3. View series

[PATCH] Fix install.sh for systemd

Jason Andryuk posted 1 patch 1 year ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20230425194622.114869-1-jandryuk@gmail.com
There is a newer version of this series
tools/Makefile | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
[PATCH] Fix install.sh for systemd
Posted by Jason Andryuk 1 year 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 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
Re: [PATCH] Fix install.sh for systemd
Posted by Olaf Hering 1 year ago 
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
Re: [PATCH] Fix install.sh for systemd
Posted by Jan Beulich 1 year ago 
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] Fix install.sh for systemd
Posted by Olaf Hering 1 year ago 
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
Re: [PATCH] Fix install.sh for systemd
Posted by Jan Beulich 1 year ago 
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
Posted by Olaf Hering 1 year ago 
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
Re: [PATCH] Fix install.sh for systemd
Posted by Jason Andryuk 1 year ago 
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
Posted by Olaf Hering 12 months ago 
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
[PATCH] libxl: device_backend_callback() print rc on error
Posted by Jason Andryuk 1 year ago 
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
Re: [PATCH] libxl: device_backend_callback() print rc on error
Posted by Jan Beulich 1 year ago 
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] libxl: device_backend_callback() print rc on error
Posted by Jason Andryuk 1 year ago 
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] libxl: device_backend_callback() print rc on error
Posted by Juergen Gross 1 year ago 
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
[PATCH] libxl: Print device_kind as a string
Posted by Jason Andryuk 1 year ago 
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: Print device_kind as a string
Posted by Juergen Gross 1 year ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.