[PATCH] box/memtx: strip_core -- Warn on linux only

[PATCH] box/memtx: strip_core -- Warn on linux only

[Cyrill Gorcunov]

We know that madvise(MADV_DONTDUMP) is present
on linux based platforms only (macos doesn't
support it at all, freebsd requires MADV_NOCORE
or similar which is unsupported by now) thus
we should not print a warning on other systems
to not spam users.

Fixes #4464
---
Sasha, could you please check if it fixes the problem?

 src/main.cc | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/main.cc b/src/main.cc
index 5776aa41d..b16cfa5fe 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -496,7 +496,19 @@ load_cfg()
 	 */
 	if (cfg_geti("strip_core")) {
 		if (!small_test_feature(SMALL_FEATURE_DONTDUMP)) {
-			say_warn("'strip_core' is set but unsupported");
+			static const char strip_msg[] =
+				"'strip_core' is set but unsupported";
+#ifdef TARGET_OS_LINUX
+			/*
+			 * Linux is known to support madvise(DONT_DUMP)
+			 * feature, thus warn on this platform only. The
+			 * rest should be notified on verbose level only
+			 * to not spam a user.
+			 */
+			say_warn(strip_msg);
+#else
+			say_verbose(strip_msg);
+#endif
 		}
 	}
 
-- 
2.20.1

Re: [tarantool-patches] [PATCH] box/memtx: strip_core -- Warn on linux only

[Konstantin Osipov]

* Cyrill Gorcunov <gorcunov@gmail.com> [19/08/28 21:17]:
> We know that madvise(MADV_DONTDUMP) is present
> on linux based platforms only (macos doesn't
> support it at all, freebsd requires MADV_NOCORE
> or similar which is unsupported by now) thus
> we should not print a warning on other systems
> to not spam users.
> 
> Fixes #4464

lgtm.


-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH] box/memtx: strip_core -- Warn on linux only

[Alexander Turenko]

LGTM.

My thoughts on the topic are below.

WBR, Alexander Turenko.

On Wed, Aug 28, 2019 at 09:13:22PM +0300, Cyrill Gorcunov wrote:
> We know that madvise(MADV_DONTDUMP) is present
> on linux based platforms only (macos doesn't
> support it at all, freebsd requires MADV_NOCORE
> or similar which is unsupported by now) thus
> we should not print a warning on other systems
> to not spam users.
> 
> Fixes #4464
> ---
> Sasha, could you please check if it fixes the problem?

The warning is not shown on Mac OS at box.cfg{} after the patch.

> 
>  src/main.cc | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/src/main.cc b/src/main.cc
> index 5776aa41d..b16cfa5fe 100644
> --- a/src/main.cc
> +++ b/src/main.cc
> @@ -496,7 +496,19 @@ load_cfg()
>  	 */
>  	if (cfg_geti("strip_core")) {
>  		if (!small_test_feature(SMALL_FEATURE_DONTDUMP)) {
> -			say_warn("'strip_core' is set but unsupported");
> +			static const char strip_msg[] =
> +				"'strip_core' is set but unsupported";
> +#ifdef TARGET_OS_LINUX
> +			/*
> +			 * Linux is known to support madvise(DONT_DUMP)
> +			 * feature, thus warn on this platform only. The
> +			 * rest should be notified on verbose level only
> +			 * to not spam a user.
> +			 */
> +			say_warn(strip_msg);
> +#else
> +			say_verbose(strip_msg);
> +#endif

I think it is okay: it is the minor thing and an easiest way to solve is
the better way.

If we'll have more OS dependent features (hopefully we'll not), then
maybe it will be worth to do two things:

* Check whether a user asks to enable something explicitly (or it was
  set by default); give a warning only for an explicit choose.
* Check whether libsmall was compiled with a feature supported and don't
  give a warning if a tarantool build does not support a feature on a
  platform at all (it is instead of TARGET_OS_LINUX check).

Those bullets implemented will give us the following logic: give a
warning only if a user explicitly asks a feature AND it is supported on
a platform AND it is not supported at runtime.

Re: [PATCH] box/memtx: strip_core -- Warn on linux only

[Cyrill Gorcunov]

On Thu, Aug 29, 2019 at 03:19:05PM +0300, Alexander Turenko wrote:
...
> 
> I think it is okay: it is the minor thing and an easiest way to solve is
> the better way.
> 
> If we'll have more OS dependent features (hopefully we'll not), then
> maybe it will be worth to do two things:
> 
> * Check whether a user asks to enable something explicitly (or it was
>   set by default); give a warning only for an explicit choose.

