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

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

Max, thanks for corerections! LGTM

On 1/9/24 16:54, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the review!
> Fixed your minor, branch is force-pushed. Here is the diff:
> ===
> diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua
> index cc66a23e..11a79a1d 100644
> --- a/tools/memprof/parse.lua
> +++ b/tools/memprof/parse.lua
> @@ -206,12 +206,13 @@ function M.parse(reader, symbols)
>     local _ = reader:read_octets(3)
>
>     if magic ~= LJM_MAGIC then
> -    error("Bad LJM format prologue: "..magic)
> +    error("Bad memprof event format prologue: "..magic)
>     end
>
>     if string.byte(version) ~= LJM_CURRENT_VERSION then
>       error(string_format(
> -      "LJM format version mismatch: the tool expects %d, but your data is %d",
> +      "Memprof event format version mismatch:"..
> +      " the tool expects %d, but your data is %d",
>         LJM_CURRENT_VERSION,
>         string.byte(version)
>       ))
> diff --git a/tools/sysprof/parse.lua b/tools/sysprof/parse.lua
> index 749f70db..0e416d37 100755
> --- a/tools/sysprof/parse.lua
> +++ b/tools/sysprof/parse.lua
> @@ -237,12 +237,13 @@ function M.parse(reader, symbols)
>     local _ = reader:read_octets(3)
>
>     if magic ~= LJP_MAGIC then
> -    error("Bad LJP format prologue: " .. tostring(magic))
> +    error("Bad sysprof event format prologue: " .. tostring(magic))
>     end
>
>     if string.byte(version) ~= LJP_CURRENT_VERSION then
>       error(string_format(
> -      "LJP format version mismatch: the tool expects %d, but your data is %d",
> +      "Sysprof event format version mismatch:"..
> +      " the tool expects %d, but your data is %d",
>         LJP_CURRENT_VERSION,
>         string.byte(version)
>       ))
> diff --git a/tools/utils/symtab.lua b/tools/utils/symtab.lua
> index c4aefef7..e80b9f33 100644
> --- a/tools/utils/symtab.lua
> +++ b/tools/utils/symtab.lua
> @@ -95,13 +95,13 @@ function M.parse(reader)
>     local _ = reader:read_octets(3)
>
>     if magic ~= LJS_MAGIC then
> -    error("Bad LJS format prologue: " .. tostring(magic))
> +    error("Bad LuaJIT symbol table format prologue: " .. tostring(magic))
>     end
>
>     if string.byte(version) ~= LJS_CURRENT_VERSION then
>       error(string_format(
> -         "LJS format version mismatch:"..
> -         "the tool expects %d, but your data is %d",
> +         "LuaJIT symbol table format version mismatch:"..
> +         " the tool expects %d, but your data is %d",
>            LJS_CURRENT_VERSION,
>            string.byte(version)
>       ))
> ===
>
> WBR,
> Max
> On Fri, Dec 29, 2023 at 03:27:46PM +0300, Sergey Bronnikov wrote:
>> Hello, Max
>>
>>
>> thanks for the patch. LGTM with minor comments
>>
>>
>> On 12/6/23 21:55, Maxim Kokryashkin wrote:
>>
>>
>> <snipped>
>>
>>> diff --git a/tools/sysprof/parse.lua b/tools/sysprof/parse.lua
>>> index 64c4a455..749f70db 100755
>>> --- a/tools/sysprof/parse.lua
>>> +++ b/tools/sysprof/parse.lua
>>> @@ -237,7 +237,7 @@ function M.parse(reader, symbols)
>>>      local _ = reader:read_octets(3)
>>>      if magic ~= LJP_MAGIC then
>>> -    error("Bad LJP format prologue: "..magic)
>>> +    error("Bad LJP format prologue: " .. tostring(magic))
>>>      end
>> LJP is not a well-known abbreviation. Probably, it is better to write the
>> error
>>
>> message without it. I see that LJP/LJS is used extensively in error message,
>> so feel free to
>>
>> ignore or fix in a separate patch.
>>
>>>      if string.byte(version) ~= LJP_CURRENT_VERSION then
>>> 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))
>>> +    os.exit(1, true)
>>> +  end
>>> +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
>>> 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
>>> @@ -95,7 +95,7 @@ function M.parse(reader)
>>>      local _ = reader:read_octets(3)
>>>      if magic ~= LJS_MAGIC then
>>> -    error("Bad LJS format prologue: "..magic)
>>> +    error("Bad LJS format prologue: " .. tostring(magic))
>> LJS is not a well-known abbreviation. Probably, it is better to write the
>> error
>>
>> message without it.
>>
>>>      end
>>>      if string.byte(version) ~= LJS_CURRENT_VERSION then