Message ID | 20170921070234.7865-1-christian.storm@siemens.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [1/2] systemd support | expand |
Hi Christian, On 21/09/2017 09:02, Christian Storm wrote: > Enable support for systemd's start-up completion notification > and, optionally, socket-based activation. > > Signed-off-by: Christian Storm <christian.storm@siemens.com> > --- > Kconfig | 7 ++++++ > Makefile.flags | 4 ++++ > core/swupdate.c | 10 ++++++++ > corelib/network_thread.c | 58 ++++++++++++++++++++++++++++++++++++--------- > corelib/progress_thread.c | 13 ++++++++++ > doc/source/swupdate.rst | 60 +++++++++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 141 insertions(+), 11 deletions(-) > > diff --git a/Kconfig b/Kconfig > index f90d218..c6dde7f 100644 > --- a/Kconfig > +++ b/Kconfig > @@ -61,6 +61,13 @@ menu "Swupdate Settings" > > menu "General Configuration" > > +config SYSTEMD > + bool "enable systemd support" > + default n > + help > + Enable support for systemd's start-up completion > + notification and socket-based activation features. > + Anyway, I guess we need a modification of swupdate.class in meta-swupdate to fix DEPENDS. Is your plan to fix it after this will be merged ? > config SCRIPTS > bool "enable pre and postinstall scripts" > default y > diff --git a/Makefile.flags b/Makefile.flags > index 391fc7f..3dbf379 100644 > --- a/Makefile.flags > +++ b/Makefile.flags > @@ -165,6 +165,10 @@ ifeq ($(CONFIG_UBOOT),y) > LDLIBS += z ubootenv > endif > > +ifeq ($(CONFIG_SYSTEMD),y) > +LDLIBS += systemd > +endif > + > # suricatta > ifneq ($(CONFIG_SURICATTA),) > ifneq ($(CONFIG_SURICATTA_SSL),) > diff --git a/core/swupdate.c b/core/swupdate.c > index e35289e..ef94bd9 100644 > --- a/core/swupdate.c > +++ b/core/swupdate.c > @@ -59,6 +59,10 @@ > #include "pctl.h" > #include "bootloader.h" > > +#ifdef CONFIG_SYSTEMD > +#include <systemd/sd-daemon.h> > +#endif > + > #define MODULE_NAME "swupdate" > > static pthread_t network_daemon; > @@ -864,6 +868,12 @@ int main(int argc, char **argv) > } > } > > +#ifdef CONFIG_SYSTEMD > + if (sd_booted()) { > + sd_notify(0, "READY=1"); > + } > +#endif > + > /* > * Install a handler for SIGTERM that cancels > * the network_daemon thread to allow atexit() > diff --git a/corelib/network_thread.c b/corelib/network_thread.c > index 5ed516e..1ddf917 100644 > --- a/corelib/network_thread.c > +++ b/corelib/network_thread.c > @@ -44,6 +44,10 @@ > #include "pctl.h" > #include "generated/autoconf.h" > > +#ifdef CONFIG_SYSTEMD > +#include <systemd/sd-daemon.h> > +#endif > + > #define LISTENQ 1024 > > #define NUM_CACHED_MESSAGES 100 > @@ -109,20 +113,43 @@ static void network_notifier(RECOVERY_STATUS status, int error, const char *msg) > int listener_create(const char *path, int type) > { > struct sockaddr_un servaddr; > - int listenfd; > - > - listenfd = socket(AF_LOCAL, type, 0); > - unlink(path); > - bzero(&servaddr, sizeof(servaddr)); > - servaddr.sun_family = AF_LOCAL; > - strcpy(servaddr.sun_path, path); > + int listenfd = -1; > > - if (bind(listenfd, (struct sockaddr *) &servaddr, sizeof(servaddr)) < 0) { > - close(listenfd); > - return -1; > +#ifdef CONFIG_SYSTEMD > + if (sd_booted()) { > + for (int fd = SD_LISTEN_FDS_START; fd < SD_LISTEN_FDS_START + sd_listen_fds(0); fd++) { > + if (sd_is_socket_unix(fd, SOCK_STREAM, 1, (char*)CONFIG_SOCKET_PROGRESS_PATH, 0)) { > + listenfd = fd; > + break; It is not clear to me why you drop the parameter (char *path) and you iterates with each CONFIG_ socket. Should we not use the following, instead ? for (int fd = SD_LISTEN_FDS_START; fd < SD_LISTEN_FDS_START + sd_listen_fds(0); fd++) { if (sd_is_socket_unix(fd, SOCK_STREAM, 1, path, 0)) { listenfd = fd; break; > + } else > + if (sd_is_socket_unix(fd, SOCK_STREAM, 1, (char*)CONFIG_SOCKET_CTRL_PATH, 0)) { > + listenfd = fd; > + break; > + } > + } > + if (listenfd == -1) { > + TRACE("got no socket at %s from systemd", path); > + } else { > + TRACE("got socket fd=%d at %s from systemd", listenfd, path); > + } > } > +#endif > > - chmod(path, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH); > + if (listenfd == -1) { > + TRACE("creating socket at %s", path); > + listenfd = socket(AF_LOCAL, type, 0); > + unlink(path); > + bzero(&servaddr, sizeof(servaddr)); > + servaddr.sun_family = AF_LOCAL; > + strcpy(servaddr.sun_path, path); > + > + if (bind(listenfd, (struct sockaddr *) &servaddr, sizeof(servaddr)) < 0) { > + close(listenfd); > + return -1; > + } > + > + chmod(path, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH); > + } > > if (type == SOCK_STREAM) > if (listen(listenfd, LISTENQ) < 0) { > @@ -173,6 +200,15 @@ static void empty_pipe(int fd) > > static void unlink_socket(void) > { > +#ifdef CONFIG_SYSTEMD > + if (sd_booted() && sd_listen_fds(0) > 0) { > + /* > + * There were socket fds handed-over by systemd, > + * so don't delete the socket file. > + */ > + return; > + } > +#endif > unlink((char*)CONFIG_SOCKET_CTRL_PATH); > } > > diff --git a/corelib/progress_thread.c b/corelib/progress_thread.c > index e0a11fe..d215166 100644 > --- a/corelib/progress_thread.c > +++ b/corelib/progress_thread.c > @@ -43,6 +43,10 @@ > #include <progress.h> > #include "generated/autoconf.h" > > +#ifdef CONFIG_SYSTEMD > +#include <systemd/sd-daemon.h> > +#endif > + > struct progress_conn { > SIMPLEQ_ENTRY(progress_conn) next; > int sockfd; > @@ -185,6 +189,15 @@ void swupdate_progress_done(const char *info) > > static void unlink_socket(void) > { > +#ifdef CONFIG_SYSTEMD > + if (sd_booted() && sd_listen_fds(0) > 0) { > + /* > + * There were socket fds handed-over by systemd, > + * so don't delete the socket file. > + */ > + return; > + } > +#endif > unlink((char*)CONFIG_SOCKET_PROGRESS_PATH); > } > > diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst > index 4df0b6b..5302e04 100644 > --- a/doc/source/swupdate.rst > +++ b/doc/source/swupdate.rst > @@ -504,6 +504,66 @@ Command line parameters > | | | downloading | > +-------------+----------+--------------------------------------------+ > > + > +systemd Integration > +------------------- > + > +SWUpdate has optional systemd_ support via the compile-time > +configuration switch ``CONFIG_SYSTEMD``. If enabled, SWUpdate > +signals systemd about start-up completion and can make optional > +use of systemd's socket-based activation feature. > + > +A sample systemd service unit file ``/etc/systemd/system/swupdate.service`` > +may look like the following starting SWUpdate in suricatta daemon mode: > + > +:: > + > + [Unit] > + Description=SWUpdate daemon > + Documentation=https://github.com/sbabic/swupdate > + Documentation=https://sbabic.github.io/swupdate > + > + [Service] > + Type=notify > + ExecStart=/usr/bin/swupdate -u '-t default -u http://localhost -i 25' > + > + [Install] > + WantedBy=multi-user.target > + > +Started via ``systemctl start swupdate.service``, SWUpdate > +(re)creates its sockets on startup. For using socket-based > +activation, an accompanying systemd socket unit file > +``/etc/systemd/system/swupdate.socket`` is required: > + > +:: > + > + [Unit] > + Description=SWUpdate socket listener > + Documentation=https://github.com/sbabic/swupdate > + Documentation=https://sbabic.github.io/swupdate > + > + [Socket] > + ListenStream=/tmp/sockinstctrl > + ListenStream=/tmp/swupdateprog > + > + [Install] > + WantedBy=sockets.target > + > +On ``swupdate.socket`` being started, systemd creates the socket > +files and hands them over to SWUpdate when it starts. So, for > +example, when talking to ``/tmp/swupdateprog``, systemd starts > +``swupdate.service`` and hands-over the socket files. The socket > +files are also handed over on a "regular" start of SWUpdate via > +``systemctl start swupdate.service``. > + > +Note that the socket paths in the two ``ListenStream=`` directives > +have to match the socket paths ``CONFIG_SOCKET_CTRL_PATH`` and > +``CONFIG_SOCKET_PROGRESS_PATH`` in SWUpdate's configuration. > +Here, the default socket path configuration is depicted. > + > +.. _systemd: https://www.freedesktop.org/wiki/Software/systemd/ > + > + > Changes in boot-loader code > =========================== > > Best regards, Stefano
Hi Stefano, > > Enable support for systemd's start-up completion notification > > and, optionally, socket-based activation. > > > > Signed-off-by: Christian Storm <christian.storm@siemens.com> > > --- > > Kconfig | 7 ++++++ > > Makefile.flags | 4 ++++ > > core/swupdate.c | 10 ++++++++ > > corelib/network_thread.c | 58 ++++++++++++++++++++++++++++++++++++--------- > > corelib/progress_thread.c | 13 ++++++++++ > > doc/source/swupdate.rst | 60 +++++++++++++++++++++++++++++++++++++++++++++++ > > 6 files changed, 141 insertions(+), 11 deletions(-) > > > > diff --git a/Kconfig b/Kconfig > > index f90d218..c6dde7f 100644 > > --- a/Kconfig > > +++ b/Kconfig > > @@ -61,6 +61,13 @@ menu "Swupdate Settings" > > > > menu "General Configuration" > > > > +config SYSTEMD > > + bool "enable systemd support" > > + default n > > + help > > + Enable support for systemd's start-up completion > > + notification and socket-based activation features. > > + > > Anyway, I guess we need a modification of swupdate.class in > meta-swupdate to fix DEPENDS. Is your plan to fix it after this will be > merged ? Well, not until now, that is :) But you're of course right, DEPENDS has to be adapted. I'll take a look... > > config SCRIPTS > > bool "enable pre and postinstall scripts" > > default y > > diff --git a/Makefile.flags b/Makefile.flags > > index 391fc7f..3dbf379 100644 > > --- a/Makefile.flags > > +++ b/Makefile.flags > > @@ -165,6 +165,10 @@ ifeq ($(CONFIG_UBOOT),y) > > LDLIBS += z ubootenv > > endif > > > > +ifeq ($(CONFIG_SYSTEMD),y) > > +LDLIBS += systemd > > +endif > > + > > # suricatta > > ifneq ($(CONFIG_SURICATTA),) > > ifneq ($(CONFIG_SURICATTA_SSL),) > > diff --git a/core/swupdate.c b/core/swupdate.c > > index e35289e..ef94bd9 100644 > > --- a/core/swupdate.c > > +++ b/core/swupdate.c > > @@ -59,6 +59,10 @@ > > #include "pctl.h" > > #include "bootloader.h" > > > > +#ifdef CONFIG_SYSTEMD > > +#include <systemd/sd-daemon.h> > > +#endif > > + > > #define MODULE_NAME "swupdate" > > > > static pthread_t network_daemon; > > @@ -864,6 +868,12 @@ int main(int argc, char **argv) > > } > > } > > > > +#ifdef CONFIG_SYSTEMD > > + if (sd_booted()) { > > + sd_notify(0, "READY=1"); > > + } > > +#endif > > + > > /* > > * Install a handler for SIGTERM that cancels > > * the network_daemon thread to allow atexit() > > diff --git a/corelib/network_thread.c b/corelib/network_thread.c > > index 5ed516e..1ddf917 100644 > > --- a/corelib/network_thread.c > > +++ b/corelib/network_thread.c > > @@ -44,6 +44,10 @@ > > #include "pctl.h" > > #include "generated/autoconf.h" > > > > +#ifdef CONFIG_SYSTEMD > > +#include <systemd/sd-daemon.h> > > +#endif > > + > > #define LISTENQ 1024 > > > > #define NUM_CACHED_MESSAGES 100 > > @@ -109,20 +113,43 @@ static void network_notifier(RECOVERY_STATUS status, int error, const char *msg) > > int listener_create(const char *path, int type) > > { > > struct sockaddr_un servaddr; > > - int listenfd; > > - > > - listenfd = socket(AF_LOCAL, type, 0); > > - unlink(path); > > - bzero(&servaddr, sizeof(servaddr)); > > - servaddr.sun_family = AF_LOCAL; > > - strcpy(servaddr.sun_path, path); > > + int listenfd = -1; > > > > - if (bind(listenfd, (struct sockaddr *) &servaddr, sizeof(servaddr)) < 0) { > > - close(listenfd); > > - return -1; > > +#ifdef CONFIG_SYSTEMD > > + if (sd_booted()) { > > + for (int fd = SD_LISTEN_FDS_START; fd < SD_LISTEN_FDS_START + sd_listen_fds(0); fd++) { > > + if (sd_is_socket_unix(fd, SOCK_STREAM, 1, (char*)CONFIG_SOCKET_PROGRESS_PATH, 0)) { > > + listenfd = fd; > > + break; > > It is not clear to me why you drop the parameter (char *path) and you > iterates with each CONFIG_ socket. Should we not use the following, > instead ? > > for (int fd = SD_LISTEN_FDS_START; fd < SD_LISTEN_FDS_START + > sd_listen_fds(0); fd++) { > if (sd_is_socket_unix(fd, SOCK_STREAM, 1, path, 0)) { > listenfd = fd; > break; Yes, absolutely right, we could do so. The reason is I wanted to be explicit about which fds SWUpdate takes from systemd. But I guess I don't have to as you're OK with it :) I'll send a v2 of this patch... Kind regards, Christian
diff --git a/Kconfig b/Kconfig index f90d218..c6dde7f 100644 --- a/Kconfig +++ b/Kconfig @@ -61,6 +61,13 @@ menu "Swupdate Settings" menu "General Configuration" +config SYSTEMD + bool "enable systemd support" + default n + help + Enable support for systemd's start-up completion + notification and socket-based activation features. + config SCRIPTS bool "enable pre and postinstall scripts" default y diff --git a/Makefile.flags b/Makefile.flags index 391fc7f..3dbf379 100644 --- a/Makefile.flags +++ b/Makefile.flags @@ -165,6 +165,10 @@ ifeq ($(CONFIG_UBOOT),y) LDLIBS += z ubootenv endif +ifeq ($(CONFIG_SYSTEMD),y) +LDLIBS += systemd +endif + # suricatta ifneq ($(CONFIG_SURICATTA),) ifneq ($(CONFIG_SURICATTA_SSL),) diff --git a/core/swupdate.c b/core/swupdate.c index e35289e..ef94bd9 100644 --- a/core/swupdate.c +++ b/core/swupdate.c @@ -59,6 +59,10 @@ #include "pctl.h" #include "bootloader.h" +#ifdef CONFIG_SYSTEMD +#include <systemd/sd-daemon.h> +#endif + #define MODULE_NAME "swupdate" static pthread_t network_daemon; @@ -864,6 +868,12 @@ int main(int argc, char **argv) } } +#ifdef CONFIG_SYSTEMD + if (sd_booted()) { + sd_notify(0, "READY=1"); + } +#endif + /* * Install a handler for SIGTERM that cancels * the network_daemon thread to allow atexit() diff --git a/corelib/network_thread.c b/corelib/network_thread.c index 5ed516e..1ddf917 100644 --- a/corelib/network_thread.c +++ b/corelib/network_thread.c @@ -44,6 +44,10 @@ #include "pctl.h" #include "generated/autoconf.h" +#ifdef CONFIG_SYSTEMD +#include <systemd/sd-daemon.h> +#endif + #define LISTENQ 1024 #define NUM_CACHED_MESSAGES 100 @@ -109,20 +113,43 @@ static void network_notifier(RECOVERY_STATUS status, int error, const char *msg) int listener_create(const char *path, int type) { struct sockaddr_un servaddr; - int listenfd; - - listenfd = socket(AF_LOCAL, type, 0); - unlink(path); - bzero(&servaddr, sizeof(servaddr)); - servaddr.sun_family = AF_LOCAL; - strcpy(servaddr.sun_path, path); + int listenfd = -1; - if (bind(listenfd, (struct sockaddr *) &servaddr, sizeof(servaddr)) < 0) { - close(listenfd); - return -1; +#ifdef CONFIG_SYSTEMD + if (sd_booted()) { + for (int fd = SD_LISTEN_FDS_START; fd < SD_LISTEN_FDS_START + sd_listen_fds(0); fd++) { + if (sd_is_socket_unix(fd, SOCK_STREAM, 1, (char*)CONFIG_SOCKET_PROGRESS_PATH, 0)) { + listenfd = fd; + break; + } else + if (sd_is_socket_unix(fd, SOCK_STREAM, 1, (char*)CONFIG_SOCKET_CTRL_PATH, 0)) { + listenfd = fd; + break; + } + } + if (listenfd == -1) { + TRACE("got no socket at %s from systemd", path); + } else { + TRACE("got socket fd=%d at %s from systemd", listenfd, path); + } } +#endif - chmod(path, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH); + if (listenfd == -1) { + TRACE("creating socket at %s", path); + listenfd = socket(AF_LOCAL, type, 0); + unlink(path); + bzero(&servaddr, sizeof(servaddr)); + servaddr.sun_family = AF_LOCAL; + strcpy(servaddr.sun_path, path); + + if (bind(listenfd, (struct sockaddr *) &servaddr, sizeof(servaddr)) < 0) { + close(listenfd); + return -1; + } + + chmod(path, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH); + } if (type == SOCK_STREAM) if (listen(listenfd, LISTENQ) < 0) { @@ -173,6 +200,15 @@ static void empty_pipe(int fd) static void unlink_socket(void) { +#ifdef CONFIG_SYSTEMD + if (sd_booted() && sd_listen_fds(0) > 0) { + /* + * There were socket fds handed-over by systemd, + * so don't delete the socket file. + */ + return; + } +#endif unlink((char*)CONFIG_SOCKET_CTRL_PATH); } diff --git a/corelib/progress_thread.c b/corelib/progress_thread.c index e0a11fe..d215166 100644 --- a/corelib/progress_thread.c +++ b/corelib/progress_thread.c @@ -43,6 +43,10 @@ #include <progress.h> #include "generated/autoconf.h" +#ifdef CONFIG_SYSTEMD +#include <systemd/sd-daemon.h> +#endif + struct progress_conn { SIMPLEQ_ENTRY(progress_conn) next; int sockfd; @@ -185,6 +189,15 @@ void swupdate_progress_done(const char *info) static void unlink_socket(void) { +#ifdef CONFIG_SYSTEMD + if (sd_booted() && sd_listen_fds(0) > 0) { + /* + * There were socket fds handed-over by systemd, + * so don't delete the socket file. + */ + return; + } +#endif unlink((char*)CONFIG_SOCKET_PROGRESS_PATH); } diff --git a/doc/source/swupdate.rst b/doc/source/swupdate.rst index 4df0b6b..5302e04 100644 --- a/doc/source/swupdate.rst +++ b/doc/source/swupdate.rst @@ -504,6 +504,66 @@ Command line parameters | | | downloading | +-------------+----------+--------------------------------------------+ + +systemd Integration +------------------- + +SWUpdate has optional systemd_ support via the compile-time +configuration switch ``CONFIG_SYSTEMD``. If enabled, SWUpdate +signals systemd about start-up completion and can make optional +use of systemd's socket-based activation feature. + +A sample systemd service unit file ``/etc/systemd/system/swupdate.service`` +may look like the following starting SWUpdate in suricatta daemon mode: + +:: + + [Unit] + Description=SWUpdate daemon + Documentation=https://github.com/sbabic/swupdate + Documentation=https://sbabic.github.io/swupdate + + [Service] + Type=notify + ExecStart=/usr/bin/swupdate -u '-t default -u http://localhost -i 25' + + [Install] + WantedBy=multi-user.target + +Started via ``systemctl start swupdate.service``, SWUpdate +(re)creates its sockets on startup. For using socket-based +activation, an accompanying systemd socket unit file +``/etc/systemd/system/swupdate.socket`` is required: + +:: + + [Unit] + Description=SWUpdate socket listener + Documentation=https://github.com/sbabic/swupdate + Documentation=https://sbabic.github.io/swupdate + + [Socket] + ListenStream=/tmp/sockinstctrl + ListenStream=/tmp/swupdateprog + + [Install] + WantedBy=sockets.target + +On ``swupdate.socket`` being started, systemd creates the socket +files and hands them over to SWUpdate when it starts. So, for +example, when talking to ``/tmp/swupdateprog``, systemd starts +``swupdate.service`` and hands-over the socket files. The socket +files are also handed over on a "regular" start of SWUpdate via +``systemctl start swupdate.service``. + +Note that the socket paths in the two ``ListenStream=`` directives +have to match the socket paths ``CONFIG_SOCKET_CTRL_PATH`` and +``CONFIG_SOCKET_PROGRESS_PATH`` in SWUpdate's configuration. +Here, the default socket path configuration is depicted. + +.. _systemd: https://www.freedesktop.org/wiki/Software/systemd/ + + Changes in boot-loader code ===========================
Enable support for systemd's start-up completion notification and, optionally, socket-based activation. Signed-off-by: Christian Storm <christian.storm@siemens.com> --- Kconfig | 7 ++++++ Makefile.flags | 4 ++++ core/swupdate.c | 10 ++++++++ corelib/network_thread.c | 58 ++++++++++++++++++++++++++++++++++++--------- corelib/progress_thread.c | 13 ++++++++++ doc/source/swupdate.rst | 60 +++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 141 insertions(+), 11 deletions(-)