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.
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:<snipped>--- 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 <src/Makefile.dep.original> to avoid conflicts when we use this file (for crossbuilds or whatever).There is no corresponding update on the branch.<snipped>-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);<snipped>+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 :)). <snipped>