[tarantool-patches] Re: [PATCH] Enable support for NOTIFY_SOCKET on macOS
Alexander Turenko
alexander.turenko at tarantool.org
Thu Aug 22 01:45:58 MSK 2019
> ${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.
More information about the Tarantool-patches
mailing list