[PATCH 1/2] main: Add support for $NOTIFY_SOCKET

[PATCH 1/2] main: Add support for $NOTIFY_SOCKET

[Luiz Augusto von Dentz]

[-- Attachment #1: Type: text/plain, Size: 3611 bytes --]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This adds support for sending states to the service manager using the
socket set in $NOTIFY_SOCKET:

https://www.freedesktop.org/software/systemd/man/sd_notify.html
---
 ell/main.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ell/main.h |  2 ++
 2 files changed, 82 insertions(+)

diff --git a/ell/main.c b/ell/main.c
index b1d12b6..7eedf1a 100644
--- a/ell/main.c
+++ b/ell/main.c
@@ -28,9 +28,12 @@
 #include <errno.h>
 #include <unistd.h>
 #include <stdlib.h>
+#include <stddef.h>
 #include <limits.h>
 #include <signal.h>
 #include <sys/epoll.h>
+#include <sys/socket.h>
+#include <sys/un.h>
 
 #include "signal.h"
 #include "queue.h"
@@ -59,6 +62,9 @@ static bool epoll_running;
 static bool epoll_terminate;
 static int idle_id;
 
+static char *notify_sock;
+static int notify_fd;
+
 static struct l_queue *idle_list;
 
 struct watch_data {
@@ -324,6 +330,27 @@ static void idle_dispatch(void *data, void *user_data)
 	idle->flags &= ~IDLE_FLAG_DISPATCHING;
 }
 
+static inline void __attribute__ ((always_inline)) notify_socket(void)
+{
+	const char *sock;
+
+	/* check if NOTIFY_SOCKET has been set */
+	sock = getenv("NOTIFY_SOCKET");
+	if (!sock)
+		return;
+
+	/* check for abstract socket or absolute path */
+	if (sock[0] != '@' && sock[0] != '/')
+		return;
+
+	notify_sock = l_strdup(sock);
+
+	notify_fd = socket(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0);
+	if (notify_fd < 0)
+		notify_fd = 0;
+}
+
+
 /**
  * l_main_init:
  *
@@ -342,6 +369,8 @@ LIB_EXPORT bool l_main_init(void)
 	if (!create_epoll())
 		return false;
 
+	notify_socket();
+
 	epoll_terminate = false;
 
 	return true;
@@ -438,6 +467,13 @@ LIB_EXPORT int l_main_run(void)
 
 	epoll_running = false;
 
+	if (notify_fd) {
+		close(notify_fd);
+		notify_fd = 0;
+		l_free(notify_sock);
+		notify_sock = NULL;
+	}
+
 	return EXIT_SUCCESS;
 }
 
@@ -570,3 +606,47 @@ LIB_EXPORT int l_main_get_epoll_fd(void)
 {
 	return epoll_fd;
 }
+
+/**
+ * l_main_sd_notify:
+ *
+ * Notify service manager about start-up completion and other service status
+ * changes.
+ *
+ * Returns: On failure, these calls return a negative errno-style error code.
+ * If $NOTIFY_SOCKET was not set and hence no status message could be sent,
+ * -ENOCONN is returned.
+ **/
+LIB_EXPORT int l_main_sd_notify(const char *state)
+{
+	struct sockaddr_un addr;
+	struct msghdr msghdr;
+	struct iovec iovec;
+
+	if (!notify_fd)
+		return -ENOTCONN;
+
+	memset(&addr, 0, sizeof(addr));
+	addr.sun_family = AF_UNIX;
+	strncpy(addr.sun_path, notify_sock, sizeof(addr.sun_path) - 1);
+
+	if (addr.sun_path[0] == '@')
+		addr.sun_path[0] = '\0';
+
+	memset(&iovec, 0, sizeof(iovec));
+	iovec.iov_base = (char *) state;
+	iovec.iov_len = strlen(state);
+
+	memset(&msghdr, 0, sizeof(msghdr));
+	msghdr.msg_name = &addr;
+	msghdr.msg_namelen = offsetof(struct sockaddr_un, sun_path) +
+							strlen(notify_sock);
+
+	if (msghdr.msg_namelen > sizeof(struct sockaddr_un))
+		msghdr.msg_namelen = sizeof(struct sockaddr_un);
+
+	msghdr.msg_iov = &iovec;
+	msghdr.msg_iovlen = 1;
+
+	return sendmsg(notify_fd, &msghdr, MSG_NOSIGNAL);
+}
diff --git a/ell/main.h b/ell/main.h
index 99b34ad..ffa40af 100644
--- a/ell/main.h
+++ b/ell/main.h
@@ -44,6 +44,8 @@ int l_main_run_with_signal(l_main_signal_cb_t callback, void *user_data);
 
 int l_main_get_epoll_fd();
 
+int l_main_sd_notify(const char *state);
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.17.2

[PATCH 2/2] main: Add support for $WATCHDOG_USEC

[Luiz Augusto von Dentz]

[-- Attachment #1: Type: text/plain, Size: 2301 bytes --]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This adds handling to watchdog set by service manager by sending
"WATCHDOG=1" to it in intervals shorter than what is set in
$WATCHDOG_USEC.
---
 ell/main.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/ell/main.c b/ell/main.c
index 7eedf1a..a59de16 100644
--- a/ell/main.c
+++ b/ell/main.c
@@ -41,6 +41,7 @@
 #include "util.h"
 #include "main.h"
 #include "private.h"
+#include "timeout.h"
 
 /**
  * SECTION:main
@@ -57,6 +58,8 @@
 #define WATCH_FLAG_DISPATCHING	1
 #define WATCH_FLAG_DESTROYED	2
 
+#define WATCHDOG_TRIGGER_FREQ	2
+
 static int epoll_fd;
 static bool epoll_running;
 static bool epoll_terminate;
@@ -65,6 +68,8 @@ static int idle_id;
 static char *notify_sock;
 static int notify_fd;
 
+static struct l_timeout *watchdog;
+
 static struct l_queue *idle_list;
 
 struct watch_data {
@@ -330,9 +335,20 @@ static void idle_dispatch(void *data, void *user_data)
 	idle->flags &= ~IDLE_FLAG_DISPATCHING;
 }
 
+static void watchdog_callback(struct l_timeout *timeout, void *user_data)
+{
+	int msec = L_PTR_TO_INT(user_data);
+
+	l_main_sd_notify("WATCHDOG=1");
+
+	l_timeout_modify_ms(timeout, msec);
+}
+
 static inline void __attribute__ ((always_inline)) notify_socket(void)
 {
 	const char *sock;
+	const char *watchdog_usec;
+	int msec;
 
 	/* check if NOTIFY_SOCKET has been set */
 	sock = getenv("NOTIFY_SOCKET");
@@ -346,8 +362,23 @@ static inline void __attribute__ ((always_inline)) notify_socket(void)
 	notify_sock = l_strdup(sock);
 
 	notify_fd = socket(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0);
-	if (notify_fd < 0)
+	if (notify_fd < 0) {
 		notify_fd = 0;
+		return;
+	}
+
+	watchdog_usec = getenv("WATCHDOG_USEC");
+	if (!watchdog_usec)
+		return;
+
+	msec = atoi(watchdog_usec) / 1000;
+	if (msec < 0)
+		return;
+
+	msec /= WATCHDOG_TRIGGER_FREQ;
+
+	watchdog = l_timeout_create_ms(msec, watchdog_callback,
+					L_INT_TO_PTR(msec), NULL);
 }
 
 
@@ -472,6 +503,8 @@ LIB_EXPORT int l_main_run(void)
 		notify_fd = 0;
 		l_free(notify_sock);
 		notify_sock = NULL;
+		l_timeout_remove(watchdog);
+		watchdog = NULL;
 	}
 
 	return EXIT_SUCCESS;
