Re: [PATCH 1/2] tools/{c,o}xenstored: Don't link against libsystemd
On 01/05/2024 2:29 pm, Anthony PERARD wrote: > On Fri, Apr 26, 2024 at 09:51:47AM +0100, Anthony PERARD wrote: >> Run `systemd-notify --ready` instead. Hopefully, that will be enough. >> ($NOTIFY_SOCKET is a socket, and a bit more complicated that I though, >> it can start with "@" for example) > FTR: If it turns out that calling systemd-notify binary isn't working > well enough, we could have an implementation of sd_notify() in our tree, > openssh are doing there own here: > https://bugzilla.mindrot.org/show_bug.cgi?id=2641 > and there's an example implementation on systemd's documentation: > https://www.freedesktop.org/software/systemd/man/devel/sd_notify.html#Notes > (Nothing for ocaml) > > But let's go with `systemd-notify --ready` as it is just easier to > write a bit of shell script. I was already thinking of going down the small-library-function route. Given that I miss-analysed the launch-xenstore, script, I'm not overly enthused with just falling back to waiting on the pidfile, because that's adding technical debt rather than removing it. ~Andrew
Re: [PATCH 1/2] tools/{c,o}xenstored: Don't link against libsystemd
On Fri, Apr 26, 2024 at 09:51:47AM +0100, Anthony PERARD wrote: > Run `systemd-notify --ready` instead. Hopefully, that will be enough. > ($NOTIFY_SOCKET is a socket, and a bit more complicated that I though, > it can start with "@" for example) FTR: If it turns out that calling systemd-notify binary isn't working well enough, we could have an implementation of sd_notify() in our tree, openssh are doing there own here: https://bugzilla.mindrot.org/show_bug.cgi?id=2641 and there's an example implementation on systemd's documentation: https://www.freedesktop.org/software/systemd/man/devel/sd_notify.html#Notes (Nothing for ocaml) But let's go with `systemd-notify --ready` as it is just easier to write a bit of shell script. Cheers, -- Anthony PERARD
Re: [PATCH 1/2] tools/{c,o}xenstored: Don't link against libsystemd
On 25.04.24 19:32, Andrew Cooper wrote:
libsystemd is a giant dependency for one single function, but in the wake of
the xz backdoor, it turns out that even systemd leadership recommend against
linking against libsystemd for sd_notify().
Since commit 7b61011e1450 ("tools: make xenstore domain easy configurable") in
Xen 4.8, the launch-xenstore script invokes systemd-notify directly, so its
not even necessary for the xenstored's to call sd_notify() themselves.
You are aware that in the daemon case the call of systemd-notify does not
signal readyness of xenstored? It is just called with the "--booted" parameter
in order to detect whether systemd is active or not.
So in order to just drop the sd_notify() call from xenstored you need to
modify the launch-xenstore script, too.
Juergen
Therefore, just drop the calls to sd_notify() and stop linking against
libsystemd.
No functional change.
Signed-off-by: Andrew Cooper
---
CC: Anthony PERARD
CC: Juergen Gross
CC: Christian Lindig
CC: Edwin Török
CC: Stefano Stabellini
---
tools/ocaml/xenstored/Makefile| 12 +--
tools/ocaml/xenstored/systemd.ml | 15 -
tools/ocaml/xenstored/systemd.mli | 16 -
tools/ocaml/xenstored/systemd_stubs.c | 47 ---
tools/ocaml/xenstored/xenstored.ml| 1 -
tools/xenstored/Makefile | 5 ---
tools/xenstored/posix.c | 9 -
7 files changed, 1 insertion(+), 104 deletions(-)
delete mode 100644 tools/ocaml/xenstored/systemd.ml
delete mode 100644 tools/ocaml/xenstored/systemd.mli
delete mode 100644 tools/ocaml/xenstored/systemd_stubs.c
diff --git a/tools/ocaml/xenstored/Makefile b/tools/ocaml/xenstored/Makefile
index e8aaecf2e630..1e4b51cc5432 100644
--- a/tools/ocaml/xenstored/Makefile
+++ b/tools/ocaml/xenstored/Makefile
@@ -4,8 +4,6 @@ include $(OCAML_TOPLEVEL)/common.make
# Include configure output (config.h)
CFLAGS += -include $(XEN_ROOT)/tools/config.h
-CFLAGS-$(CONFIG_SYSTEMD) += $(SYSTEMD_CFLAGS)
-LDFLAGS-$(CONFIG_SYSTEMD) += $(SYSTEMD_LIBS)
CFLAGS += $(CFLAGS-y)
CFLAGS += $(APPEND_CFLAGS)
@@ -25,13 +23,6 @@ poll_OBJS = poll
poll_C_OBJS = select_stubs
OCAML_LIBRARY = syslog poll
-LIBS += systemd.cma systemd.cmxa
-systemd_OBJS = systemd
-systemd_C_OBJS = systemd_stubs
-OCAML_LIBRARY += systemd
-
-LIBS_systemd += $(LDFLAGS-y)
-
OBJS = paths \
define \
stdext \
@@ -56,12 +47,11 @@ OBJS = paths \
process \
xenstored
-INTF = symbol.cmi trie.cmi syslog.cmi systemd.cmi poll.cmi
+INTF = symbol.cmi trie.cmi syslog.cmi poll.cmi
XENSTOREDLIBS = \
unix.cmxa \
-ccopt -L -ccopt . syslog.cmxa \
- -ccopt -L -ccopt . systemd.cmxa \
-ccopt -L -ccopt . poll.cmxa \
-ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/mmap
$(OCAML_TOPLEVEL)/libs/mmap/xenmmap.cmxa \
-ccopt -L -ccopt $(OCAML_TOPLEVEL)/libs/eventchn
$(OCAML_TOPLEVEL)/libs/eventchn/xeneventchn.cmxa \
diff --git a/tools/ocaml/xenstored/systemd.ml b/tools/ocaml/xenstored/systemd.ml
deleted file mode 100644
index 39127f712d72..
--- a/tools/ocaml/xenstored/systemd.ml
+++ /dev/null
@@ -1,15 +0,0 @@
-(*
- * Copyright (C) 2014 Luis R. Rodriguez
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU Lesser General Public License as published
- * by the Free Software Foundation; version 2.1 only. with the special
- * exception on linking described in file LICENSE.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU Lesser General Public License for more details.
- *)
-
-external sd_notify_ready: unit -> unit = "ocaml_sd_notify_ready"
diff --git a/tools/ocaml/xenstored/systemd.mli
b/tools/ocaml/xenstored/systemd.mli
deleted file mode 100644
index 18b9331031f9..
--- a/tools/ocaml/xenstored/systemd.mli
+++ /dev/null
@@ -1,16 +0,0 @@
-(*
- * Copyright (C) 2014 Luis R. Rodriguez
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU Lesser General Public License as published
- * by the Free Software Foundation; version 2.1 only. with the special
- * exception on linking described in file LICENSE.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU Lesser General Public License for more details.
- *)
-
-(** Tells systemd we're ready *)
-external sd_notify_ready: unit -> unit = "ocaml_sd_notify_ready"
diff --git a/tools/ocaml/xenstored/systemd_stubs.c
b/tools/ocaml/xenstored/systemd_stubs.c
deleted file mode 100644
index f4c875075abe..
--- a/tools/ocaml/xenstored/systemd_stubs.c
+++ /dev/null
@@ -1,47 +0,0 @@
-/*
Re: [PATCH 1/2] tools/{c,o}xenstored: Don't link against libsystemd
On 26/04/2024 9:51 am, Anthony PERARD wrote:
> On Thu, Apr 25, 2024 at 07:16:23PM +0100, Andrew Cooper wrote:
>> On 25/04/2024 7:06 pm, Anthony PERARD wrote:
>>> On Thu, Apr 25, 2024 at 06:32:15PM +0100, Andrew Cooper wrote:
libsystemd is a giant dependency for one single function, but in the wake
of
the xz backdoor, it turns out that even systemd leadership recommend
against
linking against libsystemd for sd_notify().
Since commit 7b61011e1450 ("tools: make xenstore domain easy
configurable") in
Xen 4.8, the launch-xenstore script invokes systemd-notify directly, so its
>>> That's not enough, it's needs to be `systemd-notify --ready` to be a
>>> replacement for sd_notify(READY=1).
>>>
not even necessary for the xenstored's to call sd_notify() themselves.
>>> So, sd_notify() or equivalent is still necessary.
>>>
Therefore, just drop the calls to sd_notify() and stop linking against
libsystemd.
>>> Sounds good, be we need to replace the call by something like:
>>> echo READY=1 > $NOTIFY_SOCKET
>>> implemented in C and ocaml. Detail to be checked.
>>>
>>> Otherwise, things won't work.
>> Hmm. It worked in XenRT when stripping this all out, but that is
> I don't know how XenServer is setup, maybe it doesn't matter?
In theory it's straight systemd, but I could also believe that Xapi
checks for the pidfile too.
> Anyway...
>
>> extremely unintuitive behaviour for `systemd-notify --booted`, seeing as
>> it's entirely different to --ready.
> Yes, this --booted option should probably not exist, and there's
> `systemctl is-system-running` that does something similar.
>
>> I've got no interest in keeping the C around, but if:
>>
>> [ -n "$NOTIFY_SOCKET" ] && echo READY=1 > $NOTIFY_SOCKET
>>
>> works, then can't we just use that after waiting for the the pidfile ?
> Run `systemd-notify --ready` instead. Hopefully, that will be enough.
> ($NOTIFY_SOCKET is a socket, and a bit more complicated that I though,
> it can start with "@" for example)
I'll do a prep patch to adjust launch-xenstore after which this patch
should be fine in this form (modulo a tweak in the commit message).
~Andrew
Re: [PATCH 1/2] tools/{c,o}xenstored: Don't link against libsystemd
On Thu, Apr 25, 2024 at 07:16:23PM +0100, Andrew Cooper wrote:
> On 25/04/2024 7:06 pm, Anthony PERARD wrote:
> > On Thu, Apr 25, 2024 at 06:32:15PM +0100, Andrew Cooper wrote:
> >> libsystemd is a giant dependency for one single function, but in the wake
> >> of
> >> the xz backdoor, it turns out that even systemd leadership recommend
> >> against
> >> linking against libsystemd for sd_notify().
> >>
> >> Since commit 7b61011e1450 ("tools: make xenstore domain easy
> >> configurable") in
> >> Xen 4.8, the launch-xenstore script invokes systemd-notify directly, so its
> > That's not enough, it's needs to be `systemd-notify --ready` to be a
> > replacement for sd_notify(READY=1).
> >
> >> not even necessary for the xenstored's to call sd_notify() themselves.
> > So, sd_notify() or equivalent is still necessary.
> >
> >> Therefore, just drop the calls to sd_notify() and stop linking against
> >> libsystemd.
> > Sounds good, be we need to replace the call by something like:
> > echo READY=1 > $NOTIFY_SOCKET
> > implemented in C and ocaml. Detail to be checked.
> >
> > Otherwise, things won't work.
>
> Hmm. It worked in XenRT when stripping this all out, but that is
I don't know how XenServer is setup, maybe it doesn't matter? Anyway...
> extremely unintuitive behaviour for `systemd-notify --booted`, seeing as
> it's entirely different to --ready.
Yes, this --booted option should probably not exist, and there's
`systemctl is-system-running` that does something similar.
>
> I've got no interest in keeping the C around, but if:
>
> [ -n "$NOTIFY_SOCKET" ] && echo READY=1 > $NOTIFY_SOCKET
>
> works, then can't we just use that after waiting for the the pidfile ?
Run `systemd-notify --ready` instead. Hopefully, that will be enough.
($NOTIFY_SOCKET is a socket, and a bit more complicated that I though,
it can start with "@" for example)
Cheers,
--
Anthony PERARD
Re: [PATCH 1/2] tools/{c,o}xenstored: Don't link against libsystemd
On 25/04/2024 7:06 pm, Anthony PERARD wrote:
> On Thu, Apr 25, 2024 at 06:32:15PM +0100, Andrew Cooper wrote:
>> libsystemd is a giant dependency for one single function, but in the wake of
>> the xz backdoor, it turns out that even systemd leadership recommend against
>> linking against libsystemd for sd_notify().
>>
>> Since commit 7b61011e1450 ("tools: make xenstore domain easy configurable")
>> in
>> Xen 4.8, the launch-xenstore script invokes systemd-notify directly, so its
> That's not enough, it's needs to be `systemd-notify --ready` to be a
> replacement for sd_notify(READY=1).
>
>> not even necessary for the xenstored's to call sd_notify() themselves.
> So, sd_notify() or equivalent is still necessary.
>
>> Therefore, just drop the calls to sd_notify() and stop linking against
>> libsystemd.
> Sounds good, be we need to replace the call by something like:
> echo READY=1 > $NOTIFY_SOCKET
> implemented in C and ocaml. Detail to be checked.
>
> Otherwise, things won't work.
Hmm. It worked in XenRT when stripping this all out, but that is
extremely unintuitive behaviour for `systemd-notify --booted`, seeing as
it's entirely different to --ready.
I've got no interest in keeping the C around, but if:
[ -n "$NOTIFY_SOCKET" ] && echo READY=1 > $NOTIFY_SOCKET
works, then can't we just use that after waiting for the the pidfile ?
~Andrew
Re: [PATCH 1/2] tools/{c,o}xenstored: Don't link against libsystemd
On Thu, Apr 25, 2024 at 06:32:15PM +0100, Andrew Cooper wrote:
> libsystemd is a giant dependency for one single function, but in the wake of
> the xz backdoor, it turns out that even systemd leadership recommend against
> linking against libsystemd for sd_notify().
>
> Since commit 7b61011e1450 ("tools: make xenstore domain easy configurable") in
> Xen 4.8, the launch-xenstore script invokes systemd-notify directly, so its
That's not enough, it's needs to be `systemd-notify --ready` to be a
replacement for sd_notify(READY=1).
> not even necessary for the xenstored's to call sd_notify() themselves.
So, sd_notify() or equivalent is still necessary.
> Therefore, just drop the calls to sd_notify() and stop linking against
> libsystemd.
Sounds good, be we need to replace the call by something like:
echo READY=1 > $NOTIFY_SOCKET
implemented in C and ocaml. Detail to be checked.
Otherwise, things won't work.
Thanks,
--
Anthony PERARD
