From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 4707725238 for ; Mon, 19 Aug 2019 15:33:52 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 6ehzyFAegdSL for ; Mon, 19 Aug 2019 15:33:52 -0400 (EDT) Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 75A1F24488 for ; Mon, 19 Aug 2019 15:33:51 -0400 (EDT) Date: Mon, 19 Aug 2019 22:33:33 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] Enable support for NOTIFY_SOCKET on macOS Message-ID: <20190819193333.z525sey6oe2dbnvg@tkn_work_nb> References: <20190819082813.38237-1-m.melentiev@corp.mail.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190819082813.38237-1-m.melentiev@corp.mail.ru> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Max Melentiev Cc: tarantool-patches@freelists.org 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 > #include > #include > @@ -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 > +# 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 > >