Hi, Sergey, changes applied and force-pushed. Sergey On 24.02.2025 15:46, Sergey Kaplun via Tarantool-patches wrote: > Hi, Sergey! > Thanks for the fixes! > LGTM, after the final polishing, please consider my comments below. > > On 20.02.25, Sergey Bronnikov wrote: >> sysprof has a number of options and with any incorrect option it >> returns `false` and error message "profiler misuse". This message >> discourages sysprof users and makes using sysprof more >> complicated. The patch sets the default profiling mode ("D", that >> shows only virtual machine state counters) if it was not passed >> and adds details to the error message with possible reasons of >> misuse. > Please add the following notes. > > | Also, it fixes the bug when the error isn't raised if the given argument > | type is incorrect. > | Also, the profiler can start if it has extra arguments given. > | They are just ignored now. Added. > >> --- >> src/lib_misc.c | 77 ++++++++++++------- >> src/lj_errmsg.h | 5 ++ >> .../profilers/misclib-sysprof-lapi.test.lua | 68 ++++++++++++++-- >> 3 files changed, 114 insertions(+), 36 deletions(-) >> >> diff --git a/src/lib_misc.c b/src/lib_misc.c >> index 5b7a4b62..1fd06dd1 100644 >> --- a/src/lib_misc.c >> +++ b/src/lib_misc.c >> @@ -14,6 +14,7 @@ >> >> #include "lj_obj.h" >> #include "lj_str.h" >> +#include "lj_strfmt.h" > Please add the corresponding include (after lj_str.h) in to > avoid conflicts when we use this file (for crossbuilds or whatever). > >> #include "lj_tab.h" >> #include "lj_lib.h" >> #include "lj_gc.h" @@ -163,6 +164,7 @@ static int on_stop_cb_default(void *opt, >> uint8_t *buf) /* The default profiling interval equals to 10 ms. */ >> #define SYSPROF_DEFAULT_INTERVAL 10 +#define SYSPROF_DEFAULT_MODE "D" >> #define SYSPROF_DEFAULT_OUTPUT "sysprof.bin" >> >> static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) { >> @@ -177,21 +179,36 @@ static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) { >> return PROFILE_SUCCESS; >> } >> >> -static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, int idx) { >> - GCtab *options = lj_lib_checktab(L, idx); >> +static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, >> + const char **err_details) { > Nit: Please, replace every 8 spaces with one tab. Fixed: --- a/src/lib_misc.c +++ b/src/lib_misc.c @@ -180,7 +180,7 @@ static int set_output_path(const char *path, struct luam_Sysprof_Options *opt) {  }  static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, -                             const char **err_details) { +                             const char **err_details) {    int n = (int)(L->top - L->base);    if (n == 0) {      opt->mode = LUAM_SYSPROF_DEFAULT; > >> + int n = (int)(L->top - L->base); >> + if (n == 0) { >> + opt->mode = LUAM_SYSPROF_DEFAULT; >> + opt->interval = SYSPROF_DEFAULT_INTERVAL; >> + return PROFILE_SUCCESS; >> + } >> + >> + GCtab *options = lj_lib_checktab(L, 1); > Please, declare (not initialize) `options` before `if` branch to avoid > -Wdeclaration-after-statement warnings. Fixed:    int n = (int)(L->top - L->base); +  GCtab *options;    if (n == 0) {      opt->mode = LUAM_SYSPROF_DEFAULT;      opt->interval = SYSPROF_DEFAULT_INTERVAL;      return PROFILE_SUCCESS;    } -  GCtab *options = lj_lib_checktab(L, 1); +  options = lj_lib_checktab(L, 1);    /* Get profiling mode. */    { > > Please, also add the comment that we ignore all other arguments given to > this function. Added:    if (n == 0) {      opt->mode = LUAM_SYSPROF_DEFAULT;      opt->interval = SYSPROF_DEFAULT_INTERVAL;      return PROFILE_SUCCESS;    } -  GCtab *options = lj_lib_checktab(L, 1); +  /* All other arguments given to this function are ignored. */ +  options = lj_lib_checktab(L, 1);    /* Get profiling mode. */    { > >> >> /* Get profiling mode. */ >> { > > >> } >> @@ -214,8 +232,10 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in >> opt->interval = SYSPROF_DEFAULT_INTERVAL; >> if (interval && tvisnumber(interval)) { >> int32_t signed_interval = numberVint(interval); >> - if (signed_interval < 1) >> + if (signed_interval < 1) { >> + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADINTERVAL); >> return PROFILE_ERRUSE; >> + } >> opt->interval = signed_interval; >> } >> } >> @@ -230,9 +250,10 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in >> cTValue *pathtv = lj_tab_getstr(options, lj_str_newlit(L, "path")); >> if (!pathtv) > Please, add the `{` braces here too. Added: @@ -248,13 +250,14 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt,      int status = 0;      cTValue *pathtv = lj_tab_getstr(options, lj_str_newlit(L, "path")); -    if (!pathtv) +    if (!pathtv) {        path = SYSPROF_DEFAULT_OUTPUT; -    else if (!tvisstr(pathtv)) { +    } else if (!tvisstr(pathtv)) {        *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADPATH);        return PROFILE_ERRUSE; -    } else +    } else {        path = strVdata(pathtv); +    }      ctx = lj_mem_new(L, sizeof(*ctx));      ctx->g = G(L); > >> path = SYSPROF_DEFAULT_OUTPUT; >> - else if (!tvisstr(pathtv)) >> + else if (!tvisstr(pathtv)) { >> + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADPATH); >> return PROFILE_ERRUSE; >> - else >> + } else > Ditto. @@ -248,13 +250,14 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt,      int status = 0;      cTValue *pathtv = lj_tab_getstr(options, lj_str_newlit(L, "path")); -    if (!pathtv) +    if (!pathtv) {        path = SYSPROF_DEFAULT_OUTPUT; -    else if (!tvisstr(pathtv)) { +    } else if (!tvisstr(pathtv)) {        *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADPATH);        return PROFILE_ERRUSE; -    } else +    } else {        path = strVdata(pathtv); +    }      ctx = lj_mem_new(L, sizeof(*ctx));      ctx->g = G(L); > >> path = strVdata(pathtv); >> >> ctx = lj_mem_new(L, sizeof(*ctx)); >> @@ -251,29 +272,26 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, in >> return PROFILE_SUCCESS; >> } > > >> +static int sysprof_error(lua_State *L, int status, const char *err_details) >> { >> switch (status) { >> case PROFILE_ERRUSE: >> lua_pushnil(L); >> - lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE)); >> + if (err_details) { > Nit: OTOH, there is no need for them here and below. > Feel free to ignore. Removed: @@ -277,21 +280,16 @@ static int sysprof_error(lua_State *L, int status, const char *err_details)    switch (status) {      case PROFILE_ERRUSE:        lua_pushnil(L); -      if (err_details) { +      if (err_details)          lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details); -      } else { +      else          lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE)); -      }        lua_pushinteger(L, EINVAL);        return 3; >> + lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details); >> + } else { >> + lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE)); >> + } >> lua_pushinteger(L, EINVAL); >> return 3; >> #if LJ_HASSYSPROF >> case PROFILE_ERRRUN: >> lua_pushnil(L); >> - lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING)); >> + if (err_details) { > Side note: `err_deails` is always `NULL` here. I suppose we don't need > this branch for now (to avoid the dead code), so these changes may be > avoided. Removed:  #if LJ_HASSYSPROF      case PROFILE_ERRRUN:        lua_pushnil(L); -      if (err_details) { -        lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details); -      } else { -        lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING)); -      } +      lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING));        lua_pushinteger(L, EINVAL);        return 3;      case PROFILE_ERRIO: >> + lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details); > Typo: s/LJ_ERR_PROF_MISUSE/LJ_ERR_PROF_ISRUNNING/ > Irrelevant, after fixing the comment above. Yep, irrelevant now. > >> + } else { >> + lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING)); >> + } >> lua_pushinteger(L, EINVAL); >> return 3; >> case PROFILE_ERRIO: >> @@ -291,15 +309,16 @@ LJLIB_CF(misc_sysprof_start) > > >> diff --git a/src/lj_errmsg.h b/src/lj_errmsg.h >> index 19c41f0b..b5c3a275 100644 >> --- a/src/lj_errmsg.h >> +++ b/src/lj_errmsg.h >> @@ -188,6 +188,11 @@ ERRDEF(PROF_ISRUNNING, "profiler is running already") >> ERRDEF(PROF_NOTRUNNING, "profiler is not running") >> #endif >> >> +ERRDEF(PROF_DETAILS_BADMODE, "profiler mode must be 'D', 'L' or 'C'") >> +ERRDEF(PROF_DETAILS_BADINTERVAL, "profiler interval must be greater than 1") >> +ERRDEF(PROF_DETAILS_BADPATH, "profiler path does not exist") > This should be something like the following: > | "profiler path should be a string" Fixed: --- a/src/lj_errmsg.h +++ b/src/lj_errmsg.h @@ -190,8 +190,7 @@ ERRDEF(PROF_NOTRUNNING,     "profiler is not running")  ERRDEF(PROF_DETAILS_BADMODE, "profiler mode must be 'D', 'L' or 'C'")  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_BADPATH, "profiler path should be a string")  #undef ERRDEF > >> +ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters") > This errmsg is unused now and can be deleted. Thanks, fixed: --- a/src/lj_errmsg.h +++ b/src/lj_errmsg.h @@ -190,8 +190,7 @@ ERRDEF(PROF_NOTRUNNING,     "profiler is not running")  ERRDEF(PROF_DETAILS_BADMODE, "profiler mode must be 'D', 'L' or 'C'")  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_BADPATH, "profiler path should be a string")  #undef ERRDEF @@ -127,6 +127,13 @@ test:ok(res == nil and err:match("No such file or directory"),          "result status and error with bad path") test:ok(type(errno) == "number", "errno with bad path") +-- Path is not a string. +res, err, errno = misc.sysprof.start({ mode = 'C', path = 190 }) +test:ok(res == nil, "result status with path is not a string") +test:ok(err:match("profiler path should be a string"), +        "err with path is not a string") +test:ok(type(errno) == "number", "errno with path is not a string") +  -- Bad interval.  res, err, errno = misc.sysprof.start{ mode = "C", interval = BAD_INTERVAL } test:is(res, nil, "result status and error with bad interval") > >> + >> #undef ERRDEF >> >> /* Detecting unused error messages: >> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua >> index 32fa384c..9915f565 100644 >> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua >> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua > > >> @@ -24,6 +24,8 @@ local profilename = require("utils").tools.profilename >> >> local TMP_BINFILE = profilename("sysprofdata.tmp.bin") >> local BAD_PATH = profilename("sysprofdata/tmp.bin") >> +local BAD_MODE = "A" >> +local BAD_INTERVAL = -1 > Why do we need this definition for only one type of bad interval? > Let's consistently declare both of them like: > | local BAD_INTERVAL_M1 = -1 > | local BAD_INTERVAL_0 = 0 > Or don't declare them at all. > > I'm OK with both variants. BAD_INTERVAL is used in two testcases therefore it is declared as constant. >> >> local function payload() >> local function fib(n) >> @@ -64,11 +66,44 @@ end > > >> test:ok(type(errno) == "number", "errno with wrong profiling mode") >> >> +-- Missed profiling mode. >> +res, err, errno = misc.sysprof.start{} >> +test:is(res, true, "res with missed profiling mode") >> +test:is(err, nil, "no error with missed profiling mode") >> +test:is(errno, nil, "no errno with missed profiling mode") >> +assert(misc.sysprof.stop()) > Minor: It would be nice to check that there is no default file created, > i.e., that we actually run the default profiler mode without stack > inspection. Feel free to ignore, through. Added: @@ -77,6 +79,9 @@ res, err, errno = misc.sysprof.start{} test:is(res, true, "res with missed profiling mode") test:is(err, nil, "no error with missed profiling mode") test:is(errno, nil, "no errno with missed profiling mode") +local ok, err_msg = pcall(read_file, SYSPROF_DEFAULT_OUTPUT_FILE) +test:ok(ok == false and err_msg:match("cannot open a file"), +        "default output file is empty")  assert(misc.sysprof.stop())  -- Not a table. >> + >> +-- Not a table. >> +res, err, errno = pcall(misc.sysprof.start, "NOT A TABLE") >> +print(res, err, errno) > Excess debug print. Removed, thanks! > >> +test:is(res, false, "res with not a table") >> +test:ok(err:match("table expected, got string"), >> + "error with not a table") >> +test:is(errno, nil, "errno with not a table") > There is no need to check or declare the `errno` variable. It's always > `nil` for the case when the error is raised. The check is cheap, why not? > >> + >> +-- All parameters are invalid. >> +res, err, errno = misc.sysprof.start({ >> + mode = BAD_MODE, path = BAD_PATH, interval = BAD_INTERVAL }) >> +test:ok(res == nil, "res with all invalid parameters") >> +test:ok(err:match("profiler misuse: profiler mode must be 'D', 'L' or 'C'"), >> + "error with all invalid parameters") >> +test:ok(type(errno) == "number", "errno with all invalid parameters") >> + >> +-- All parameters are invalid, except the first one. >> +res, err, errno = misc.sysprof.start({ >> + mode = "C", path = BAD_PATH, interval = BAD_INTERVAL }) >> +test:ok(res == nil, "res with all invalid parameters except the first one") >> +test:ok(err:match("profiler misuse: profiler interval must be greater than 1"), >> + "error with all invalid parameters except the first one") >> +test:ok(type(errno) == "number", >> + "errno with all invalid parameters except the first one") > This is not exactly what I mean by invalid/partially invalid parameters > (sorry, for the bad wording): > I mean to add tests that cover the following calls: > | pcall(misc.sysprof.start, '', '', '') -- shouldn't started -- error > | -- (expected table got nil) > | misc.sysprof.start({}, '', '') -- should started with the default options > Verbally decided to left these testcases with a comment: test:is(res, false, "res with not a table") test:ok(err:match("table expected, got string"),          "error with not a table") test:is(errno, nil, "errno with not a table") --- All parameters are invalid. +-- All parameters are invalid. Check parameter validation order +-- (not strict documented behaviour).  res, err, errno = misc.sysprof.start({    mode = BAD_MODE, path = BAD_PATH, interval = BAD_INTERVAL }) test:ok(res == nil, "res with all invalid parameters") @@ -95,7 +100,8 @@ test:ok(err:match("profiler misuse: profiler mode must be 'D', 'L' or 'C'"),          "error with all invalid parameters") test:ok(type(errno) == "number", "errno with all invalid parameters") --- All parameters are invalid, except the first one. +-- All parameters are invalid, except the first one. Check +-- parameter validation order (not strict documented behaviour).  res, err, errno = misc.sysprof.start({    mode = "C", path = BAD_PATH, interval = BAD_INTERVAL }) test:ok(res == nil, "res with all invalid parameters except the first one") >> + >> -- Already running. >> res, err = misc.sysprof.start{ mode = "D" } >> assert(res, err) >> @@ -93,11 +128,30 @@test:ok(res == nil anderr:match("No such file or directory"), >> test:ok(type(errno) == "number", "errno with bad path") >> >> -- Bad interval. >> -res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 } >> -test:ok(res == nil anderr:match("profiler misuse"), >> - "result status and error with bad interval") >> +res, err, errno = misc.sysprof.start{ mode = "C", interval = BAD_INTERVAL } > Side note: See my comments about this constant above. decided to left as is > >> +test:is(res, nil, "result status and error with bad interval") >> +test:ok(err:match("profiler interval must be greater than 1"), >> + "error with bad interval") >> test:ok(type(errno) == "number", "errno with bad interval") >> >> +-- Bad interval (0). >> +res, err, errno = misc.sysprof.start{ mode = "C", interval = 0 } >> +test:ok(res == nil, "res with bad interval 0") >> +test:ok(err:match("profiler interval must be greater than 1"), >> + "error with bad interval 0") >> +test:ok(type(errno) == "number", "errno with bad interval 0") >> + >> +-- Good interval (1). >> +res, err, errno = misc.sysprof.start{ >> + mode = "C", >> + interval = 1, >> + path = "/dev/null", >> +} >> +test:is(res, true, "res with good interval 1") >> +test:is(err, nil, "no error with good interval 1") >> +test:is(errno, nil, "no errno with good interval 1") >> +misc.sysprof.stop() >> + >> -- DEFAULT MODE >> >> if not pcall(generate_output, { mode = "D", interval = 100 }) then >> -- >> 2.43.0 >>