-- 
2.17.2

Re: [PATCH 1/2] main: Add support for $NOTIFY_SOCKET

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 4654 bytes --]

Hi Luiz,

On 11/28/2018 06:16 AM, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> This adds support for sending states to the service manager using the
> socket set in $NOTIFY_SOCKET:
> 
> https://www.freedesktop.org/software/systemd/man/sd_notify.html
> ---
>   ell/main.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   ell/main.h |  2 ++
>   2 files changed, 82 insertions(+)
> 
> diff --git a/ell/main.c b/ell/main.c
> index b1d12b6..7eedf1a 100644
> --- a/ell/main.c
> +++ b/ell/main.c
> @@ -28,9 +28,12 @@
>   #include <errno.h>
>   #include <unistd.h>
>   #include <stdlib.h>
> +#include <stddef.h>
>   #include <limits.h>
>   #include <signal.h>
>   #include <sys/epoll.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
>   
>   #include "signal.h"
>   #include "queue.h"
> @@ -59,6 +62,9 @@ static bool epoll_running;
>   static bool epoll_terminate;
>   static int idle_id;
>   
> +static char *notify_sock;
> +static int notify_fd;
> +
>   static struct l_queue *idle_list;
>   
>   struct watch_data {
> @@ -324,6 +330,27 @@ static void idle_dispatch(void *data, void *user_data)
>   	idle->flags &= ~IDLE_FLAG_DISPATCHING;
>   }
>   
> +static inline void __attribute__ ((always_inline)) notify_socket(void)

