[tarantool-patches] Re: [PATCH] Enable support for NOTIFY_SOCKET on macOS

["Maxim Melentiev" <dmarc-noreply@freelists.org> (Redacted sender "m.melentiev" for DMARC)]

 From 18e567ff6fdb06c1f72626818548011a30a62ecb Mon Sep 17 00:00:00 2001
From: Max Melentiev <m.melentiev@corp.mail.ru>
Date: Thu, 25 Jul 2019 10:05:24 +0300
Subject: [PATCH] Enable support for NOTIFY_SOCKET in envs without systemd

To make it possible to develop and test related features on
systems without systemd.

WITH_SYSTEMD cmake flag is used to generate systemd related files:
unit, generator script, etc. To keep this behavior and make it possible
to use NOTIFY_SOCKET without other systemd-related stuff,
I added WITH_NOTIFY_SOCKET cmake flag.

It also required some changes to support other OS:

SOCK_CLOEXEC (not available on macOS) flag for socket()
is replaced with `fcntl(fd, F_SETFD, FD_CLOEXEC)` which has the same effect.

MSG_NOSIGNAL flag for sendmsg is also not available on macOS.
However it has SO_NOSIGPIPE flag for setsockopt which disables SIGPIPE.
So it requires different solution for different OS. Inspired by
https://nwat.xyz/blog/2014/01/16/porting-msg_more-and-msg_nosigpipe-to-osx/

Have to reduce send-buffer size to 4MB because larger values
are not supported on macOS by default. This value should be enough
for all systems because notification messages are usually less than 1KB.

