From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 AC80C4696C3 for ; Wed, 15 Apr 2020 20:28:33 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\)) From: Serge Petrenko In-Reply-To: Date: Wed, 15 Apr 2020 20:28:31 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <8DB58B24-A230-4CFA-8FEB-10073767D364@tarantool.org> References: Subject: Re: [Tarantool-patches] [PATCH 4/4] feedback: add cmake option to disable the daemon List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the patchset! All LGTM except one comment below. > 12 =D0=B0=D0=BF=D1=80. 2020 =D0=B3., =D0=B2 23:13, Vladislav Shpilevoy = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0= =B0): >=20 > There is a complaint that the feedback daemon is a 'spying' tool > and because of that can't be used on Gentoo. Its default disabled > option also is not acceptable, the daemon should be eliminated > completely. >=20 > The patch introduces cmake option ENABLE_FEEDBACK_DAEMON. It is > ON by default. When set to OFF, all feedback daemon's code is not > included into the binary, its configuration options disappear. >=20 > Closes #3308 > --- > src/box/CMakeLists.txt | 10 ++++++++- > src/box/lua/init.c | 9 +++++++++ > src/box/lua/load_cfg.lua | 29 +++++++++++++++++++++------ > src/trivia/config.h.cmake | 2 ++ > test/box-tap/feedback_daemon.test.lua | 23 +++++++++++++++++---- > test/box/misc.result | 19 ++++++++++++++++-- > test/box/misc.test.lua | 15 +++++++++++++- > 7 files changed, 93 insertions(+), 14 deletions(-) >=20 > diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt > index 56758bd2f..4827e786e 100644 > --- a/src/box/CMakeLists.txt > +++ b/src/box/CMakeLists.txt > @@ -1,5 +1,11 @@ > file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/src/box/lua) >=20 > +# Sometimes 'spying' code is not acceptable even if it would be > +# disabled by default. That option allows to remove the feedback > +# daemon from the build completely. > +option(ENABLE_FEEDBACK_DAEMON "Feedback daemon which reports debug = data to \ > + the Tarantool team" ON) > + > add_subdirectory(sql) >=20 > set(lua_sources) > @@ -7,7 +13,9 @@ lua_source(lua_sources lua/load_cfg.lua) > lua_source(lua_sources lua/schema.lua) > lua_source(lua_sources lua/tuple.lua) > lua_source(lua_sources lua/session.lua) > -lua_source(lua_sources lua/feedback_daemon.lua) > +if (ENABLE_FEEDBACK_DAEMON) > + lua_source(lua_sources lua/feedback_daemon.lua) > +endif() > lua_source(lua_sources lua/net_box.lua) > lua_source(lua_sources lua/upgrade.lua) > lua_source(lua_sources lua/console.lua) > diff --git a/src/box/lua/init.c b/src/box/lua/init.c > index 9db740de6..29217c21e 100644 > --- a/src/box/lua/init.c > +++ b/src/box/lua/init.c > @@ -71,7 +71,9 @@ extern char session_lua[], > schema_lua[], > load_cfg_lua[], > xlog_lua[], > +#if ENABLE_FEEDBACK_DAEMON > feedback_daemon_lua[], > +#endif > net_box_lua[], > upgrade_lua[], > console_lua[], > @@ -81,7 +83,14 @@ static const char *lua_sources[] =3D { > "box/session", session_lua, > "box/tuple", tuple_lua, > "box/schema", schema_lua, > +#if ENABLE_FEEDBACK_DAEMON > + /* > + * It is important to initialize the daemon before > + * load_cfg, because the latter peeks up some values Peeks -> Picks ? > + * from the feedback daemon. > + */ > "box/feedback_daemon", feedback_daemon_lua, > +#endif > "box/upgrade", upgrade_lua, > "box/net_box", net_box_lua, > "box/console", console_lua, > diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua > index b671eb7a2..5d818addf 100644 > --- a/src/box/lua/load_cfg.lua > +++ b/src/box/lua/load_cfg.lua > @@ -21,6 +21,20 @@ local function locked(f) > end > end >=20 > +-- > +-- When feedback is disabled, every single mentioning of it should > +-- be eliminated. Even box.cfg{} should not accept any > +-- 'feedback_*' parameters as valid. This is why they are set to > +-- nil, when the daemon does not exist. > +-- > +local function ifdef_feedback(value) > + return private.feedback_daemon ~=3D nil and value or nil > +end > + > +local ifdef_feedback_set_params =3D > + private.feedback_daemon ~=3D nil and > + private.feedback_daemon.set_feedback_params or nil > + > -- all available options > local default_cfg =3D { > listen =3D nil, > @@ -143,9 +157,9 @@ local template_cfg =3D { > replication_connect_quorum =3D 'number', > replication_skip_conflict =3D 'boolean', > replication_anon =3D 'boolean', > - feedback_enabled =3D 'boolean', > - feedback_host =3D 'string', > - feedback_interval =3D 'number', > + feedback_enabled =3D ifdef_feedback('boolean'), > + feedback_host =3D ifdef_feedback('string'), > + feedback_interval =3D ifdef_feedback('number'), > net_msg_max =3D 'number', > sql_cache_size =3D 'number', > } > @@ -236,9 +250,9 @@ local dynamic_cfg =3D { > checkpoint_interval =3D private.cfg_set_checkpoint_interval, > checkpoint_wal_threshold =3D = private.cfg_set_checkpoint_wal_threshold, > worker_pool_threads =3D private.cfg_set_worker_pool_threads, > - feedback_enabled =3D = private.feedback_daemon.set_feedback_params, > - feedback_host =3D = private.feedback_daemon.set_feedback_params, > - feedback_interval =3D = private.feedback_daemon.set_feedback_params, > + feedback_enabled =3D ifdef_feedback_set_params, > + feedback_host =3D ifdef_feedback_set_params, > + feedback_interval =3D ifdef_feedback_set_params, > -- do nothing, affects new replicas, which query this value on = start > wal_dir_rescan_delay =3D function() end, > custom_proc_title =3D function() > @@ -258,6 +272,9 @@ local dynamic_cfg =3D { > sql_cache_size =3D private.cfg_set_sql_cache_size, > } >=20 > +ifdef_feedback =3D nil > +ifdef_feedback_set_params =3D nil > + > -- > -- For some options it is important in which order they are set. > -- For example, setting 'replication', including self, before > diff --git a/src/trivia/config.h.cmake b/src/trivia/config.h.cmake > index 226f83128..89e0d39c6 100644 > --- a/src/trivia/config.h.cmake > +++ b/src/trivia/config.h.cmake > @@ -251,6 +251,8 @@ > /* Cacheline size to calculate alignments */ > #define CACHELINE_SIZE 64 >=20 > +#cmakedefine ENABLE_FEEDBACK_DAEMON 1 > + > /* > * vim: syntax=3Dc > */ > diff --git a/test/box-tap/feedback_daemon.test.lua = b/test/box-tap/feedback_daemon.test.lua > index 9e6c59efc..d4adb71f1 100755 > --- a/test/box-tap/feedback_daemon.test.lua > +++ b/test/box-tap/feedback_daemon.test.lua > @@ -8,8 +8,6 @@ local fiber =3D require('fiber') > local test =3D tap.test('feedback_daemon') > local socket =3D require('socket') >=20 > -test:plan(11) > - > box.cfg{log =3D 'report.log', log_level =3D 6} >=20 > local feedback_save =3D nil > @@ -49,10 +47,27 @@ local server =3D socket.tcp_server("127.0.0.1", 0, = http_handle) > local port =3D server:name().port >=20 > local interval =3D 0.01 > -box.cfg{ > +local ok, err =3D pcall(box.cfg, { > feedback_host =3D string.format("127.0.0.1:%i", port), > feedback_interval =3D interval, > -} > +}) > + > +if not ok then > + -- > + -- gh-3308: feedback daemon is an optional pre-compile time > + -- defined feature, depending on CMake flags. It is not > + -- present always. When it is disabled, is should not be > + -- visible anywhere. > + -- > + test:plan(2) > + test:diag('Feedback daemon is supposed to be disabled') > + test:like(err, 'unexpected option', 'Feedback options are = undefined') > + test:isnil(box.feedback, 'No feedback daemon - no its API') > + test:check() > + os.exit(0) > +end > + > +test:plan(11) >=20 > local function check(message) > while feedback_count < 1 do > diff --git a/test/box/misc.result b/test/box/misc.result > index 9b76d7665..10699adc2 100644 > --- a/test/box/misc.result > +++ b/test/box/misc.result > @@ -49,8 +49,24 @@ index =3D space:create_index('primary', { type =3D = 'hash' }) > --- > - ' lua says: hello' > ... > +-- > +-- gh-3308: feedback daemon is an optional pre-compile time > +-- defined feature, depending on CMake flags. It is not present > +-- always. > +-- > +optional =3D {feedback =3D true} > +--- > +... > -- # What's in the box? > -t =3D {} for n in pairs(box) do table.insert(t, tostring(n)) end = table.sort(t) > +t =3D {} > +--- > +... > +for n in pairs(box) do = \ > + if not optional[n] then = \ > + table.insert(t, tostring(n)) = \ > + end = \ > +end = \ > +table.sort(t) > --- > ... > t > @@ -64,7 +80,6 @@ t > - ctl > - error > - execute > - - feedback > - func > - index > - info > diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua > index 350f9f75d..4d7fb0a16 100644 > --- a/test/box/misc.test.lua > +++ b/test/box/misc.test.lua > @@ -17,8 +17,21 @@ index =3D space:create_index('primary', { type =3D = 'hash' }) > -- Test Lua from admin console. Whenever producing output, > -- make sure it's a valid YAML. > ' lua says: hello' > + > +-- > +-- gh-3308: feedback daemon is an optional pre-compile time > +-- defined feature, depending on CMake flags. It is not present > +-- always. > +-- > +optional =3D {feedback =3D true} > -- # What's in the box? > -t =3D {} for n in pairs(box) do table.insert(t, tostring(n)) end = table.sort(t) > +t =3D {} > +for n in pairs(box) do = \ > + if not optional[n] then = \ > + table.insert(t, tostring(n)) = \ > + end = \ > +end = \ > +table.sort(t) > t > t =3D nil >=20 > --=20 > 2.21.1 (Apple Git-122.3) >=20 -- Serge Petrenko sergepetrenko@tarantool.org