| Message ID | 20180423123109.18590-8-christian.storm@siemens.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/8] core: warn about non-Linux parent SIGTERM tracking | expand |
Hi Christian, On 23/04/2018 14:31, Christian Storm wrote: > As FreeBSD doesn't have abstract sockets like Linux has, > use filesystem-backed sockets for FreeBSD. > > Signed-off-by: Christian Storm <christian.storm@siemens.com> > --- > Kconfig | 8 ++++++++ > core/notifier.c | 36 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 44 insertions(+) > > diff --git a/Kconfig b/Kconfig > index c7d1d5c..8381827 100644 > --- a/Kconfig > +++ b/Kconfig > @@ -155,6 +155,14 @@ config SOCKET_REMOTE_HANDLER_DIRECTORY > Directory where sockets to remote handler processes > are expected to be found. > > +config SOCKET_NOTIFIER_DIRECTORY > + string "SWUpdate notifier socket directory" > + depends on !HAVE_LINUX > + default "/tmp/" > + help > + Path to SWUpdate's Notifier sockets on FreeBSD as > + Linux-like abstract sockets are not available. > + > endmenu > > config MTD > diff --git a/core/notifier.c b/core/notifier.c > index 1a79a95..e2d1407 100644 > --- a/core/notifier.c > +++ b/core/notifier.c > @@ -203,6 +203,26 @@ static void process_notifier (RECOVERY_STATUS status, int event, int level, cons > > } > > +#if !defined(__linux__) This is for not-klinux, but it does not mean that it is for FreeBSD. We do not know if SWUpdate will run even on other OSes. It would be better to have a positive assertion (#if defined (__FreeBSD__)) else a negative as here. This affects other patches, too, even if I have already reviewed them. > +static char* socket_path = NULL; > +static void unlink_socket(void) > +{ > + if (socket_path) { > + unlink(socket_path); > + free(socket_path); > + } > +} > + > +static void setup_socket_cleanup(struct sockaddr_un *addr) > +{ > + socket_path = strndupa(addr->sun_path, sizeof(addr->sun_path)); > + if (atexit(unlink_socket) != 0) { > + TRACE("Cannot setup socket cleanup on exit, %s won't be unlinked.", socket_path); > + } > + unlink(socket_path); > +} > +#endif > + > /* > * Utility function to setup the internal IPC > */ > @@ -210,11 +230,20 @@ static void addr_init(struct sockaddr_un *addr, const char *path) > { > memset(addr, 0, sizeof(struct sockaddr_un)); > addr->sun_family = AF_UNIX; > +#if defined(__linux__) > /* > * Use Linux-specific abstract sockets for this internal interface > */ > strcpy(&addr->sun_path[1], path); > addr->sun_path[0] = '\0'; > +#else > + /* > + * Use filesystem-backed sockets although this interface is internal > + */ > + strncpy(addr->sun_path, CONFIG_SOCKET_NOTIFIER_DIRECTORY, sizeof(addr->sun_path)); > + strncat(addr->sun_path, path, > + sizeof(addr->sun_path)-strlen(CONFIG_SOCKET_NOTIFIER_DIRECTORY)); > +#endif > } > > /* > @@ -236,6 +265,10 @@ static void *notifier_thread (void __attribute__ ((__unused__)) *data) > exit(2); > } > > +#if !defined(__linux__) > + setup_socket_cleanup(¬ify_server); > +#endif > + > if (bind(serverfd, (const struct sockaddr *) ¬ify_server, > sizeof(struct sockaddr_un)) < 0) { > fprintf(stderr, "Error binding notifier socket: %s, exiting.\n", strerror(errno)); > @@ -288,6 +321,9 @@ void notify_init(void) > char buf[60]; > snprintf(buf, sizeof(buf), "Notify%d", pid); > addr_init(¬ify_client, buf); > +#if !defined(__linux__) > + setup_socket_cleanup(¬ify_client); > +#endif > notifyfd = socket(AF_UNIX, SOCK_DGRAM, 0); > if (notifyfd < 0) { > printf("Error creating notifier socket for pid %d", pid); > Best regards, Stefano
Hi Stefano, > > [...] > > > > +#if !defined(__linux__) > > This is for not-klinux, but it does not mean that it is for FreeBSD. We > do not know if SWUpdate will run even on other OSes. It would be better > to have a positive assertion (#if defined (__FreeBSD__)) else a negative > as here. This affects other patches, too, even if I have already > reviewed them. Yes, I have thought about this as well. I found that the APIs on FreeBSD are not that elaborated compared to those available on Linux and hence I thought it would be more likely to run on other OSes (or BSDs for that matter) than the Linux variant. That's why I marked the "primitive" version to be used for non-Linux OSes. That said, I'm also fine with explicitly white-listing FreeBSD only. However, I suspect that most of this also works on OpenBSD/NetBSD/other BSDs as well -- although I haven't tested it... So, what do you think is the best option here? Kind regards, Christian
On 24/04/2018 13:59, Christian Storm wrote: > Hi Stefano, > >>> [...] >>> >>> +#if !defined(__linux__) >> >> This is for not-klinux, but it does not mean that it is for FreeBSD. We >> do not know if SWUpdate will run even on other OSes. It would be better >> to have a positive assertion (#if defined (__FreeBSD__)) else a negative >> as here. This affects other patches, too, even if I have already >> reviewed them. > > Yes, I have thought about this as well. I found that the APIs on FreeBSD > are not that elaborated compared to those available on Linux and hence I > thought it would be more likely to run on other OSes (or BSDs for that > matter) than the Linux variant. That's why I marked the "primitive" > version to be used for non-Linux OSes. > That said, I'm also fine with explicitly white-listing FreeBSD only. > However, I suspect that most of this also works on OpenBSD/NetBSD/other > BSDs as well -- although I haven't tested it... > > So, what do you think is the best option here? > Go on with the white list option - if someone on the ML will take care of testing on other OSes, we can add them later. Best regards, Stefano
diff --git a/Kconfig b/Kconfig index c7d1d5c..8381827 100644 --- a/Kconfig +++ b/Kconfig @@ -155,6 +155,14 @@ config SOCKET_REMOTE_HANDLER_DIRECTORY Directory where sockets to remote handler processes are expected to be found. +config SOCKET_NOTIFIER_DIRECTORY + string "SWUpdate notifier socket directory" + depends on !HAVE_LINUX + default "/tmp/" + help + Path to SWUpdate's Notifier sockets on FreeBSD as + Linux-like abstract sockets are not available. + endmenu config MTD diff --git a/core/notifier.c b/core/notifier.c index 1a79a95..e2d1407 100644 --- a/core/notifier.c +++ b/core/notifier.c @@ -203,6 +203,26 @@ static void process_notifier (RECOVERY_STATUS status, int event, int level, cons } +#if !defined(__linux__) +static char* socket_path = NULL; +static void unlink_socket(void) +{ + if (socket_path) { + unlink(socket_path); + free(socket_path); + } +} + +static void setup_socket_cleanup(struct sockaddr_un *addr) +{ + socket_path = strndupa(addr->sun_path, sizeof(addr->sun_path)); + if (atexit(unlink_socket) != 0) { + TRACE("Cannot setup socket cleanup on exit, %s won't be unlinked.", socket_path); + } + unlink(socket_path); +} +#endif + /* * Utility function to setup the internal IPC */ @@ -210,11 +230,20 @@ static void addr_init(struct sockaddr_un *addr, const char *path) { memset(addr, 0, sizeof(struct sockaddr_un)); addr->sun_family = AF_UNIX; +#if defined(__linux__) /* * Use Linux-specific abstract sockets for this internal interface */ strcpy(&addr->sun_path[1], path); addr->sun_path[0] = '\0'; +#else + /* + * Use filesystem-backed sockets although this interface is internal + */ + strncpy(addr->sun_path, CONFIG_SOCKET_NOTIFIER_DIRECTORY, sizeof(addr->sun_path)); + strncat(addr->sun_path, path, + sizeof(addr->sun_path)-strlen(CONFIG_SOCKET_NOTIFIER_DIRECTORY)); +#endif } /* @@ -236,6 +265,10 @@ static void *notifier_thread (void __attribute__ ((__unused__)) *data) exit(2); } +#if !defined(__linux__) + setup_socket_cleanup(¬ify_server); +#endif + if (bind(serverfd, (const struct sockaddr *) ¬ify_server, sizeof(struct sockaddr_un)) < 0) { fprintf(stderr, "Error binding notifier socket: %s, exiting.\n", strerror(errno)); @@ -288,6 +321,9 @@ void notify_init(void) char buf[60]; snprintf(buf, sizeof(buf), "Notify%d", pid); addr_init(¬ify_client, buf); +#if !defined(__linux__) + setup_socket_cleanup(¬ify_client); +#endif notifyfd = socket(AF_UNIX, SOCK_DGRAM, 0); if (notifyfd < 0) { printf("Error creating notifier socket for pid %d", pid);
As FreeBSD doesn't have abstract sockets like Linux has, use filesystem-backed sockets for FreeBSD. Signed-off-by: Christian Storm <christian.storm@siemens.com> --- Kconfig | 8 ++++++++ core/notifier.c | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+)