Hi, Sergey, also added err_details to processing of PROFILE_ERRIO as discussed: diff --git a/src/lib_misc.c b/src/lib_misc.c index 79cfaf7b..83669268 100644 --- a/src/lib_misc.c +++ b/src/lib_misc.c @@ -267,6 +267,7 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt,      status = set_output_path(path, opt);      if (status != PROFILE_SUCCESS) { +      *err_details = path;        lj_mem_free(ctx->g, ctx, sizeof(*ctx));        return status;      } @@ -293,7 +294,7 @@ static int sysprof_error(lua_State *L, int status, const char *err_details)        lua_pushinteger(L, EINVAL);        return 3;      case PROFILE_ERRIO: -      return luaL_fileresult(L, 0, NULL); +      return luaL_fileresult(L, 0, err_details);  #endif      default:        lj_assertL(0, "bad sysprof error %d", status); diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua index 32ce75bc..9fa779a7 100644 --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua @@ -128,8 +128,9 @@ test:ok(type(errno) == "number", "errno with not running")  -- Bad path.  res, err, errno = misc.sysprof.start({ mode = "C", path = BAD_PATH }) -test:ok(res == nil and err:match("No such file or directory"), -        "result status and error with bad path") +test:ok(res == nil, "result status with bad path") +local error_msg = ("%s: No such file or directory"):format(BAD_PATH) +test:ok(err == error_msg, "error with bad path") test:ok(type(errno) == "number", "errno with bad path")  -- Path is not a string. On 05.03.2025 10:55, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the fixes! > LGTM, after fixing 3 minor nits below. > > On 24.02.25, Sergey Bronnikov wrote: >> 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: > > >>>> --- >>>> 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). > There is no corresponding update on the branch. > > > >>>> -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) { > Thanks! > Please apply the following patch. It fixes all patchset-related places. > > =================================================================== > diff --git a/src/lib_misc.c b/src/lib_misc.c > index 94ec6564..79cfaf7b 100644 > --- a/src/lib_misc.c > +++ b/src/lib_misc.c > @@ -199,13 +199,13 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, > cTValue *mode_opt = lj_tab_getstr(options, lj_str_newlit(L, "mode")); > if (mode_opt) { > if (!tvisstr(mode_opt)) { > - *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE); > - return PROFILE_ERRUSE; > + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE); > + return PROFILE_ERRUSE; > } > mode = strVdata(mode_opt); > if (strlen(mode) == 0 || mode[1] != '\0') { > - *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE); > - return PROFILE_ERRUSE; > + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE); > + return PROFILE_ERRUSE; > } > } > > @@ -223,8 +223,8 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, > opt->mode = LUAM_SYSPROF_CALLGRAPH; > break; > default: > - *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE); > - return PROFILE_ERRUSE; > + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADMODE); > + return PROFILE_ERRUSE; > } > } > > @@ -235,8 +235,8 @@ static int parse_sysprof_opts(lua_State *L, struct luam_Sysprof_Options *opt, > if (interval && tvisnumber(interval)) { > int32_t signed_interval = numberVint(interval); > if (signed_interval < 1) { > - *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADINTERVAL); > - return PROFILE_ERRUSE; > + *err_details = err2msg(LJ_ERR_PROF_DETAILS_BADINTERVAL); > + return PROFILE_ERRUSE; > } > opt->interval = signed_interval; > } > @@ -281,9 +281,9 @@ static int sysprof_error(lua_State *L, int status, const char *err_details) > case PROFILE_ERRUSE: > lua_pushnil(L); > if (err_details) > - lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details); > + lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details); > else > - lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE)); > + lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE)); > lua_pushinteger(L, EINVAL); > return 3; > #if LJ_HASSYSPROF > =================================================================== > >>    int n = (int)(L->top - L->base); > > >>>> +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? > This is kind of misleading, plus this is a dead check. It is __always__ > `nil`, or we will get an error in the first test and the second test. > Since we are not testing `pcall()` implementation here, let's just drop > it (it is even cheaper :)). > > >