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

[Alexander Turenko <alexander.turenko@tarantool.org>]

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