Re: [Tarantool-patches] [PATCH luajit 2/3] misc: use prof_error for handling errors

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

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

Hi, Sergey,

see my comments

On 05.03.2025 17:49, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except 1 minor comment and 1 reminder below.
>
> On 25.02.25, Sergey Bronnikov wrote:
>> The patch consolidates handling profilers errors into a single
>> place - in a function `prof_error()` and handles PROFILE_ERRIO,
>> generated in a function `misc_memprof_start`, in a `prof_error()`.
>> ---
>>   src/lib_misc.c | 49 +++++++++----------------------------------------
>>   1 file changed, 9 insertions(+), 40 deletions(-)
>>
>> diff --git a/src/lib_misc.c b/src/lib_misc.c
>> index b4b58509..c4b40996 100644
>> --- a/src/lib_misc.c
>> +++ b/src/lib_misc.c
>> @@ -286,14 +286,14 @@ static int prof_error(lua_State *L, int status, const char *err_details)
>>           lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));
>>         lua_pushinteger(L, EINVAL);
>>         return 3;
>> -#if LJ_HASSYSPROF
>> +#if LJ_HASSYSPROF || LJ_HASMEMPROF
>>       case PROFILE_ERRRUN:
>>         lua_pushnil(L);
>>         lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING));
>>         lua_pushinteger(L, EINVAL);
>>         return 3;
>>       case PROFILE_ERRIO:
>> -      return luaL_fileresult(L, 0, NULL);
>> +      return luaL_fileresult(L, 0, err_details);
> Discussed offline that this should be done in the previous patch-set.
The hunk is gone after rebase.
>
>>   #endif
> <snipped>
>
>> +  if (LJ_UNLIKELY(status == PROFILE_ERRRUN)) {
>>         lua_pushnil(L);
>>         lua_pushstring(L, err2msg(LJ_ERR_PROF_NOTRUNNING));
>>         lua_pushinteger(L, EINVAL);
>>         return 3;
>> -    case PROFILE_ERRIO:
>> -      return luaL_fileresult(L, 0, NULL);
>> -#endif
>> -    default:
>> -      lj_assertL(0, "bad memprof error %d", status);
>> -      return 0;
>> -    }
>>     }
>> +  if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
> Minor: Looks like more natural to use `else if` here?
> Feel free to ignore.

Fixed:

--- a/src/lib_misc.c
+++ b/src/lib_misc.c
@@ -443,9 +443,9 @@ LJLIB_CF(misc_memprof_stop)
        lua_pushstring(L, err2msg(LJ_ERR_PROF_NOTRUNNING));
        lua_pushinteger(L, EINVAL);
        return 3;
-  }
-  if (LJ_UNLIKELY(status != PROFILE_SUCCESS))
+  } else if (LJ_UNLIKELY(status != PROFILE_SUCCESS)) {
      return prof_error(L, status, NULL);
+  }

    lua_pushboolean(L, 1);
    return 1;

>> +    return prof_error(L, status, NULL);
>> +
>>     lua_pushboolean(L, 1);
>>     return 1;
>>   #endif /* !LJ_HASMEMPROF */
>> -- 
>> 2.43.0
>>

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