This is called once no?  Why does it need to be always_inline?

> +{
> +	const char *sock;
> +
> +	/* check if NOTIFY_SOCKET has been set */
> +	sock = getenv("NOTIFY_SOCKET");
> +	if (!sock)
> +		return;
> +
> +	/* check for abstract socket or absolute path */
> +	if (sock[0] != '@' && sock[0] != '/')
> +		return;
> +
> +	notify_sock = l_strdup(sock);

Why don't you just bind the socket here and save book-keeping this variable.

> +
> +	notify_fd = socket(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0);
> +	if (notify_fd < 0)
> +		notify_fd = 0;
> +}
> +
> +

Double empty line...

>   /**
>    * l_main_init:
>    *
> @@ -342,6 +369,8 @@ LIB_EXPORT bool l_main_init(void)
>   	if (!create_epoll())
>   		return false;
>   
> +	notify_socket();
> +

Maybe create_sd_notify_socket to make things clearer?  I also wonder if 
this needs to be behind #if SYSTEMD_SUPPORT or something...  Maybe it is 
okay as is.

>   	epoll_terminate = false;
>   
>   	return true;
> @@ -438,6 +467,13 @@ LIB_EXPORT int l_main_run(void)
>   
>   	epoll_running = false;
>   
> +	if (notify_fd) {
> +		close(notify_fd);
> +		notify_fd = 0;
> +		l_free(notify_sock);
> +		notify_sock = NULL;
> +	}
> +
>   	return EXIT_SUCCESS;
>   }
>   
> @@ -570,3 +606,47 @@ LIB_EXPORT int l_main_get_epoll_fd(void)
>   {
>   	return epoll_fd;
>   }
> +
> +/**
> + * l_main_sd_notify:
> + *
> + * Notify service manager about start-up completion and other service status
> + * changes.
> + *
> + * Returns: On failure, these calls return a negative errno-style error code.
> + * If $NOTIFY_SOCKET was not set and hence no status message could be sent,
> + * -ENOCONN is returned.

Might be worthwhile to reference the freedesktop page here directly 
instead of in the commit description

> + **/
> +LIB_EXPORT int l_main_sd_notify(const char *state)
> +{
> +	struct sockaddr_un addr;
> +	struct msghdr msghdr;
> +	struct iovec iovec;
> +
> +	if (!notify_fd)
> +		return -ENOTCONN;
> +
> +	memset(&addr, 0, sizeof(addr));
> +	addr.sun_family = AF_UNIX;
> +	strncpy(addr.sun_path, notify_sock, sizeof(addr.sun_path) - 1);
> +
> +	if (addr.sun_path[0] == '@')
> +		addr.sun_path[0] = '\0';

As mentioned before, why not bind() to the address in the first place?

> +
> +	memset(&iovec, 0, sizeof(iovec));
> +	iovec.iov_base = (char *) state;
> +	iovec.iov_len = strlen(state);
> +
> +	memset(&msghdr, 0, sizeof(msghdr));
> +	msghdr.msg_name = &addr;
> +	msghdr.msg_namelen = offsetof(struct sockaddr_un, sun_path) +
> +							strlen(notify_sock);
> +
> +	if (msghdr.msg_namelen > sizeof(struct sockaddr_un))
> +		msghdr.msg_namelen = sizeof(struct sockaddr_un);
> +
> +	msghdr.msg_iov = &iovec;
> +	msghdr.msg_iovlen = 1;

Then this can just become send()

> +
> +	return sendmsg(notify_fd, &msghdr, MSG_NOSIGNAL);

This doesn't actually return a negative errno...

> +}
> diff --git a/ell/main.h b/ell/main.h
> index 99b34ad..ffa40af 100644
> --- a/ell/main.h
> +++ b/ell/main.h
> @@ -44,6 +44,8 @@ int l_main_run_with_signal(l_main_signal_cb_t callback, void *user_data);
>   
>   int l_main_get_epoll_fd();
>   
> +int l_main_sd_notify(const char *state);
> +
>   #ifdef __cplusplus
>   }
>   #endif
> 

