[Tarantool-patches] [PATCH luajit v2 6/6] profilers: print user-friendly errors

[Sergey Kaplun]

Hi, Maxim!
Thanks for the fixes!
Please consider my comments below.

On 06.12.23, Maxim Kokryashkin wrote:

<snipped>

> ---
>  Makefile.original                             |  2 +-
>  .../gh-5688-tool-cli-flag.test.lua            |  4 +-
>  ...17-profile-parsers-error-handling.test.lua | 90 +++++++++++++++++++
>  tools/CMakeLists.txt                          |  4 +
>  tools/memprof.lua                             | 11 +--
>  tools/sysprof.lua                             | 10 +--
>  tools/sysprof/parse.lua                       |  2 +-
>  tools/utils/safe_event_reader.lua             | 34 +++++++
>  tools/utils/symtab.lua                        |  2 +-
>  9 files changed, 144 insertions(+), 15 deletions(-)
>  create mode 100644 test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua
>  create mode 100644 tools/utils/safe_event_reader.lua

I suggest renaming to the <evread.lua> so it will be match <bufread.lua>.

> 
> diff --git a/Makefile.original b/Makefile.original
> index 2a56d648..1ef6fbe6 100644
> --- a/Makefile.original
> +++ b/Makefile.original

<snipped>

> diff --git a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
> index 75293f11..ec958031 100644
> --- a/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
> +++ b/test/tarantool-tests/gh-5688-tool-cli-flag.test.lua
> @@ -42,7 +42,7 @@ local SMOKE_CMD_SET = {
>  local MEMPROF_CMD_SET = {
>    {
>      cmd = MEMPROF_PARSER .. BAD_PATH,
> -    like = 'fopen, errno: 2',
> +    like = 'Failed to open',
>    },
>    {
>      cmd = MEMPROF_PARSER .. TMP_BINFILE_MEMPROF,
> @@ -61,7 +61,7 @@ local MEMPROF_CMD_SET = {
>  local SYSPROF_CMD_SET = {
>    {
>      cmd = SYSPROF_PARSER .. BAD_PATH,
> -    like = 'fopen, errno: 2',
> +    like = 'Failed to open',
>    },
>    {
>      cmd = SYSPROF_PARSER .. TMP_BINFILE_SYSPROF,

After adding the original error message, these changes appear not to be
necessary.

We can still add matching with updated error messages to the `fopen()`
error too. Like the following:

| like = 'Failed to open.*fopen, errno: 2'

> diff --git a/test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua b/test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua
> new file mode 100644
> index 00000000..c2e0f0a6
> --- /dev/null
> +++ b/test/tarantool-tests/gh-9217-profile-parsers-error-handling.test.lua

<snipped>

> diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
> index a4adc15b..7f728787 100644
> --- a/tools/CMakeLists.txt
> +++ b/tools/CMakeLists.txt

<snipped>

> diff --git a/tools/memprof.lua b/tools/memprof.lua
> index 537cb869..d491b3dc 100644
> --- a/tools/memprof.lua
> +++ b/tools/memprof.lua

<snipped>

> diff --git a/tools/sysprof.lua b/tools/sysprof.lua
> index 8449b23f..9c0c23c9 100644
> --- a/tools/sysprof.lua
> +++ b/tools/sysprof.lua

<snipped>

> diff --git a/tools/utils/safe_event_reader.lua b/tools/utils/safe_event_reader.lua
> new file mode 100644
> index 00000000..39246a9d
> --- /dev/null
> +++ b/tools/utils/safe_event_reader.lua
> @@ -0,0 +1,34 @@
> +local bufread = require('utils.bufread')
> +local symtab = require('utils.symtab')
> +
> +local function make_error_handler(fmt, inputfile)
> +  return function(err)
> +    io.stderr:write(string.format(fmt, inputfile))
> +    io.stderr:write(string.format('\nOriginal error: %s', err))

Typo: s/%s/%s\n/

> +    os.exit(1, true)
> +  end

For now the output is the following:

| 14:09:05 jobs:0 burii at root:~/reviews/luajit/refactor-profilers$ 
| >>> LUA_PATH="src/?.lua;tools/?.lua;;" src/luajit -ts /tmp/memprof.bin
| Failed to parse profile data from /tmp/memprof.bin.
| Original error: tools/sysprof/parse.lua:240: Bad LJP format prologue: ljm14:30:50 jobs:0 burii at root:~/reviews/luajit/refactor-profilers$[1] 
| >>>

I suppose the following format is more readable:

| Failed to parse profile data from /tmp/memprof.bin:
| 	tools/sysprof/parse.lua:240: Bad LJP format prologue: ljm

The ident level is one tab.

> +end
> +
> +local function safe_event_reader(parser, inputfile)
> +  local _, reader = xpcall(
> +    bufread.new,
> +    make_error_handler('Failed to open %s.', inputfile),
> +    inputfile
> +  )
> +
> +  local _, symbols = xpcall(
> +    symtab.parse,
> +    make_error_handler('Failed to parse symtab from %s.', inputfile),
> +    reader
> +  )
> +
> +  local _, events = xpcall(
> +    parser,
> +    make_error_handler('Failed to parse profile data from %s.', inputfile),
> +    reader,
> +    symbols
> +  )
> +  return events, symbols
> +end
> +
> +return safe_event_reader

Nit: If we return this function at once, do we need to name it?
Feel free to ignore.

> diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua
> index 0c878e7a..c4aefef7 100644
> --- a/tools/utils/symtab.lua
> +++ b/tools/utils/symtab.lua

<snipped>

> -- 
> 2.43.0
> 

-- 
Best regards,
Sergey Kaplun