From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <tarantool-patches-bounces@dev.tarantool.org>
Received: from [87.239.111.99] (localhost [127.0.0.1])
by dev.tarantool.org (Postfix) with ESMTP id 6E29DF45B27;
Mon, 24 Feb 2025 21:05:11 +0300 (MSK)
DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6E29DF45B27
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev;
t=1740420311; bh=iQLdrS1xb2DZQuFQBU2OSskUVUrbVLNgovgfR6vsaCg=;
h=Date:To:Cc:References:In-Reply-To:Subject:List-Id:
List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe:
From:Reply-To:From;
b=GxLAdQSIH/Hjh2H/WQdo25/LhJb1kq6JLlf0wy16tLZotAb3JYuWXpWVu2/4MLAtd
I5pSciiB8SOFbJ1JE9YyXfTYWUXaDK7lDOrY+vvMgxMgeP8obqnaAD8ppGkdrpOND3
qBOsfxmBYJxRTw6cg/EXRMBEZhfQZO0gurtsfcSI=
Received: from send37.i.mail.ru (send37.i.mail.ru [89.221.237.132])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
(No client certificate requested)
by dev.tarantool.org (Postfix) with ESMTPS id 177F2F45B01
for <tarantool-patches@dev.tarantool.org>;
Mon, 24 Feb 2025 21:05:10 +0300 (MSK)
DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 177F2F45B01
Received: by exim-smtp-75f5fcb77d-b85gh with esmtpa (envelope-from
<sergeyb@tarantool.org>)
id 1tmcpI-00000000N2a-39vK; Mon, 24 Feb 2025 21:05:09 +0300
Content-Type: multipart/alternative;
boundary="------------VgAxBR0vfvy2m68pkqAOWZI5"
Message-ID: <52e2b52f-33a8-4f06-bcd8-ec8aa9fc457a@tarantool.org>
Date: Mon, 24 Feb 2025 21:05:08 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Content-Language: en-US
To: Sergey Kaplun <skaplun@tarantool.org>, Sergey Bronnikov <estetus@gmail.com>
Cc: tarantool-patches@dev.tarantool.org
References: <cover.1740050074.git.sergeyb@tarantool.org>
<881980705ead69f58c0474d5f49c00603e4c6e72.1740050074.git.sergeyb@tarantool.org>
<Z7xqDHtLuS0NHxCe@root>
In-Reply-To: <Z7xqDHtLuS0NHxCe@root>
X-Mailru-Src: smtp
X-4EC0790: 10
X-7564579A: B8F34718100C35BD
X-77F55803: 4F1203BC0FB41BD957BCB5CA1E0F722CCB0EA13E3528152EFFE0F164204A5E85182A05F53808504087A676D772259FB63DE06ABAFEAF6705E9B6B3441B9617042308A7AB66F554E58FA4FAB0904F4167
X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7AD2F2D6F6013FF7FC2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE7A25B432396ED6D32EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BC08E230531AC9C90299B4624F8345C5A08BF37BF1065DC672951A61E646B4F84A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE71AE4D56B06699BBC9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C37EF884183F8E4D67117882F4460429728AD0CFFFB425014E868A13BD56FB6657D81D268191BDAD3DC09775C1D3CA48CF85B415260A3EE7FDBA3038C0950A5D36C8A9BA7A39EFB766D91E3A1F190DE8FDBA3038C0950A5D36D5E8D9A59859A8B64B198F4260265E7476E601842F6C81A1F004C906525384303E02D724532EE2C3F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7CCC4B623DB76FBBCBC4224003CC83647689D4C264860C145E
X-C1DE0DAB: 0D63561A33F958A562D155B33EB9BDC55002B1117B3ED6968EF9250AFEFE6CE6AD0703CEB2EF9A27823CB91A9FED034534781492E4B8EEAD30F6261E3B51B4EABDAD6C7F3747799A
X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CFF84EADC413B22C6A2465F7C4D27ED32603A2A825541139E31773EE2135B6410C9FECE8BBF8D52592DD1EB5C6CC1B9E94F6AD87FDBA7D37A1F240A34C9ECE165D970397AA09AB12405F4332CA8FE04980913E6812662D5F2AB9AF64DB4688768036DF5FE9C0001AF333F2C28C22F508233FCF178C6DD14203
X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu53w8ahmwBjZKM/YPHZyZHvz5uv+WouB9+ObcCpyrx6l7KImUglyhkEat/+ysWwi0gdhEs0JGjl6ggRWTy1haxBpVdbIX1nthFXMZebaIdHP2ghjoIc/363UZI6Kf1ptIMVQQG/FugD0/CWbj44bgovrs=
X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F2588458257DD1E88AE0614081508210FEB0A3EAE82C97E641A271EFC1CF48893EAF9E6F645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196
X-Mras: Ok
Subject: Re: [Tarantool-patches] [PATCH luajit 4/8][v3] sysprof: introduce
specific errors and default mode
X-BeenThere: tarantool-patches@dev.tarantool.org
X-Mailman-Version: 2.1.34
Precedence: list
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>,
<mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>,
<mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
From: Sergey Bronnikov via Tarantool-patches
<tarantool-patches@dev.tarantool.org>
Reply-To: Sergey Bronnikov <sergeyb@tarantool.org>
Errors-To: tarantool-patches-bounces@dev.tarantool.org
Sender: "Tarantool-patches" <tarantool-patches-bounces@dev.tarantool.org>
This is a multi-part message in MIME format.
--------------VgAxBR0vfvy2m68pkqAOWZI5
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
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 <src/Makefile.dep.original> 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. */
>> {
> <snipped>
>
>> }
>> @@ -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;
>> }
> <snipped>
>
>> +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)
> <snipped>
>
>> 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
> <snipped>
>
>> @@ -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
> <snipped>
>
>> 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
>>
--------------VgAxBR0vfvy2m68pkqAOWZI5
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit
<!DOCTYPE html>
<html data-lt-installed="true">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body style="padding-bottom: 1px;">
<p>Hi, Sergey,</p>
<p>changes applied and force-pushed.</p>
<p>Sergey<br>
</p>
<div class="moz-cite-prefix">On 24.02.2025 15:46, Sergey Kaplun via
Tarantool-patches wrote:<br>
</div>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">Hi, Sergey!
Thanks for the fixes!
LGTM, after the final polishing, please consider my comments below.
On 20.02.25, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">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.
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.</pre>
</blockquote>
Added.<br>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">---
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"
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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).
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> #include "lj_tab.h"
#include "lj_lib.h"
#include "lj_gc.h<a class="moz-txt-link-rfc2396E" href="mailto:@@-163,6+164,7@@staticinton_stop_cb_default(void*opt,uint8_t*buf)/*Thedefaultprofilingintervalequalsto10ms.*/#defineSYSPROF_DEFAULT_INTERVAL10+#defineSYSPROF_DEFAULT_MODE">"
@@ -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 "</a>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) {
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Nit: Please, replace every 8 spaces with one tab.</pre>
</blockquote>
<p>Fixed:</p>
<p>--- a/src/lib_misc.c<br>
+++ b/src/lib_misc.c<br>
@@ -180,7 +180,7 @@ static int set_output_path(const char *path,
struct luam_Sysprof_Options *opt) {<br>
}<br>
<br>
static int parse_sysprof_opts(lua_State *L, struct
luam_Sysprof_Options *opt,<br>
- const char **err_details) {<br>
+ const char **err_details) {<br>
int n = (int)(L->top - L->base);<br>
if (n == 0) {<br>
opt->mode = LUAM_SYSPROF_DEFAULT;<br>
</p>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+ 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);
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Please, declare (not initialize) `options` before `if` branch to avoid
-Wdeclaration-after-statement warnings.</pre>
</blockquote>
<p>Fixed:</p>
<p> int n = (int)(L->top - L->base);<br>
+ GCtab *options;<br>
if (n == 0) {<br>
opt->mode = LUAM_SYSPROF_DEFAULT;<br>
opt->interval = SYSPROF_DEFAULT_INTERVAL;<br>
return PROFILE_SUCCESS;<br>
}<br>
<br>
- GCtab *options = lj_lib_checktab(L, 1);<br>
+ options = lj_lib_checktab(L, 1);<br>
<br>
/* Get profiling mode. */<br>
{<br>
</p>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">
Please, also add the comment that we ignore all other arguments given to
this function.</pre>
</blockquote>
<p>Added:</p>
<p> if (n == 0) {<br>
opt->mode = LUAM_SYSPROF_DEFAULT;<br>
opt->interval = SYSPROF_DEFAULT_INTERVAL;<br>
return PROFILE_SUCCESS;<br>
}<br>
<br>
- GCtab *options = lj_lib_checktab(L, 1);<br>
+ /* All other arguments given to this function are ignored. */<br>
+ options = lj_lib_checktab(L, 1);<br>
<br>
/* Get profiling mode. */<br>
{<br>
</p>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">
/* Get profiling mode. */
{
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> }
@@ -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)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Please, add the `{` braces here too.</pre>
</blockquote>
<p>Added:</p>
<p>@@ -248,13 +250,14 @@ static int parse_sysprof_opts(lua_State *L,
struct luam_Sysprof_Options *opt,<br>
int status = 0;<br>
<br>
cTValue *pathtv = lj_tab_getstr(options, lj_str_newlit(L,
"path"));<br>
- if (!pathtv)<br>
+ if (!pathtv) {<br>
path = SYSPROF_DEFAULT_OUTPUT;<br>
- else if (!tvisstr(pathtv)) {<br>
+ } else if (!tvisstr(pathtv)) {<br>
*err_details = err2msg(LJ_ERR_PROF_DETAILS_BADPATH);<br>
return PROFILE_ERRUSE;<br>
- } else<br>
+ } else {<br>
path = strVdata(pathtv);<br>
+ }<br>
<br>
ctx = lj_mem_new(L, sizeof(*ctx));<br>
ctx->g = G(L);<br>
</p>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> 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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Ditto.</pre>
</blockquote>
@@ -248,13 +250,14 @@ static int parse_sysprof_opts(lua_State *L,
struct luam_Sysprof_Options *opt,<br>
int status = 0;<br>
<br>
cTValue *pathtv = lj_tab_getstr(options, lj_str_newlit(L,
"path"));<br>
- if (!pathtv)<br>
+ if (!pathtv) {<br>
path = SYSPROF_DEFAULT_OUTPUT;<br>
- else if (!tvisstr(pathtv)) {<br>
+ } else if (!tvisstr(pathtv)) {<br>
*err_details = err2msg(LJ_ERR_PROF_DETAILS_BADPATH);<br>
return PROFILE_ERRUSE;<br>
- } else<br>
+ } else {<br>
path = strVdata(pathtv);<br>
+ }<br>
<br>
ctx = lj_mem_new(L, sizeof(*ctx));<br>
ctx->g = G(L);<br>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> 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;
}
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+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) {
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Nit: OTOH, there is no need for them here and below.
Feel free to ignore.
</pre>
</blockquote>
<p>Removed:</p>
<p>@@ -277,21 +280,16 @@ static int sysprof_error(lua_State *L, int
status, const char *err_details)<br>
switch (status) {<br>
case PROFILE_ERRUSE:<br>
lua_pushnil(L);<br>
- if (err_details) {<br>
+ if (err_details)<br>
lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE),
err_details);<br>
- } else {<br>
+ else<br>
lua_pushstring(L, err2msg(LJ_ERR_PROF_MISUSE));<br>
- }<br>
lua_pushinteger(L, EINVAL);<br>
return 3;<br>
</p>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+ 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) {
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.
</pre>
</blockquote>
<p>Removed:</p>
<p> #if LJ_HASSYSPROF<br>
case PROFILE_ERRRUN:<br>
lua_pushnil(L);<br>
- if (err_details) {<br>
- lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE),
err_details);<br>
- } else {<br>
- lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING));<br>
- }<br>
+ lua_pushstring(L, err2msg(LJ_ERR_PROF_ISRUNNING));<br>
lua_pushinteger(L, EINVAL);<br>
return 3;<br>
case PROFILE_ERRIO:<br>
</p>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+ lj_strfmt_pushf(L, "%s: %s", err2msg(LJ_ERR_PROF_MISUSE), err_details);
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Typo: s/LJ_ERR_PROF_MISUSE/LJ_ERR_PROF_ISRUNNING/
Irrelevant, after fixing the comment above.</pre>
</blockquote>
Yep, irrelevant now.<br>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+ } 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)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">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")
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
This should be something like the following:
| "profiler path should be a string"</pre>
</blockquote>
<p>Fixed:</p>
<p>--- a/src/lj_errmsg.h<br>
+++ b/src/lj_errmsg.h<br>
@@ -190,8 +190,7 @@ ERRDEF(PROF_NOTRUNNING, "profiler is not
running")<br>
<br>
ERRDEF(PROF_DETAILS_BADMODE, "profiler mode must be 'D', 'L' or
'C'")<br>
ERRDEF(PROF_DETAILS_BADINTERVAL, "profiler interval must be
greater than 1")<br>
-ERRDEF(PROF_DETAILS_BADPATH, "profiler path does not exist")<br>
-ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with
parameters")<br>
+ERRDEF(PROF_DETAILS_BADPATH, "profiler path should be a string")<br>
<br>
#undef ERRDEF<br>
</p>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with parameters")
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
This errmsg is unused now and can be deleted.</pre>
</blockquote>
<p>Thanks, fixed:</p>
<p>--- a/src/lj_errmsg.h<br>
+++ b/src/lj_errmsg.h<br>
@@ -190,8 +190,7 @@ ERRDEF(PROF_NOTRUNNING, "profiler is not
running")<br>
<br>
ERRDEF(PROF_DETAILS_BADMODE, "profiler mode must be 'D', 'L' or
'C'")<br>
ERRDEF(PROF_DETAILS_BADINTERVAL, "profiler interval must be
greater than 1")<br>
-ERRDEF(PROF_DETAILS_BADPATH, "profiler path does not exist")<br>
-ERRDEF(PROF_DETAILS_BADTABLE, "profiler expects a table with
parameters")<br>
+ERRDEF(PROF_DETAILS_BADPATH, "profiler path should be a string")<br>
<br>
#undef ERRDEF</p>
<p>@@ -127,6 +127,13 @@ <a class="moz-txt-link-freetext" href="test:ok(res">test:ok(res</a> == nil and <a class="moz-txt-link-freetext" href="err:match(">err:match(</a>"No such
file or directory"),<br>
"result status and error with bad path")<br>
<a class="moz-txt-link-freetext" href="test:ok(type(errno)">test:ok(type(errno)</a> == "number", "errno with bad path")<br>
<br>
+-- Path is not a string.<br>
+res, err, errno = misc.sysprof.start({ mode = 'C', path = 190 })<br>
+test:ok(res == nil, "result status with path is not a string")<br>
+test:ok(<a class="moz-txt-link-freetext" href="err:match(">err:match(</a>"profiler path should be a string"),<br>
+ "err with path is not a string")<br>
+test:ok(type(errno) == "number", "errno with path is not a
string")<br>
+<br>
-- Bad interval.<br>
res, err, errno = misc.sysprof.start{ mode = "C", interval =
BAD_INTERVAL }<br>
<a class="moz-txt-link-freetext" href="test:is(res">test:is(res</a>, nil, "result status and error with bad interval")<br>
</p>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
#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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">@@ -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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.
</pre>
</blockquote>
<br>
<pre wrap="" class="moz-quote-pre">BAD_INTERVAL is used in two testcases therefore it is declared as constant.</pre>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">
local function payload()
local function fib(n)
@@ -64,11 +66,44 @@ end
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
<snipped>
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre"> <a class="moz-txt-link-freetext" href="test:ok(type(errno)">test:ok(type(errno)</a> == "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())
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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.
</pre>
</blockquote>
<p>Added:</p>
<p>@@ -77,6 +79,9 @@ res, err, errno = misc.sysprof.start{}<br>
<a class="moz-txt-link-freetext" href="test:is(res">test:is(res</a>, true, "res with missed profiling mode")<br>
<a class="moz-txt-link-freetext" href="test:is(err">test:is(err</a>, nil, "no error with missed profiling mode")<br>
<a class="moz-txt-link-freetext" href="test:is(errno">test:is(errno</a>, nil, "no errno with missed profiling mode")<br>
+local ok, err_msg = pcall(read_file, SYSPROF_DEFAULT_OUTPUT_FILE)<br>
+test:ok(ok == false and err_<a class="moz-txt-link-freetext" href="msg:match(">msg:match(</a>"cannot open a file"),<br>
+ "default output file is empty")<br>
assert(misc.sysprof.stop())<br>
<br>
-- Not a table.<br>
</p>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+-- Not a table.
+res, err, errno = pcall(misc.sysprof.start, "NOT A TABLE")
+print(res, err, errno)
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Excess debug print.</pre>
</blockquote>
Removed, thanks!<br>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+test:is(res, false, "res with not a table")
+test:ok(<a class="moz-txt-link-freetext" href="err:match(">err:match(</a>"table expected, got string"),
+ "error with not a table")
+test:is(errno, nil, "errno with not a table")
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
There is no need to check or declare the `errno` variable. It's always
`nil` for the case when the error is raised.</pre>
</blockquote>
<p>The check is cheap, why not?</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
+-- 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(<a class="moz-txt-link-freetext" href="err:match(">err:match(</a>"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(<a class="moz-txt-link-freetext" href="err:match(">err:match(</a>"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")
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
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
</pre>
</blockquote>
<p>Verbally decided to left these testcases with a comment:</p>
<p> <a class="moz-txt-link-freetext" href="test:is(res">test:is(res</a>, false, "res with not a table")<br>
<a class="moz-txt-link-freetext" href="test:ok(err:match(">test:ok(err:match(</a>"table expected, got string"),<br>
"error with not a table")<br>
<a class="moz-txt-link-freetext" href="test:is(errno">test:is(errno</a>, nil, "errno with not a table")<br>
<br>
--- All parameters are invalid.<br>
+-- All parameters are invalid. Check parameter validation order<br>
+-- (not strict documented behaviour).<br>
res, err, errno = misc.sysprof.start({<br>
mode = BAD_MODE, path = BAD_PATH, interval = BAD_INTERVAL })<br>
<a class="moz-txt-link-freetext" href="test:ok(res">test:ok(res</a> == nil, "res with all invalid parameters")<br>
@@ -95,7 +100,8 @@ <a class="moz-txt-link-freetext" href="test:ok(err:match(">test:ok(err:match(</a>"profiler misuse: profiler
mode must be 'D', 'L' or 'C'"),<br>
"error with all invalid parameters")<br>
<a class="moz-txt-link-freetext" href="test:ok(type(errno)">test:ok(type(errno)</a> == "number", "errno with all invalid
parameters")<br>
<br>
--- All parameters are invalid, except the first one.<br>
+-- All parameters are invalid, except the first one. Check<br>
+-- parameter validation order (not strict documented behaviour).<br>
res, err, errno = misc.sysprof.start({<br>
mode = "C", path = BAD_PATH, interval = BAD_INTERVAL })<br>
<a class="moz-txt-link-freetext" href="test:ok(res">test:ok(res</a> == nil, "res with all invalid parameters except the
first one")<br>
</p>
<br>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+
-- Already running.
res, err = misc.sysprof.start{ mode = "D" }
assert(res, err)
@@ -93,11 +128,30 @@ <a class="moz-txt-link-freetext" href="test:ok(res">test:ok(res</a> == nil and <a class="moz-txt-link-freetext" href="err:match(">err:match(</a>"No such file or directory"),
<a class="moz-txt-link-freetext" href="test:ok(type(errno)">test:ok(type(errno)</a> == "number", "errno with bad path")
-- Bad interval.
-res, err, errno = misc.sysprof.start{ mode = "C", interval = -1 }
-test:ok(res == nil and <a class="moz-txt-link-freetext" href="err:match(">err:match(</a>"profiler misuse"),
- "result status and error with bad interval")
+res, err, errno = misc.sysprof.start{ mode = "C", interval = BAD_INTERVAL }
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
Side note: See my comments about this constant above.</pre>
</blockquote>
decided to left as is<br>
<blockquote type="cite" cite="mid:Z7xqDHtLuS0NHxCe@root">
<pre wrap="" class="moz-quote-pre">
</pre>
<blockquote type="cite">
<pre wrap="" class="moz-quote-pre">+test:is(res, nil, "result status and error with bad interval")
+test:ok(<a class="moz-txt-link-freetext" href="err:match(">err:match(</a>"profiler interval must be greater than 1"),
+ "error with bad interval")
<a class="moz-txt-link-freetext" href="test:ok(type(errno)">test:ok(type(errno)</a> == "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(<a class="moz-txt-link-freetext" href="err:match(">err:match(</a>"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
</pre>
</blockquote>
<pre wrap="" class="moz-quote-pre">
</pre>
</blockquote>
</body>
<lt-container></lt-container>
</html>
--------------VgAxBR0vfvy2m68pkqAOWZI5--