From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C74F646970F for ; Tue, 26 Nov 2019 02:23:28 +0300 (MSK) References: <1574629939.202877645@f184.i.mail.ru> From: Vladislav Shpilevoy Message-ID: <08dd3b3a-bcdf-f1fd-4591-b2d70d622697@tarantool.org> Date: Tue, 26 Nov 2019 00:23:26 +0100 MIME-Version: 1.0 In-Reply-To: <1574629939.202877645@f184.i.mail.ru> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] build: suppress the warning for GCC >= 8 List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chris Sosnin , tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! See 5 comments below. On 24/11/2019 22:12, Chris Sosnin wrote: > As it is mentioned in #4515, strncpy() warning needs > to be suppressed so it does not fail the build with > -DENABLE_WERROR=ON. > > Closes: #4515 1. Please, don't use ':' in github tags. It just is not our commit message style. > --- 2. Specify branch and issue links here before sending the email. Otherwise I need to spend some time to find your branch by an issue number. What may be complicated, when an issue has many branches. 3. On the branch (ksosnin/gh-4515-build-warning) I see 2 commits. Here I see one. Please, send the latest and the most actual version. > src/systemd.c | 2 ++ > src/systemd.h | 13 +++++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/src/systemd.c b/src/systemd.c > index 6686c3ce0..8e81b274c 100644 > --- a/src/systemd.c > +++ b/src/systemd.c > @@ -117,7 +117,9 @@ int systemd_notify(const char *message) { > .sun_family = AF_UNIX, > }; > > + WSTRINGOP_TRUNC_OFF > strncpy(sa.sun_path, sd_unix_path, sizeof(sa.sun_path)); > + WSTRINGOP_TRUNC_ON > if (sa.sun_path[0] == '@') 4. In fact, the initial advice in the issue is wrong. We should not ignore this error. Sun_path is expected to be zero terminated (at least on Linux). Cite: "for maximum portability and ease of coding" http://man7.org/linux/man-pages/man7/unix.7.html. As Alexander said, on some implementations it may be not required. But on others it may be. So just fix the warning. Don't ignore it. It is not worth trying to save the one byte in a hope, that all supported versions of all supported OSs are able to cope with not zero terminated path. > sa.sun_path[0] = '\0'; > > diff --git a/src/systemd.h b/src/systemd.h > index 861a9af35..854405847 100644 > --- a/src/systemd.h > +++ b/src/systemd.h > @@ -107,4 +107,17 @@ systemd_snotify(const char *format, ...); > } /* extern "C" */ > #endif /* defined(__cplusplus) */ > > +#ifdef __GNUC__ > +#if __GNUC__ >= 8 > +#define WSTRINGOP_TRUNC_OFF \ > + _Pragma ("GCC diagnostic push") \ > + _Pragma ("GCC diagnostic ignored \"-Wstringop-truncation\"") > +#define WSTRINGOP_TRUNC_ON \ > + _Pragma ("GCC diagnostic pop") > +#endif > +#else > +#define WSTRINGOP_TRUNC_OFF > +#define WSTRINGOP_TRUNC_ON > +#endif > + 5. In case I had agreed with necessity to ignore the warning, I would have said, that we should not put these ignore instructions here. We have trivia/util.h for that. And we would have needed a better name than WSTRINGOP. But anyway the warning is valid, so this piece of code is not needed. > #endif /* TARANTOOL_SYSTEMD_H_INCLUDED */ > -- > 2.24.0 >