We will need to track then how exactly the feature is set up -- currently
we simply don't have such ability. IOW, we will need two seprate tables:
one for default values and second for runtime values. If it is acceptable
I could try to implement it.

> * Check whether libsmall was compiled with a feature supported and don't
>   give a warning if a tarantool build does not support a feature on a
>   platform at all (it is instead of TARGET_OS_LINUX check).

Wait, do you propose to do similar compile-time check for madvise
feature inside tarantool code?

> Those bullets implemented will give us the following logic: give a
> warning only if a user explicitly asks a feature AND it is supported on
> a platform AND it is not supported at runtime.

Sounds reasonable.

Re: [tarantool-patches] [PATCH] box/memtx: strip_core -- Warn on linux only

[Kirill Yukhin]

Hello,

On 28 авг 21:13, Cyrill Gorcunov wrote:
> We know that madvise(MADV_DONTDUMP) is present
> on linux based platforms only (macos doesn't
> support it at all, freebsd requires MADV_NOCORE
> or similar which is unsupported by now) thus
> we should not print a warning on other systems
> to not spam users.
> 
> Fixes #4464

I've checked your patch into 2.2 and master.

--
Regards, Kirill Yukhin

Re: [PATCH] box/memtx: strip_core -- Warn on linux only

[Alexander Turenko]

On Thu, Aug 29, 2019 at 09:55:43PM +0300, Cyrill Gorcunov wrote:
> On Thu, Aug 29, 2019 at 03:19:05PM +0300, Alexander Turenko wrote:
> ...
> > 
> > I think it is okay: it is the minor thing and an easiest way to solve is
> > the better way.
> > 
> > If we'll have more OS dependent features (hopefully we'll not), then
> > maybe it will be worth to do two things:
> > 
> > * Check whether a user asks to enable something explicitly (or it was
> >   set by default); give a warning only for an explicit choose.
> 
> We will need to track then how exactly the feature is set up -- currently
> we simply don't have such ability. IOW, we will need two seprate tables:
> one for default values and second for runtime values. If it is acceptable
> I could try to implement it.

I think that this certain issue is not enough reason to do so.

> 
> > * Check whether libsmall was compiled with a feature supported and don't
> >   give a warning if a tarantool build does not support a feature on a
> >   platform at all (it is instead of TARGET_OS_LINUX check).
> 
> Wait, do you propose to do similar compile-time check for madvise
> feature inside tarantool code?

I imagine it as two functions in small: one for check whether madvise is
compiled-in (i.e. it is expected to be supported on a platform) and
another (existing now) whether it is actually works. After that we can
separately check 'is it supported on a platform' and 'is it works' from
tarantool code to implement the logic described below.

Anyway, I would not bother with all this stuff until at least one
another feature will require something like that.

> 
> > Those bullets implemented will give us the following logic: give a
> > warning only if a user explicitly asks a feature AND it is supported on
> > a platform AND it is not supported at runtime.
> 
> Sounds reasonable.

Re: [PATCH] box/memtx: strip_core -- Warn on linux only

[Cyrill Gorcunov]

On Fri, Aug 30, 2019 at 01:16:15AM +0300, Alexander Turenko wrote:
> > Wait, do you propose to do similar compile-time check for madvise
> > feature inside tarantool code?
> 
> I imagine it as two functions in small: one for check whether madvise is
> compiled-in (i.e. it is expected to be supported on a platform) and
> another (existing now) whether it is actually works. After that we can
> separately check 'is it supported on a platform' and 'is it works' from
> tarantool code to implement the logic described below.
> 
> Anyway, I would not bother with all this stuff until at least one
> another feature will require something like that.

I see, thanks!