Fixes #4436
---
  CMakeLists.txt            |  1 +
  src/systemd.c             | 38 +++++++++++++++++++++++++++++++++-----
  src/systemd.h             |  6 +++---
  src/trivia/config.h.cmake |  4 ++--
  4 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index bfb15effb..86b81f04b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -282,6 +282,7 @@ find_package_message(MODULE_LUAPATH "Lua 
package.path: ${MODULE_LUAPATH}"
  find_package_message(MODULE_LIBPATH "Lua package.cpath: ${MODULE_LIBPATH}"
      "${MODULE_LIBPATH}")

+option(WITH_NOTIFY_SOCKET "Enable notifications on NOTIFY_SOCKET" ON)

  ##
  ## Third-Party libraries
diff --git a/src/systemd.c b/src/systemd.c
index 073c9bb76..e8def7eae 100644
--- a/src/systemd.c
+++ b/src/systemd.c
@@ -30,7 +30,7 @@
   */
  #include "systemd.h"

-#if defined(WITH_SYSTEMD)
+#if defined(WITH_NOTIFY_SOCKET)
  #include <errno.h>
  #include <stdio.h>
  #include <stdarg.h>
@@ -48,6 +48,15 @@
  static int systemd_fd = -1;
  static const char *sd_unix_path = NULL;

+/*
+ * Linux supports MSG_NOSIGNAL flag for sendmsg.
+ * macOS lacks it, but has SO_NOSIGPIPE for setsockopt
+ * to achieve same behavior.
+ */
+#if !defined(MSG_NOSIGNAL) && !defined(SO_NOSIGPIPE)
+#error "No way to block SIGPIPE in sendmsg!"
+#endif
+
  int systemd_init() {
      sd_unix_path = getenv("NOTIFY_SOCKET");
      if (sd_unix_path == NULL) {
@@ -68,11 +77,25 @@ int systemd_init() {
          say_error("systemd: NOTIFY_SOCKET is longer that MAX_UNIX_PATH");
          goto error;
      }
-    if ((systemd_fd = socket(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0)) == 
-1) {
+    if ((systemd_fd = socket(AF_UNIX, SOCK_DGRAM, 0)) == -1) {
          say_syserror("systemd: failed to create unix socket");
          goto error;
      }
-    int sndbuf_sz = 8 * 1024 * 1024;
+    if (fcntl(systemd_fd, F_SETFD, FD_CLOEXEC) == -1) {
+        say_syserror("systemd: fcntl failed to set FD_CLOEXEC");
+        goto error;
+    }
+
+    #ifdef SO_NOSIGPIPE
+    int val = 1;
+    if (setsockopt(systemd_fd, SOL_SOCKET, SO_NOSIGPIPE, (void*)&val,
+            sizeof(val)) < 0) {
+        say_syserror("systemd: failed to set NOSIGPIPE");
+        goto error;
+    }
+    #endif
+
+    int sndbuf_sz = 4 * 1024 * 1024;
      if (setsockopt(systemd_fd, SOL_SOCKET, SO_SNDBUF, &sndbuf_sz,
                sizeof(int)) < 0) {
          say_syserror("systemd: failed to set sndbuf size");
@@ -106,8 +129,13 @@ int systemd_notify(const char *message) {
          sa.sun_path[0] = '\0';

      say_debug("systemd: sending message '%s'", message);
+    #ifdef MSG_NOSIGNAL
+    int flags = MSG_NOSIGNAL;
+    #else
+    int flags = 0;
+    #endif
      ssize_t sent = sendto(systemd_fd, message, (size_t) strlen(message),
-        MSG_NOSIGNAL, (struct sockaddr *) &sa, sizeof(sa));
+        flags, (struct sockaddr *) &sa, sizeof(sa));
      if (sent == -1) {
          say_syserror("systemd: failed to send message");
          return -1;
@@ -142,4 +170,4 @@ systemd_snotify(const char *format, ...)
      va_end(args);
      return res;
  }
-#endif /* defined(WITH_SYSTEMD) */
+#endif /* defined(WITH_NOTIFY_SOCKET) */
diff --git a/src/systemd.h b/src/systemd.h
index f4f36e2cf..861a9af35 100644
--- a/src/systemd.h
+++ b/src/systemd.h
@@ -39,7 +39,7 @@
  extern "C" {
  #endif /* defined(__cplusplus) */

-#if defined(WITH_SYSTEMD)
+#if defined(WITH_NOTIFY_SOCKET)
  /**
   * Open connection with systemd daemon (using unix socket located in
   * "NOTIFY_SOCKET" environmnent variable)
@@ -95,13 +95,13 @@ systemd_vsnotify(const char *format, va_list ap);
  int
  systemd_snotify(const char *format, ...);

-#else /* !defined(WITH_SYSTEMD) */
+#else /* !defined(WITH_NOTIFY_SOCKET) */
  #  define systemd_init()
  #  define systemd_free()
  #  define systemd_notify(...)
  #  define systemd_vsnotify(...)
  #  define systemd_snotify(...)
-#endif /* WITH_SYSTEMD */
+#endif /* WITH_NOTIFY_SOCKET */

  #if defined(__cplusplus)
  } /* extern "C" */
diff --git a/src/trivia/config.h.cmake b/src/trivia/config.h.cmake
index ca0057d2b..ad0bd134a 100644
--- a/src/trivia/config.h.cmake
+++ b/src/trivia/config.h.cmake
@@ -202,9 +202,9 @@
  #cmakedefine HAVE_ICU_STRCOLLUTF8 1

  /*
-* Defined if systemd is enabled
+* Defined if notifications on NOTIFY_SOCKET are enabled
   */
-#cmakedefine WITH_SYSTEMD 1
+#cmakedefine WITH_NOTIFY_SOCKET 1

  /** \cond public */

--
2.21.0

On 19/08/2019 22:33, Alexander Turenko wrote:
> I don't have objections against the approach at whole. Please, consider
> comments below about details.
>
> Please, also file an issue with a problem statement (if there is no one)
> and close it from the commit message.
>
> WBR, Alexander Turenko.
>
> On Mon, Aug 19, 2019 at 11:28:13AM +0300, Max Melentiev wrote:
>> To make it possible to develop and test related features on macOS.
>>
>> SOCK_CLOEXEC (not available on macOS) flag for socket()
>> is replaced with `fcntl(fd, F_SETFD, FD_CLOEXEC)` which has the same effect.
>>
>> MSG_NOSIGNAL flag for sendmsg is also not available on macOS.
>> However it has SO_NOSIGPIPE flag for setsockopt which disables SIGPIPE.
>> So it requires different solution for different OS.
>> Inspired by https://nwat.xyz/blog/2014/01/16/porting-msg_more-and-msg_nosigpipe-to-osx/
>>
>> `sendmsg()` is replaced with `sendto()` because `msg_namelen` was calculated incorrectly.
>> New method builds message automatically without errors.
> Nit: A commit message body should be 72 symbols wide at max. Please,
> consider the developer guide:
> https://www.tarantool.io/en/doc/1.10/dev_guide/developer_guidelines/#how-to-write-a-commit-message
>
> Are there any other reason to replace sendmsg() with sendto()? It seems
> it is not much needed here, so I'm okay with sendto()--it even shrink
> the code--but please clarify such points in the commit message. I often
> investigating problems over old commits and explicit motivation of
> changes is really helpful. Even if it is 'X had broken in A way and I
> decided to don't fix it and use Y' -- it makes clear that there was no
> other reasons.
>
>> ---
> Please, don't forget to leave an issue and a branch here. Also, please
> add me to 'To' or 'CC' email headers when expect a review from me;
> otherwise I'll likely will miss your email.
>
>>   src/systemd.c             | 34 ++++++++++++++++++----------------
>>   src/systemd.h             | 25 ++++++++++++++++---------
>>   src/trivia/config.h.cmake |  5 -----
>>   3 files changed, 34 insertions(+), 30 deletions(-)
>>
>> diff --git a/src/systemd.c b/src/systemd.c
>> index b6c48afe2..f49a7f3f9 100644
>> --- a/src/systemd.c
>> +++ b/src/systemd.c
>> @@ -30,7 +30,6 @@
>>    */
>>   #include "systemd.h"
>>   
>> -#if defined(WITH_SYSTEMD)
> I see you had enabled the flag unconditionally, so it can be removed
> entirely from cmake, rpm and debian scripts. Please, grep it and remove
> so. And also update the commit message to reflect this change.
>
>>   #include <errno.h>
>>   #include <stdio.h>
>>   #include <stdarg.h>
>> @@ -68,11 +67,25 @@ int systemd_init() {
>>   		say_error("systemd: NOTIFY_SOCKET is longer that MAX_UNIX_PATH");
>>   		goto error;
>>   	}
>> -	if ((systemd_fd = socket(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, 0)) == -1) {
>> +	if ((systemd_fd = socket(AF_UNIX, SOCK_DGRAM, 0)) == -1) {
>>   		say_syserror("systemd: failed to create unix socket");
>>   		goto error;
>>   	}
>> -	int sndbuf_sz = 8 * 1024 * 1024;
>> +	if (fcntl(systemd_fd, F_SETFD, FD_CLOEXEC) == -1) {
>> +		say_syserror("systemd: fcntl failed to set FD_CLOEXEC");
>> +		goto error;
>> +	}
>> +
>> +	#ifdef SYSTEMD_USE_SO_NOSIGPIPE
>> +	int val = 1;
>> +	if (setsockopt(systemd_fd, SOL_SOCKET, SO_NOSIGPIPE, (void*)&val,
>> +			sizeof(val)) < 0) {
>> +		say_syserror("systemd: failed to set NOSIGPIPE");
>> +		goto error;
>> +	}
>> +	#endif
>> +
>> +	int sndbuf_sz = 4 * 1024 * 1024;
> It was changed from 8 MiB to 4 Mib and this change is not mentioned in
> the commit message. Need to be clarified.
>
>>   	if (setsockopt(systemd_fd, SOL_SOCKET, SO_SNDBUF, &sndbuf_sz,
>>   		      sizeof(int)) < 0) {
>>   		say_syserror("systemd: failed to set sndbuf size");
>> @@ -100,23 +113,13 @@ int systemd_notify(const char *message) {
>>   	struct sockaddr_un sa = {
>>   		.sun_family = AF_UNIX,
>>   	};
>> -	struct iovec vec = {
>> -		.iov_base = (char  *)message,
>> -		.iov_len  = (size_t )strlen(message)
>> -	};
>> -	struct msghdr msg = {
>> -		.msg_iov    = &vec,
>> -		.msg_iovlen = 1,
>> -		.msg_name   = &sa,
>> -	};
>> -
>> -	msg.msg_namelen = sizeof(sa.sun_family) + strlen(sd_unix_path);
>>   	strncpy(sa.sun_path, sd_unix_path, sizeof(sa.sun_path));
>>   	if (sa.sun_path[0] == '@')
>>   		sa.sun_path[0] = '\0';
>>   
>>   	say_debug("systemd: sending message '%s'", message);
>> -	ssize_t sent = sendmsg(systemd_fd, &msg, MSG_NOSIGNAL);
>> +	ssize_t sent = sendto(systemd_fd, message, (size_t)strlen(message),
>> +		SYSTEMD_MSG_NOSIGNAL, (struct sockaddr*)&sa, sizeof(sa));
>>   	if (sent == -1) {
>>   		say_syserror("systemd: failed to send message");
>>   		return -1;
>> @@ -151,4 +154,3 @@ systemd_snotify(const char *format, ...)
>>   	va_end(args);
>>   	return res;
>>   }
>> -#endif /* defined(WITH_SYSTEMD) */
>> diff --git a/src/systemd.h b/src/systemd.h
>> index f4f36e2cf..f86159581 100644
>> --- a/src/systemd.h
>> +++ b/src/systemd.h
>> @@ -35,11 +35,26 @@
>>   
>>   #include "trivia/config.h"
>>   
>> +/*
>> + * Linux supports MSG_NOSIGNAL flag for sendmsg.
>> + * macOS lacks it, but has SO_NOSIGPIPE for setsockopt to achieve same behaviour.
> Nit: Our code style requires to fit a comment to 66 symbols.
>
>> + */
>> +#ifdef MSG_NOSIGNAL
>> +# define SYSTEMD_MSG_NOSIGNAL MSG_NOSIGNAL
>> +#else
>> +# define SYSTEMD_MSG_NOSIGNAL 0
>> +# include <sys/socket.h>
>> +# ifdef SO_NOSIGPIPE
>> +#  define SYSTEMD_USE_SO_NOSIGPIPE
>> +# else
>> +#  error "No way to block SIGPIPE in sendmsg!"
>> +# endif
>> +#endif
>> +
> It is better to move the block to .c file (esp. #include). I would even
> perform needed checks right in the code:
>
>   | #if !defined(MSG_NOSIGNAL) && !defined(SO_NOSIGPIPE)
>   | #error <...>
>   | #endif
>
>   | int flags = 0;
>   | #ifdef MSG_NOSIGNAL
>   |     /* A comment here. */
>   |     flags |= MSG_NOSIGNAL;
>   | #endif
>   |     sendto(..., flags, ...);
>
>   | #if defined(SO_NOSIGPIPE) && !defined(MSG_NOSIGNAL)
>   |     /* A comment here. */
>   |     setsockopt(..., SO_NOSIGPIPE, ...);
>   | #endif
>
>>   #if defined(__cplusplus)
>>   extern "C" {
>>   #endif /* defined(__cplusplus) */
>>   
>> -#if defined(WITH_SYSTEMD)
>>   /**
>>    * Open connection with systemd daemon (using unix socket located in
>>    * "NOTIFY_SOCKET" environmnent variable)
>> @@ -95,14 +110,6 @@ systemd_vsnotify(const char *format, va_list ap);
>>   int
>>   systemd_snotify(const char *format, ...);
>>   
>> -#else /* !defined(WITH_SYSTEMD) */
>> -#  define systemd_init()
>> -#  define systemd_free()
>> -#  define systemd_notify(...)
>> -#  define systemd_vsnotify(...)
>> -#  define systemd_snotify(...)
>> -#endif /* WITH_SYSTEMD */
>> -
>>   #if defined(__cplusplus)
>>   } /* extern "C" */
>>   #endif /* defined(__cplusplus) */
>> diff --git a/src/trivia/config.h.cmake b/src/trivia/config.h.cmake
>> index ca0057d2b..f50f64a02 100644
>> --- a/src/trivia/config.h.cmake
>> +++ b/src/trivia/config.h.cmake
>> @@ -201,11 +201,6 @@
>>    */
>>   #cmakedefine HAVE_ICU_STRCOLLUTF8 1
>>   
>> -/*
>> -* Defined if systemd is enabled
>> - */
>> -#cmakedefine WITH_SYSTEMD 1
>> -
>>   /** \cond public */
>>   
>>   /** System configuration dir (e.g /etc) */
>> -- 
>> 2.21.0
>>
>>
>