Re: [Tarantool-patches] [PATCH luajit 4/8][v3] sysprof: introduce specific errors and default mode

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 6994 bytes --]

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:
> <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>
>

[-- Attachment #2: Type: text/html, Size: 9420 bytes --]