Regards,
-Denis

Re: [PATCH 1/2] main: Add support for $NOTIFY_SOCKET

[Luiz Augusto von Dentz]

[-- Attachment #1: Type: text/plain, Size: 5946 bytes --]

Hi Denis,

On Thu, Nov 29, 2018 at 8:01 PM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Luiz,
>
> On 11/28/2018 06:16 AM, Luiz Augusto von Dentz wrote:
> > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >
> > This adds support for sending states to the service manager using the
> > socket set in $NOTIFY_SOCKET:
> >
> > https://www.freedesktop.org/software/systemd/man/sd_notify.html
> > ---
> >   ell/main.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   ell/main.h |  2 ++
> >   2 files changed, 82 insertions(+)
> >
> > diff --git a/ell/main.c b/ell/main.c
> > index b1d12b6..7eedf1a 100644
> > --- a/ell/main.c
> > +++ b/ell/main.c
> > @@ -28,9 +28,12 @@
> >   #include <errno.h>
> >   #include <unistd.h>
> >   #include <stdlib.h>
> > +#include <stddef.h>
> >   #include <limits.h>
> >   #include <signal.h>
> >   #include <sys/epoll.h>
> > +#include <sys/socket.h>
> > +#include <sys/un.h>
> >
> >   #include "signal.h"
> >   #include "queue.h"
> > @@ -59,6 +62,9 @@ static bool epoll_running;
> >   static bool epoll_terminate;
> >   static int idle_id;
> >
> > +static char *notify_sock;
> > +static int notify_fd;
> > +
> >   static struct l_queue *idle_list;
> >
> >   struct watch_data {
> > @@ -324,6 +330,27 @@ static void idle_dispatch(void *data, void *user_data)
> >       idle->flags &= ~IDLE_FLAG_DISPATCHING;
> >   }
> >
> > +static inline void __attribute__ ((always_inline)) notify_socket(void)
>
> This is called once no?  Why does it need to be always_inline?

I thought I would call it more than once, but with notify_fd that is
no longer needed.

> > +{
> > +     const char *sock;
> > +
> > +     /* check if NOTIFY_SOCKET has been set */
> > +     sock = getenv("NOTIFY_SOCKET");
> > +     if (!sock)
> > +             return;
> > +
> > +     /* check for abstract socket or absolute path */
> > +     if (sock[0] != '@' && sock[0] != '/')
> > +             return;
> > +
> > +     notify_sock = l_strdup(sock);
>
> Why don't you just bind the socket here and save book-keeping this variable.

The original code wasn't binding, but now that you mentioned it
probably make sense to just bind it.

> > +
> > +     notify_fd = socket(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0);
> > +     if (notify_fd < 0)
> > +             notify_fd = 0;
> > +}
> > +
> > +
>
> Double empty line...

Will fix.

> >   /**
> >    * l_main_init:
> >    *
> > @@ -342,6 +369,8 @@ LIB_EXPORT bool l_main_init(void)
> >       if (!create_epoll())
> >               return false;
> >
> > +     notify_socket();
> > +
>
> Maybe create_sd_notify_socket to make things clearer?  I also wonder if
> this needs to be behind #if SYSTEMD_SUPPORT or something...  Maybe it is
> okay as is.

I was wondering if this is supposed to be systemd specific or there
are other service managers that use it, anyway I don't have a strong
opinion on this since it is not a big deal to check if an environment
variable is set and open a socket.

> >       epoll_terminate = false;
> >
> >       return true;
> > @@ -438,6 +467,13 @@ LIB_EXPORT int l_main_run(void)
> >
> >       epoll_running = false;
> >
> > +     if (notify_fd) {
> > +             close(notify_fd);
> > +             notify_fd = 0;
> > +             l_free(notify_sock);
> > +             notify_sock = NULL;
> > +     }
> > +
> >       return EXIT_SUCCESS;
> >   }
> >
> > @@ -570,3 +606,47 @@ LIB_EXPORT int l_main_get_epoll_fd(void)
> >   {
> >       return epoll_fd;
> >   }
> > +
> > +/**
> > + * l_main_sd_notify:
> > + *
> > + * Notify service manager about start-up completion and other service status
> > + * changes.
> > + *
> > + * Returns: On failure, these calls return a negative errno-style error code.
> > + * If $NOTIFY_SOCKET was not set and hence no status message could be sent,
> > + * -ENOCONN is returned.
>
> Might be worthwhile to reference the freedesktop page here directly
> instead of in the commit description

Will do.

> > + **/
> > +LIB_EXPORT int l_main_sd_notify(const char *state)
> > +{
> > +     struct sockaddr_un addr;
> > +     struct msghdr msghdr;
> > +     struct iovec iovec;
> > +
> > +     if (!notify_fd)
> > +             return -ENOTCONN;
> > +
> > +     memset(&addr, 0, sizeof(addr));
> > +     addr.sun_family = AF_UNIX;
> > +     strncpy(addr.sun_path, notify_sock, sizeof(addr.sun_path) - 1);
> > +
> > +     if (addr.sun_path[0] == '@')
> > +             addr.sun_path[0] = '\0';
>
> As mentioned before, why not bind() to the address in the first place?

Will do.

> > +
> > +     memset(&iovec, 0, sizeof(iovec));
> > +     iovec.iov_base = (char *) state;
> > +     iovec.iov_len = strlen(state);
> > +
> > +     memset(&msghdr, 0, sizeof(msghdr));
> > +     msghdr.msg_name = &addr;
> > +     msghdr.msg_namelen = offsetof(struct sockaddr_un, sun_path) +
> > +                                                     strlen(notify_sock);
> > +
> > +     if (msghdr.msg_namelen > sizeof(struct sockaddr_un))
> > +             msghdr.msg_namelen = sizeof(struct sockaddr_un);
> > +
> > +     msghdr.msg_iov = &iovec;
> > +     msghdr.msg_iovlen = 1;
>
> Then this can just become send()

Sure, but I guess we are maintaining the practice of using MSG_NOSIGNAL.

> > +
> > +     return sendmsg(notify_fd, &msghdr, MSG_NOSIGNAL);
>
> This doesn't actually return a negative errno...
>
> > +}
> > diff --git a/ell/main.h b/ell/main.h
> > index 99b34ad..ffa40af 100644
> > --- a/ell/main.h
> > +++ b/ell/main.h
> > @@ -44,6 +44,8 @@ int l_main_run_with_signal(l_main_signal_cb_t callback, void *user_data);
> >
> >   int l_main_get_epoll_fd();
> >
> > +int l_main_sd_notify(const char *state);
> > +
> >   #ifdef __cplusplus
> >   }
> >   #endif
> >
>
> Regards,
> -Denis



-- 
Luiz Augusto von Dentz