Re: [Tarantool-patches] [PATCH] build: suppress the warning for GCC >= 8

[Vladislav Shpilevoy <v.shpilevoy@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
>