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