| Message ID | 20231023022529.15081-5-felix.moessbauer@siemens.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Rework socket creation to better integrate with systemd | expand |
Hi Felix, On 23.10.23 04:25, 'Felix Moessbauer' via swupdate wrote: > When running on a systemd bootet system, the IPC sockets must be created > below /run to be in sync with sockets provided by systemd units. > > Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com> > --- > ipc/network_ipc.c | 10 +++++++++- > ipc/progress_ipc.c | 10 +++++++++- > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/ipc/network_ipc.c b/ipc/network_ipc.c > index 20332b3..435a579 100644 > --- a/ipc/network_ipc.c > +++ b/ipc/network_ipc.c > @@ -17,6 +17,10 @@ > #include "network_ipc.h" > #include "compat.h" > > +#ifdef CONFIG_SYSTEMD > +#include <systemd/sd-daemon.h> > +#endif > + > #ifdef CONFIG_SOCKET_CTRL_PATH > static char* SOCKET_CTRL_PATH = (char*)CONFIG_SOCKET_CTRL_PATH; > #else > @@ -30,7 +34,11 @@ char *get_ctrl_socket(void) { > const char *tmpdir = getenv("TMPDIR"); > if (!tmpdir) > tmpdir = "/tmp"; > - > +// on systemd systems /run is available and used for sockets created by systemd > +#ifdef CONFIG_SYSTEMD > + if(sd_booted()) > + tmpdir = "/run"; > +#endif No, this does not cover all use cases. Think about we have multiple instances of SWUpdate on the same device. This is a pattern when the device acts with different roles (gateway vs device is one of them), or when the update is split into domains, and an instance of SWUpdate can be set with lower rights if it is in charge to update just a part of the system. We need two or multiple sets of sockets, and they should never conflict. This is currently done by setting TMPDIR before launching SWUpdate, and in this case letting SWUpdate to create the sockets itself. Or an instance is maybe getting the sockets from systemd, the second one can create the sockets. It is still configurable, but if sockets have the same names, your change breaks this use case. Nevertheless, why do we need to hardcode the path into the code when we can set it before launching ? We can just set TMPDIR as /run in the service unit, and this is ok in most cases (swupdate -i will create the sockets in /tmp). It is also what systemd is doing, it uses env vars to pass the fds: it sets LISTEN_FDS to retrieve the list of fds. Best regards, Stefano > if (asprintf(&SOCKET_CTRL_PATH, "%s/%s", tmpdir, SOCKET_CTRL_DEFAULT) == -1) > return (char *)"/tmp/"SOCKET_CTRL_DEFAULT; > } > diff --git a/ipc/progress_ipc.c b/ipc/progress_ipc.c > index 745dc44..a0def5c 100644 > --- a/ipc/progress_ipc.c > +++ b/ipc/progress_ipc.c > @@ -14,6 +14,10 @@ > #include <unistd.h> > #include <stdbool.h> > > +#ifdef CONFIG_SYSTEMD > +#include <systemd/sd-daemon.h> > +#endif > + > #include <progress_ipc.h> > > #ifdef CONFIG_SOCKET_PROGRESS_PATH > @@ -29,7 +33,11 @@ char *get_prog_socket(void) { > const char *tmpdir = getenv("TMPDIR"); > if (!tmpdir) > tmpdir = "/tmp"; > - > +// on systemd systems /run is available and used for sockets created by systemd > +#ifdef CONFIG_SYSTEMD > + if(sd_booted()) > + tmpdir = "/run"; > +#endif > if (asprintf(&SOCKET_PROGRESS_PATH, "%s/%s", tmpdir, SOCKET_PROGRESS_DEFAULT) == -1) > return (char *)"/tmp/"SOCKET_PROGRESS_DEFAULT; > }
On Mon, 2023-10-23 at 11:28 +0200, Stefano Babic wrote: > Hi Felix, > > On 23.10.23 04:25, 'Felix Moessbauer' via swupdate wrote: > > When running on a systemd bootet system, the IPC sockets must be > > created > > below /run to be in sync with sockets provided by systemd units. > > > > Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com> > > --- > > ipc/network_ipc.c | 10 +++++++++- > > ipc/progress_ipc.c | 10 +++++++++- > > 2 files changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/ipc/network_ipc.c b/ipc/network_ipc.c > > index 20332b3..435a579 100644 > > --- a/ipc/network_ipc.c > > +++ b/ipc/network_ipc.c > > @@ -17,6 +17,10 @@ > > #include "network_ipc.h" > > #include "compat.h" > > > > +#ifdef CONFIG_SYSTEMD > > +#include <systemd/sd-daemon.h> > > +#endif > > + > > #ifdef CONFIG_SOCKET_CTRL_PATH > > static char* SOCKET_CTRL_PATH = (char*)CONFIG_SOCKET_CTRL_PATH; > > #else > > @@ -30,7 +34,11 @@ char *get_ctrl_socket(void) { > > const char *tmpdir = getenv("TMPDIR"); > > if (!tmpdir) > > tmpdir = "/tmp"; > > - > > +// on systemd systems /run is available and used for sockets > > created by systemd > > +#ifdef CONFIG_SYSTEMD > > + if(sd_booted()) > > + tmpdir = "/run"; > > +#endif > > No, this does not cover all use cases. > > Think about we have multiple instances of SWUpdate on the same > device. > This is a pattern when the device acts with different roles (gateway > vs > device is one of them), or when the update is split into domains, and > an > instance of SWUpdate can be set with lower rights if it is in charge > to > update just a part of the system. We need two or multiple sets of > sockets, and they should never conflict. This is currently done by > setting TMPDIR before launching SWUpdate, and in this case letting > SWUpdate to create the sockets itself. > > Or an instance is maybe getting the sockets from systemd, the second > one > can create the sockets. It is still configurable, but if sockets have > the same names, your change breaks this use case. > > Nevertheless, why do we need to hardcode the path into the code when > we > can set it before launching ? We can just set TMPDIR as /run in the > service unit, and this is ok in most cases (swupdate -i will create > the > sockets in /tmp). Hi Stefano, that's a valid point I overlooked so far. Unfortunately we cannot simply set TMPDIR freely, as this has quite some side effects: When doing an update with a compressed image, the image is first decompressed to TMPDIR and then flashed. However, this artifact might be quite big and should never be stored in a tmpfs (usually we run a bit low on memory anyways). Basically all tooling which is used as part of the update process (including mktemp) uses TMPDIR to find a location to store temporary artifacts. This why I started this whole overhaul in the first place: I had to set TMPDIR to /var/tmp (which is persistent storage in my case), as /tmp is a tmpfs where we simply do not have enough space. And by setting it, I went down the rabbit hole with improper cleanup, silently failing the swupdate-progress service (forgot to set TMPDIR there as well), no reboots on update, ... We need a mechanism to control where the sockets are created, which does not depend on the TMPDIR. How about a new variable? We could even make that backwards compatible by first checking for the new variable, then for the TMPDIR. Best regards, Felix > > It is also what systemd is doing, it uses env vars to pass the fds: > it > sets LISTEN_FDS to retrieve the list of fds. > > Best regards, > Stefano > > > if (asprintf(&SOCKET_CTRL_PATH, "%s/%s", tmpdir, > > SOCKET_CTRL_DEFAULT) == -1) > > return (char *)"/tmp/"SOCKET_CTRL_DEFAULT; > > } > > diff --git a/ipc/progress_ipc.c b/ipc/progress_ipc.c > > index 745dc44..a0def5c 100644 > > --- a/ipc/progress_ipc.c > > +++ b/ipc/progress_ipc.c > > @@ -14,6 +14,10 @@ > > #include <unistd.h> > > #include <stdbool.h> > > > > +#ifdef CONFIG_SYSTEMD > > +#include <systemd/sd-daemon.h> > > +#endif > > + > > #include <progress_ipc.h> > > > > #ifdef CONFIG_SOCKET_PROGRESS_PATH > > @@ -29,7 +33,11 @@ char *get_prog_socket(void) { > > const char *tmpdir = getenv("TMPDIR"); > > if (!tmpdir) > > tmpdir = "/tmp"; > > - > > +// on systemd systems /run is available and used for sockets > > created by systemd > > +#ifdef CONFIG_SYSTEMD > > + if(sd_booted()) > > + tmpdir = "/run"; > > +#endif > > if (asprintf(&SOCKET_PROGRESS_PATH, "%s/%s", > > tmpdir, SOCKET_PROGRESS_DEFAULT) == -1) > > return (char > > *)"/tmp/"SOCKET_PROGRESS_DEFAULT; > > } >
Hi Felix, On 23.10.23 12:34, 'MOESSBAUER, Felix' via swupdate wrote: > On Mon, 2023-10-23 at 11:28 +0200, Stefano Babic wrote: >> Hi Felix, >> >> On 23.10.23 04:25, 'Felix Moessbauer' via swupdate wrote: >>> When running on a systemd bootet system, the IPC sockets must be >>> created >>> below /run to be in sync with sockets provided by systemd units. >>> >>> Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com> >>> --- >>> ipc/network_ipc.c | 10 +++++++++- >>> ipc/progress_ipc.c | 10 +++++++++- >>> 2 files changed, 18 insertions(+), 2 deletions(-) >>> >>> diff --git a/ipc/network_ipc.c b/ipc/network_ipc.c >>> index 20332b3..435a579 100644 >>> --- a/ipc/network_ipc.c >>> +++ b/ipc/network_ipc.c >>> @@ -17,6 +17,10 @@ >>> #include "network_ipc.h" >>> #include "compat.h" >>> >>> +#ifdef CONFIG_SYSTEMD >>> +#include <systemd/sd-daemon.h> >>> +#endif >>> + >>> #ifdef CONFIG_SOCKET_CTRL_PATH >>> static char* SOCKET_CTRL_PATH = (char*)CONFIG_SOCKET_CTRL_PATH; >>> #else >>> @@ -30,7 +34,11 @@ char *get_ctrl_socket(void) { >>> const char *tmpdir = getenv("TMPDIR"); >>> if (!tmpdir) >>> tmpdir = "/tmp"; >>> - >>> +// on systemd systems /run is available and used for sockets >>> created by systemd >>> +#ifdef CONFIG_SYSTEMD >>> + if(sd_booted()) >>> + tmpdir = "/run"; >>> +#endif >> >> No, this does not cover all use cases. >> >> Think about we have multiple instances of SWUpdate on the same >> device. >> This is a pattern when the device acts with different roles (gateway >> vs >> device is one of them), or when the update is split into domains, and >> an >> instance of SWUpdate can be set with lower rights if it is in charge >> to >> update just a part of the system. We need two or multiple sets of >> sockets, and they should never conflict. This is currently done by >> setting TMPDIR before launching SWUpdate, and in this case letting >> SWUpdate to create the sockets itself. >> >> Or an instance is maybe getting the sockets from systemd, the second >> one >> can create the sockets. It is still configurable, but if sockets have >> the same names, your change breaks this use case. >> >> Nevertheless, why do we need to hardcode the path into the code when >> we >> can set it before launching ? We can just set TMPDIR as /run in the >> service unit, and this is ok in most cases (swupdate -i will create >> the >> sockets in /tmp). > > Hi Stefano, that's a valid point I overlooked so far. > > Unfortunately we cannot simply set TMPDIR freely, as this has quite > some side effects: When doing an update with a compressed image, the > image is first decompressed to TMPDIR and then flashed. However, this > artifact might be quite big and should never be stored in a tmpfs > (usually we run a bit low on memory anyways). True, but SWUpdate supports zero-copy, and this is usually set for large artifacts. Nothing is stored temporarily. > Basically all tooling > which is used as part of the update process (including mktemp) uses > TMPDIR to find a location to store temporary artifacts. > > This why I started this whole overhaul in the first place: I had to set > TMPDIR to /var/tmp (which is persistent storage in my case), as /tmp is > a tmpfs where we simply do not have enough space. Ok > And by setting it, I > went down the rabbit hole with improper cleanup, silently failing the > swupdate-progress service (forgot to set TMPDIR there as well), no > reboots on update, ... Of course, everything should be consistent then. Internally, consistency is guaranteed because processes that wants to communicate call first get_ctrl_socket() to get the right path. But this cannot be guaranteed for the external processes. > > We need a mechanism to control where the sockets are created, which > does not depend on the TMPDIR. So you want to have sockets and temporary files into different directories. > > How about a new variable? Possible, I am just careful to avoid to use a lot of envs. But ther eis currently only TMPDIR, so we can introduce a new one with fallback to TMPDIR if not set. > We could even make that backwards compatible > by first checking for the new variable, then for the TMPDIR. Ok -fine. So I will apply 1-3 (this is cleanup and better code understanding), and drop 4-5 for the moment. Best regards, Stefano > > Best regards, > Felix > >> >> It is also what systemd is doing, it uses env vars to pass the fds: >> it >> sets LISTEN_FDS to retrieve the list of fds. >> >> Best regards, >> Stefano >> >>> if (asprintf(&SOCKET_CTRL_PATH, "%s/%s", tmpdir, >>> SOCKET_CTRL_DEFAULT) == -1) >>> return (char *)"/tmp/"SOCKET_CTRL_DEFAULT; >>> } >>> diff --git a/ipc/progress_ipc.c b/ipc/progress_ipc.c >>> index 745dc44..a0def5c 100644 >>> --- a/ipc/progress_ipc.c >>> +++ b/ipc/progress_ipc.c >>> @@ -14,6 +14,10 @@ >>> #include <unistd.h> >>> #include <stdbool.h> >>> >>> +#ifdef CONFIG_SYSTEMD >>> +#include <systemd/sd-daemon.h> >>> +#endif >>> + >>> #include <progress_ipc.h> >>> >>> #ifdef CONFIG_SOCKET_PROGRESS_PATH >>> @@ -29,7 +33,11 @@ char *get_prog_socket(void) { >>> const char *tmpdir = getenv("TMPDIR"); >>> if (!tmpdir) >>> tmpdir = "/tmp"; >>> - >>> +// on systemd systems /run is available and used for sockets >>> created by systemd >>> +#ifdef CONFIG_SYSTEMD >>> + if(sd_booted()) >>> + tmpdir = "/run"; >>> +#endif >>> if (asprintf(&SOCKET_PROGRESS_PATH, "%s/%s", >>> tmpdir, SOCKET_PROGRESS_DEFAULT) == -1) >>> return (char >>> *)"/tmp/"SOCKET_PROGRESS_DEFAULT; >>> } >> >
diff --git a/ipc/network_ipc.c b/ipc/network_ipc.c index 20332b3..435a579 100644 --- a/ipc/network_ipc.c +++ b/ipc/network_ipc.c @@ -17,6 +17,10 @@ #include "network_ipc.h" #include "compat.h" +#ifdef CONFIG_SYSTEMD +#include <systemd/sd-daemon.h> +#endif + #ifdef CONFIG_SOCKET_CTRL_PATH static char* SOCKET_CTRL_PATH = (char*)CONFIG_SOCKET_CTRL_PATH; #else @@ -30,7 +34,11 @@ char *get_ctrl_socket(void) { const char *tmpdir = getenv("TMPDIR"); if (!tmpdir) tmpdir = "/tmp"; - +// on systemd systems /run is available and used for sockets created by systemd +#ifdef CONFIG_SYSTEMD + if(sd_booted()) + tmpdir = "/run"; +#endif if (asprintf(&SOCKET_CTRL_PATH, "%s/%s", tmpdir, SOCKET_CTRL_DEFAULT) == -1) return (char *)"/tmp/"SOCKET_CTRL_DEFAULT; } diff --git a/ipc/progress_ipc.c b/ipc/progress_ipc.c index 745dc44..a0def5c 100644 --- a/ipc/progress_ipc.c +++ b/ipc/progress_ipc.c @@ -14,6 +14,10 @@ #include <unistd.h> #include <stdbool.h> +#ifdef CONFIG_SYSTEMD +#include <systemd/sd-daemon.h> +#endif + #include <progress_ipc.h> #ifdef CONFIG_SOCKET_PROGRESS_PATH @@ -29,7 +33,11 @@ char *get_prog_socket(void) { const char *tmpdir = getenv("TMPDIR"); if (!tmpdir) tmpdir = "/tmp"; - +// on systemd systems /run is available and used for sockets created by systemd +#ifdef CONFIG_SYSTEMD + if(sd_booted()) + tmpdir = "/run"; +#endif if (asprintf(&SOCKET_PROGRESS_PATH, "%s/%s", tmpdir, SOCKET_PROGRESS_DEFAULT) == -1) return (char *)"/tmp/"SOCKET_PROGRESS_DEFAULT; }
When running on a systemd bootet system, the IPC sockets must be created below /run to be in sync with sockets provided by systemd units. Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com> --- ipc/network_ipc.c | 10 +++++++++- ipc/progress_ipc.c | 10 +++++++++- 2 files changed, 18 insertions(+), 2 deletions(-)