Hi, Sergey,
thanks for review!
Fixed.Hi, Sergey! Thanks for the patch! LGTM, after fixing minor nits below. On 13.02.25, Sergey Bronnikov wrote:sysprof and memprof Lua API functions returns an error messageTypo: s/returns/return/
Fixed."profiler misuse", when appropriate profiler is disabled in build.Typo: s/appropriate/the corresponding/ Typo: s/build/the build/
Fixed.It is not possible to easily distinquish whether it is reallyTypo: s/distinquish/distinguish/
Fixed.misuse or profiler was not enabled in build. The patch changesTypo: s/profiler/or if the profiler/ Typo: s/build/the build/
Fixed.errors messages, so when profiler was not enabled in build messageTypo: s/errors/error/ Typo: s/profiler/a profiler/ Typo: s/was/is/ Typo: s/in build message/in the build, the message/
Fixed.is the following: "profiler misuse: profiler is disabled". --- src/lib_misc.c | 27 ++++++++++++++++- src/lj_errmsg.h | 1 + .../misclib-memprof-lapi-disabled.test.lua | 22 ++++++++++++++ .../misclib-sysprof-lapi-disabled.test.lua | 29 +++++++++++++++++++ 4 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua create mode 100644 test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua diff --git a/src/lib_misc.c b/src/lib_misc.c index d71904e4..7666d85f 100644 --- a/src/lib_misc.c +++ b/src/lib_misc.c @@ -315,10 +315,15 @@ static int sysprof_error(lua_State *L, int status, const char *err_details) /* local res, err, errno = sysprof.start(options) */ LJLIB_CF(misc_sysprof_start) { + const char *err_details = NULL; +#if !LJ_HASSYSPROFIt's more LuaJIT-way to use something like the following: | if (!LJ_HASSYSPROF) { | /* ... */ | } This helps to avoid strange early return.
Also, please be avaired to declare all variables (stataus, opt, err_details) in the beginning of the block.
It is a requirement for the code that strictly follows c89, when
you must declare all of your variables at the beginning of a scope
block.
AFAIK, we have no such requirement, and also there is no option
"-std=c89" in CI and no any mentions in the contribution guide.
Here and below.
Fixed.+ err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED); + return sysprof_error(L, PROFILE_ERRUSE, err_details); +#endif /* !LJ_HASSYSPROF */ + int status = PROFILE_SUCCESS; struct luam_Sysprof_Options opt = {}; - const char *err_details = NULL; status = parse_sysprof_opts(L, &opt, &err_details); if (LJ_UNLIKELY(status != PROFILE_SUCCESS)) @@ -336,6 +341,11 @@ LJLIB_CF(misc_sysprof_start) /* local res, err, errno = profile.sysprof_stop() */ LJLIB_CF(misc_sysprof_stop) { +#if !LJ_HASSYSPROF + const char *err_details = NULL; + err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED);Looks like we just can do the following for this block: | const char *err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED); Here and below.
+ return sysprof_error(L, PROFILE_ERRUSE, err_details); +#endif /* !LJ_HASSYSPROF */ int status = luaM_sysprof_stop(L); if (LJ_UNLIKELY(status != PROFILE_SUCCESS)) return sysprof_error(L, status, NULL); @@ -347,6 +357,11 @@ LJLIB_CF(misc_sysprof_stop) /* local counters, err, errno = sysprof.report() */ LJLIB_CF(misc_sysprof_report) { +#if !LJ_HASSYSPROF + const char *err_details = NULL; + err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED); + return sysprof_error(L, PROFILE_ERRUSE, err_details); +#endif /* !LJ_HASSYSPROF */ struct luam_Sysprof_Counters counters = {}; GCtab *data_tab = NULL; GCtab *count_tab = NULL; @@ -386,6 +401,11 @@ LJLIB_CF(misc_sysprof_report) /* local started, err, errno = misc.memprof.start(fname) */ LJLIB_CF(misc_memprof_start) { +#if !LJ_HASMEMPROF + const char *err_details = NULL; + err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED); + return sysprof_error(L, PROFILE_ERRUSE, err_details); +#endif /* !LJ_HASMEMPROF */ struct lj_memprof_options opt = {0}; const char *fname = strdata(lj_lib_checkstr(L, 1)); struct profile_ctx *ctx; @@ -440,6 +460,11 @@ LJLIB_CF(misc_memprof_start) /* local stopped, err, errno = misc.memprof.stop() */ LJLIB_CF(misc_memprof_stop) { +#if !LJ_HASMEMPROF + const char *err_details = NULL; + err_details = err2msg(LJ_ERR_PROF_DETAILS_DISABLED); + return sysprof_error(L, PROFILE_ERRUSE, err_details); +#endif /* !LJ_HASMEMPROF */ int status = lj_memprof_stop(L); if (status != PROFILE_SUCCESS) { switch (status) { diff --git a/src/lj_errmsg.h b/src/lj_errmsg.h index b5c3a275..e26f5e38 100644 --- a/src/lj_errmsg.h +++ b/src/lj_errmsg.h @@ -193,6 +193,7 @@ ERRDEF(PROF_DETAILS_BADINTERVAL, "profiler interval must be greater than 1") ERRDEF(PROF_DETAILS_BADPATH, "profiler path does not exist") ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters") +ERRDEF(PROF_DETAILS_DISABLED, "profiler is disabled")I suppose this should be declared under `#else` directive (i.e. if we have at least one profiler disabled).
err message is required by code for checking disabled profilers
(see above)
#undef ERRDEF /* Detecting unused error messages: diff --git a/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua new file mode 100644 index 00000000..a0669cc6 --- /dev/null +++ b/test/tarantool-tests/profilers/misclib-memprof-lapi-disabled.test.lua @@ -0,0 +1,22 @@ +local tap = require("tap")Please use single quotes everywhere across this file for the consistency.
Fixed.
Fixed.+local test = tap.test("misclib-memprof-lapi-disabled"):skipcond({ + ["Memprof is enabled"] = not os.getenv('LUAJIT_DISABLE_MEMPROF'), +}) + +test:plan(6) + +-- Attempt to start memprof when memprof is disabled.Minor: s/start memprof when memprof/start memprof when it/
Fixed.+local res, err, errno = misc.memprof.start() +test:is(res, nil, "result status on start when memprof is disabled") +test:ok(err:match("profiler is disabled"), + "error on start when memprof is disabled") +test:ok(type(errno) == "number", "errno on start when memprof is disabled") + +-- Attempt to stop memprof when memprof is disabled.Minor: s/stop memprof when memprof/stop memprof when it/
Fixed.+res, err, errno = misc.memprof.stop() +test:is(res, nil, "result status on stop when memprof is disabled") +test:ok(err:match("profiler is disabled"), + "error on stop when memprof is disabled") +test:ok(type(errno) == "number", "errno on start when memprof is disabled") + +test:done(true) diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua new file mode 100644 index 00000000..79707434 --- /dev/null +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi-disabled.test.lua @@ -0,0 +1,29 @@ +local tap = require("tap")Please use single quotes everywhere across this file for the consistency.
+local test = tap.test("misclib-sysprof-lapi-disabled"):skipcond({ + ["Sysprof is enabled"] = not os.getenv('LUAJIT_DISABLE_SYSPROF'), +}) + +test:plan(9) + +-- Attempt to start sysprof when sysprof is disabled. +local res, err, errno = misc.sysprof.start() +test:is(res, nil, "result status on start when sysprof is disabled") +test:ok(err:match("profiler is disabled"), + "error on start when sysprof is disabled") +test:ok(type(errno) == "number", "errno on start when sysprof is disabled") + +-- Attempt to stop sysprof when sysprof is disabled. +res, err, errno = misc.sysprof.stop() +test:is(res, nil, "result status on stop when sysprof is disabled") +test:ok(err:match("profiler is disabled"), + "error on stop when sysprof is disabled") +test:ok(type(errno) == "number", "errno on start when sysprof is disabled") + +-- Attempt to report when sysprof is disabled. +res, err, errno = misc.sysprof.report() +test:is(res, nil, "result status on report when sysprof is disabled") +test:ok(err:match("profiler is disabled"), + "error on stop when sysprof is disabled") +test:ok(type(errno) == "number", "errno on start when sysprof is disabled") + +test:done(true) -- 2.34.1