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

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

[Max Melentiev]

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.
---
 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)
 #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;
 	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.
+ */
+#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
+
 #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

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

[Alexander Turenko]

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
> 
> 

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

[Maxim Melentiev]

 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
>>
>>
>

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

[Konstantin Osipov]

* Maxim Melentiev <m.melentiev@corp.mail.ru> [19/08/21 11:32]:

this doesn't address my review.

If you disagree with the review, please reply to it. Otherwise
there is no point in sending a new patch.

-- 
Konstantin Osipov, Moscow, Russia

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

[Maxim Melentiev]

Does this work as you expect? Is there any reason to expose 
HAVE_MSG_NOSIGNAL to C code?

option(WITH_NOTIFY_SOCKET "Enable notifications on NOTIFY_SOCKET" ON)

if (WITH_SYSTEMD AND NOT WITH_NOTIFY_SOCKET)
     message(FATAL_ERROR "WITH_NOTIFY_SOCKET must be enabled when 
WITH_SYSTEMD enabled")
endif()

if (WITH_NOTIFY_SOCKET)
     # Linux supports MSG_NOSIGNAL flag for sendmsg.
     # macOS lacks it, but has SO_NOSIGPIPE for setsockopt
     # to achieve same behavior.
     CHECK_C_SOURCE_COMPILES("
         #include <sys/types.h>
         #include <sys/socket.h>
         int main(){ return MSG_NOSIGNAL; }
     " HAVE_MSG_NOSIGNAL)
     CHECK_C_SOURCE_COMPILES("
         #include <sys/types.h>
         #include <sys/socket.h>
         int main(){ return SO_NOSIGPIPE; }
     " HAVE_SO_NOSIGPIPE)
     if (NOT HAVE_MSG_NOSIGNAL AND NOT HAVE_SO_NOSIGPIPE)
         message(FATAL_ERROR
             "No way to block SIGPIPE in sendmsg. Can not compile with 
WITH_NOTIFY_SOCKET"
         )
     endif()
endif()

And C code uses #ifdef SO_NOSIGPIP, #ifdef MSG_NOSIGNAL

On 21/08/2019 18:52, Konstantin Osipov wrote:
> * Maxim Melentiev <m.melentiev@corp.mail.ru> [19/08/21 11:32]:
>
> this doesn't address my review.
>
> If you disagree with the review, please reply to it. Otherwise
> there is no point in sending a new patch.
>

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

[Konstantin Osipov]

* Maxim Melentiev <m.melentiev@corp.mail.ru> [19/08/22 12:04]:
> And C code uses #ifdef SO_NOSIGPIP, #ifdef MSG_NOSIGNAL

C code should use config.h defines, HAVE_MSG_NOSIGNAL,
HAVE_SO_NOSIGPIPE.

-- 
Konstantin Osipov, Moscow, Russia

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

[Maxim Melentiev]

 From d171f4a0721be0168e591aaa734c4729b96ea87c 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            | 28 +++++++++++++++++++++++++++-
  src/systemd.c             | 28 +++++++++++++++++++++++-----
  src/systemd.h             |  6 +++---
  src/trivia/config.h.cmake |  7 +++++--
  4 files changed, 58 insertions(+), 11 deletions(-)

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

-
  ##
  ## Third-Party libraries
  ##
@@ -482,6 +481,33 @@ add_subdirectory(extra)
  add_subdirectory(test)
  add_subdirectory(doc)

+option(WITH_NOTIFY_SOCKET "Enable notifications on NOTIFY_SOCKET" ON)
+
+if (WITH_SYSTEMD AND NOT WITH_NOTIFY_SOCKET)
+    message(FATAL_ERROR "WITH_NOTIFY_SOCKET must be enabled when 
WITH_SYSTEMD enabled")
+endif()
+
+if (WITH_NOTIFY_SOCKET)
+    check_c_source_compiles("
+        #include <sys/types.h>
+        #include <sys/socket.h>
+        int main(){ return MSG_NOSIGNAL; }
+    " HAVE_MSG_NOSIGNAL)
+    check_c_source_compiles("
+        #include <sys/types.h>
+        #include <sys/socket.h>
+        int main(){ return SO_NOSIGPIPE; }
+    " HAVE_SO_NOSIGPIPE)
+    # Linux supports MSG_NOSIGNAL flag for sendmsg.
+    # macOS lacks it, but has SO_NOSIGPIPE for setsockopt
+    # to achieve same behavior.
+    if (NOT HAVE_MSG_NOSIGNAL AND NOT HAVE_SO_NOSIGPIPE)
+        message(FATAL_ERROR
+            "No way to block SIGPIPE in sendmsg. Can not compile with 
WITH_NOTIFY_SOCKET"
+        )
+    endif()
+endif()
+
  if(NOT "${PROJECT_BINARY_DIR}" STREQUAL "${PROJECT_SOURCE_DIR}")
      add_custom_target(distclean)
      add_custom_command(TARGET distclean
diff --git a/src/systemd.c b/src/systemd.c
index 073c9bb76..0cc929274 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>
@@ -68,11 +68,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 HAVE_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 +120,12 @@ int systemd_notify(const char *message) {
          sa.sun_path[0] = '\0';

      say_debug("systemd: sending message '%s'", message);
+    int flags = 0;
+    #ifdef HAVE_MSG_NOSIGNAL
+        flags |= MSG_NOSIGNAL;
+    #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 +160,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..226f83128 100644
--- a/src/trivia/config.h.cmake
+++ b/src/trivia/config.h.cmake
@@ -171,6 +171,9 @@
  #cmakedefine HAVE_MREMAP 1
  #cmakedefine HAVE_SYNC_FILE_RANGE 1

+#cmakedefine HAVE_MSG_NOSIGNAL 1
+#cmakedefine HAVE_SO_NOSIGPIPE 1
+
  #cmakedefine HAVE_PRCTL_H 1

  #cmakedefine HAVE_UUIDGEN 1
@@ -202,9 +205,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 22/08/2019 13:06, Konstantin Osipov wrote:
> * Maxim Melentiev <m.melentiev@corp.mail.ru> [19/08/22 12:04]:
>> And C code uses #ifdef SO_NOSIGPIP, #ifdef MSG_NOSIGNAL
> C code should use config.h defines, HAVE_MSG_NOSIGNAL,
> HAVE_SO_NOSIGPIPE.
>

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

[Konstantin Osipov]

* Maxim Melentiev <m.melentiev@corp.mail.ru> [19/08/22 15:46]:
> From d171f4a0721be0168e591aaa734c4729b96ea87c 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

lgtm, but please fix #ifdef alignment


-- 
Konstantin Osipov, Moscow, Russia

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

[Maxim Melentiev]

fixed and pushed to 
https://github.com/tarantool/tarantool/tree/notify_socket_for_macos

On 22/08/2019 16:03, Konstantin Osipov wrote:
> * Maxim Melentiev <m.melentiev@corp.mail.ru> [19/08/22 15:46]:
>>  From d171f4a0721be0168e591aaa734c4729b96ea87c 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
> lgtm, but please fix #ifdef alignment
>
>

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

[Alexander Turenko]

I looked briefly again and I don't have any objections.

WBR, Alexander Turenko.

On Thu, Aug 22, 2019 at 05:27:25PM +0300, Maxim Melentiev wrote:
> fixed and pushed to
> https://github.com/tarantool/tarantool/tree/notify_socket_for_macos
> 
> On 22/08/2019 16:03, Konstantin Osipov wrote:
> > * Maxim Melentiev <m.melentiev@corp.mail.ru> [19/08/22 15:46]:
> > >  From d171f4a0721be0168e591aaa734c4729b96ea87c 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
> > lgtm, but please fix #ifdef alignment
> > 
> > 

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

[Kirill Yukhin]

Hello,

On 22 авг 17:27, Maxim Melentiev wrote:
> fixed and pushed to
> https://github.com/tarantool/tarantool/tree/notify_socket_for_macos

Please, in future use your nickname as a prefix to branch name.

--
Regards, Kirill Yukhin

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

[Alexander Turenko]

> ${MODULE_LUAPATH}"
>  find_package_message(MODULE_LIBPATH "Lua package.cpath: ${MODULE_LIBPATH}"
>      "${MODULE_LIBPATH}")
> 
> +option(WITH_NOTIFY_SOCKET "Enable notifications on NOTIFY_SOCKET" ON)

Kostya asks to enable it always when WITH_SYSTEMD is enabled. It is ON
be default, so I guess the acceptable way to achieve the requested
behaviour is to give an error when WITH_SYSTEMD is enabled, but
WITH_NOTIFY_SOCKET is disabled.

> +/*
> + * 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

I think it is better to give an error on the configuration step (check
it is cmake files).

> -    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

AFAIR, Kostya asks to set WITH_SO_NOSIGNAL / WITH_SO_NOSIGPIPE from
cmake.

Nit: We don't indent preprocessor directives.

>      say_debug("systemd: sending message '%s'", message);
> +    #ifdef MSG_NOSIGNAL
> +    int flags = MSG_NOSIGNAL;
> +    #else
> +    int flags = 0;
> +    #endif

Nit: I would write it so:

 | int flags = 0;
 | #ifdef WITH_MSG_NOSIGNAL
 |         flags |= MSG_NOSIGNAL
 | #endif

This way it is simpler to add a flag in a future. Don't worry about a
generated machine code: a compiler will perform the constant propagation
at compile time.

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

[Konstantin Osipov]

* Max Melentiev <dmarc-noreply@freelists.org> [19/08/19 11:31]:


> 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.

Just like Alexander, generally looks good, but a lot of nitpicks:

> -#if defined(WITH_SYSTEMD)

I don't think you should remove this ifdef at all. Simply enable
it on Mac OSX by default in cmake if it works there now.

>  #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;
> +	}

This hunk is good, but I agree that the new buffer size choice
need a comment. Better right in the source code. The old choice
was inexplicable, the new one is just as confusing.

> +
> +	#ifdef SYSTEMD_USE_SO_NOSIGPIPE

We prefer feature-test defines, not platform-specific
defines, which basically means you need to use 

https://cmake.org/cmake/help/v3.0/module/CheckCSourceCompiles.html

for checking whether SO_NOSIGPIPE and MSG_NOSIGNAL exist, 
define HAVE_SONOSIGPIPE and HAVE_MSG_NOSIGNAL in config.h.in, and
test them here.

A quick and hackish way is to look for TARGET_OS_DARWIN right
here. Inventing your own within-the-code define is ugly and not
acceptable.

Please keep in mind we also support FreeBSD - or at least pretend
we do.

-- 
Konstantin Osipov, Moscow, Russia

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

[Maxim Melentiev]

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

It's quite  complicated now: cmake WITH_SYSTEMD flag is also used to 
generate systemd units, tarantool-generator and other systemd related files.

I think that support for NOTIFY_SOCKET should not be systemd-related, as 
it's enabled in runtime by envar and is disabled otherwise.

On 19/08/2019 23:18, Konstantin Osipov wrote:
>> -#if defined(WITH_SYSTEMD)
> I don't think you should remove this ifdef at all. Simply enable
> it on Mac OSX by default in cmake if it works there now.
>

[-- Attachment #2: Type: text/html, Size: 1003 bytes --]

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

[Konstantin Osipov]

* Maxim Melentiev <m.melentiev@corp.mail.ru> [19/08/20 18:29]:
> It's quite  complicated now: cmake WITH_SYSTEMD flag is also used to
> generate systemd units, tarantool-generator and other systemd related files.
> 
> I think that support for NOTIFY_SOCKET should not be systemd-related, as
> it's enabled in runtime by envar and is disabled otherwise.

Then add WITH_NOITFY_SOCKET as a separate option. WITH_SYSTEMD
could imply it.

> On 19/08/2019 23:18, Konstantin Osipov wrote:
> > > -#if defined(WITH_SYSTEMD)
> > I don't think you should remove this ifdef at all. Simply enable
> > it on Mac OSX by default in cmake if it works there now.

-- 
Konstantin Osipov, Moscow, Russia

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

[Kirill Yukhin]

Hello,

On 19 авг 11:28, 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.

I've checked your patch into 2.2 and master.

--
Regards, Kirill Yukhin