[PATCH 0/2 v2] fio: show function name in all fio errors

[PATCH 0/2 v2] fio: show function name in all fio errors

[Roman Khabibov]

Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3580-err-msg-pathjoin
Issue: https://github.com/tarantool/tarantool/issues/3580

Roman Khabibov (1):
  lua: modify the error message in 'fio.pathjoin'

Vladislav Shpilevoy (1):
  fio: show function name in all fio errors

 src/lua/fio.lua       | 30 +++++++++++++++---------------
 test/app/fio.result   | 13 +++++++++++++
 test/app/fio.test.lua |  5 +++++
 3 files changed, 33 insertions(+), 15 deletions(-)

-- 
2.19.1

[PATCH 1/2 v2] lua: modify the error message in 'fio.pathjoin'

[Roman Khabibov]

Display line number with error and fix the bug with bad argument number.

Closes #3580
---
 src/lua/fio.lua       |  4 ++--
 test/app/fio.result   | 13 +++++++++++++
 test/app/fio.test.lua |  5 +++++
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index 55faebdcb..1d0bb4bd1 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -213,7 +213,7 @@ fio.pathjoin = function(...)
     while i <= len do
         local sp = select(i, ...)
         if sp == nil then
-            error("Undefined path part "..i)
+            error("fio.pathjoin() undefined path part "..i, 2)
         end
 
         sp = tostring(sp)
@@ -233,7 +233,7 @@ fio.pathjoin = function(...)
     while i <= len do
         local sp = select(i, ...)
         if sp == nil then
-            error("Undefined path part")
+            error("fio.pathjoin() undefined path part "..i, 2)
         end
 
         sp = tostring(sp)
diff --git a/test/app/fio.result b/test/app/fio.result
index b7a1f65c6..9777a0aec 100644
--- a/test/app/fio.result
+++ b/test/app/fio.result
@@ -55,6 +55,19 @@ fio.pathjoin('abc', 'awdeq///qweqwqwe///', "//asda//")
 ---
 - abc/awdeq/qweqwqwe/asda
 ...
+--gh-3580 Modify the error message in 'fio.pathjoin'.
+fio.pathjoin(nil)
+---
+- error: fio.pathjoin() undefined path part 1
+...
+fio.pathjoin('abc', nil)
+---
+- error: fio.pathjoin() undefined path part 2
+...
+fio.pathjoin('abc', 'cde', nil)
+---
+- error: fio.pathjoin() undefined path part 3
+...
 -- basename
 st, err = pcall(fio.basename, nil)
 ---
diff --git a/test/app/fio.test.lua b/test/app/fio.test.lua
index 4f34fd11c..b6c79648c 100644
--- a/test/app/fio.test.lua
+++ b/test/app/fio.test.lua
@@ -18,6 +18,11 @@ fio.pathjoin('/', '/cde')
 fio.pathjoin('/a', '/')
 fio.pathjoin('abc', 'awdeq///qweqwqwe///', "//asda//")
 
+--gh-3580 Modify the error message in 'fio.pathjoin'.
+fio.pathjoin(nil)
+fio.pathjoin('abc', nil)
+fio.pathjoin('abc', 'cde', nil)
+
 -- basename
 st, err = pcall(fio.basename, nil)
 st
-- 
2.19.1

[PATCH 2/2 v2] fio: show function name in all fio errors

[Roman Khabibov]

From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>

Otherwise it is hard to debug code, throwing
exceptions. fio.pathjoin was just one example among
many.

Follow up #3580
---
 src/lua/fio.lua | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index 1d0bb4bd1..f279c9f10 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -126,7 +126,7 @@ fio_methods.seek = function(self, offset, whence)
     end
     if type(whence) == 'string' then
         if fio.c.seek[whence] == nil then
-            error(sprintf("Unknown whence: %s", whence))
+            error(sprintf("fio.seek(): unknown whence: %s", whence), 2)
         end
         whence = fio.c.seek[whence]
     else
@@ -164,7 +164,7 @@ fio.open = function(path, flags, mode)
     local iflag = 0
     local imode = 0
     if type(path) ~= 'string' then
-        error("Usage open(path[, flags[, mode]])")
+        error("Usage open(path[, flags[, mode]])", 2)
     end
     if type(flags) ~= 'table' then
         flags = { flags }
@@ -179,7 +179,7 @@ fio.open = function(path, flags, mode)
             iflag = bit.bor(iflag, flag)
         else
             if fio.c.flag[ flag ] == nil then
-                error(sprintf("Unknown flag: %s", flag))
+                error(sprintf("fio.open(): unknown flag: %s", flag), 2)
             end
             iflag = bit.bor(iflag, fio.c.flag[ flag ])
         end
@@ -188,7 +188,7 @@ fio.open = function(path, flags, mode)
     for _, m in pairs(mode) do
         if type(m) == 'string' then
             if fio.c.mode[m] == nil then
-                error(sprintf("Unknown mode: %s", m))
+                error(sprintf("fio.open(): unknown mode: %s", m), 2)
             end
             imode = bit.bor(imode, fio.c.mode[m])
         else
@@ -254,7 +254,7 @@ end
 
 fio.basename = function(path, suffix)
     if type(path) ~= 'string' then
-        error("Usage fio.basename(path[, suffix])")
+        error("Usage fio.basename(path[, suffix])", 2)
     end
 
     path = tostring(path)
@@ -273,7 +273,7 @@ end
 
 fio.dirname = function(path)
     if type(path) ~= 'string' then
-        error("Usage fio.dirname(path)")
+        error("Usage fio.dirname(path)", 2)
     end
     path = ffi.new('char[?]', #path + 1, path)
     return ffi.string(ffi.C.dirname(path))
@@ -297,7 +297,7 @@ fio.abspath = function(path)
     -- following established conventions of fio module:
     -- letting nil through and converting path to string
     if path == nil then
-        error("Usage fio.abspath(path)")
+        error("Usage fio.abspath(path)", 2)
     end
     path = path
     local joined_path = ''
@@ -319,14 +319,14 @@ end
 
 fio.chdir = function(path)
     if type(path)~='string' then
-        error("Usage: fio.chdir(path)")
+        error("Usage: fio.chdir(path)", 2)
     end
     return ffi.C.chdir(path) == 0
 end
 
 fio.listdir = function(path)
     if type(path) ~= 'string' then
-        error("Usage: fio.listdir(path)")
+        error("Usage: fio.listdir(path)", 2)
     end
     local str, err = internal.listdir(path)
     if err ~= nil then
@@ -345,7 +345,7 @@ end
 
 fio.mktree = function(path, mode)
     if type(path) ~= "string" then
-        error("Usage: fio.mktree(path[, mode])")
+        error("Usage: fio.mktree(path[, mode])", 2)
     end
     path = fio.abspath(path)
 
@@ -371,7 +371,7 @@ end
 
 fio.rmtree = function(path)
     if type(path) ~= 'string' then
-        error("Usage: fio.rmtree(path)")
+        error("Usage: fio.rmtree(path)", 2)
     end
     local status, err
     path = fio.abspath(path)
@@ -402,7 +402,7 @@ end
 
 fio.copyfile = function(from, to)
     if type(from) ~= 'string' or type(to) ~= 'string' then
-        error('Usage: fio.copyfile(from, to)')
+        error('Usage: fio.copyfile(from, to)', 2)
     end
     local st = fio.stat(to)
     if st and st:is_dir() then
@@ -417,7 +417,7 @@ end
 
 fio.copytree = function(from, to)
     if type(from) ~= 'string' or type(to) ~= 'string' then
-        error('Usage: fio.copytree(from, to)')
+        error('Usage: fio.copytree(from, to)', 2)
     end
     local status, reason
     local st = fio.stat(from)
-- 
2.19.1

Re: [PATCH 0/2 v2] fio: show function name in all fio errors

[Vladimir Davydov]

Alexander, please take a look.

On Tue, Dec 04, 2018 at 07:00:36PM +0300, Roman Khabibov wrote:
> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3580-err-msg-pathjoin
> Issue: https://github.com/tarantool/tarantool/issues/3580
> 
> Roman Khabibov (1):
>   lua: modify the error message in 'fio.pathjoin'
> 
> Vladislav Shpilevoy (1):
>   fio: show function name in all fio errors
> 
>  src/lua/fio.lua       | 30 +++++++++++++++---------------
>  test/app/fio.result   | 13 +++++++++++++
>  test/app/fio.test.lua |  5 +++++
>  3 files changed, 33 insertions(+), 15 deletions(-)

Re: [PATCH 0/2 v2] fio: show function name in all fio errors

[Alexander Turenko]

Hi all!

I don't think we should hide a real function where an error occurs. We
never know on what level the error was introduced. Let's see example:

```
local fio = require('fio')

local function read_app_config(app_name)
    local config_dir = os.getenv('TARANTOOL_APP_CONFIG_DIR') or '/etc'
    local config_path = fio.pathjoin(config_dir, app_name .. '.cfg.lua')
    dofile(config_path) -- exposes config = { ... } variable
    return config or {}
end

local function init()
    local config = read_app_config(os.getenv('TARANTOOL_APP_NAME'))
    ... do some initialization ...
end
```

When TARANTOOL_APP_NAME is not set in the example above the real error
is in the init() function and not in the read_app_config().

An application can use xpcall + debug.traceback to show or log
unexpected (or even expected) errors with full traceback.

I think within scope of this issue we should only add function names
into error messages and add path part number, but remove the second
parameter of the error() function where it is used across the fio
module.

WBR, Alexander Turenko.

On Tue, Dec 04, 2018 at 07:12:39PM +0300, Vladimir Davydov wrote:
> Alexander, please take a look.
> 
> On Tue, Dec 04, 2018 at 07:00:36PM +0300, Roman Khabibov wrote:
> > Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3580-err-msg-pathjoin
> > Issue: https://github.com/tarantool/tarantool/issues/3580
> > 
> > Roman Khabibov (1):
> >   lua: modify the error message in 'fio.pathjoin'
> > 
> > Vladislav Shpilevoy (1):
> >   fio: show function name in all fio errors
> > 
> >  src/lua/fio.lua       | 30 +++++++++++++++---------------
> >  test/app/fio.result   | 13 +++++++++++++
> >  test/app/fio.test.lua |  5 +++++
> >  3 files changed, 33 insertions(+), 15 deletions(-)

Re: [tarantool-patches] Re: [PATCH 0/2 v2] fio: show function name in all fio errors

[roman]

Hi! Thanks for review.
> I think within scope of this issue we should only add function names
> into error messages and add path part number, but remove the second
> parameter of the error() function where it is used across the fio
> module.

Removed.

diff --git a/src/lua/fio.lua b/src/lua/fio.lua
index 55faebdcb..2fee13e2d 100644
--- a/src/lua/fio.lua
+++ b/src/lua/fio.lua
@@ -126,7 +126,7 @@ fio_methods.seek = function(self, offset, whence)
      end
      if type(whence) == 'string' then
          if fio.c.seek[whence] == nil then
-            error(sprintf("Unknown whence: %s", whence))
+            error(sprintf("fio.seek(): unknown whence: %s", whence))
          end
          whence = fio.c.seek[whence]
      else
@@ -179,7 +179,7 @@ fio.open = function(path, flags, mode)
              iflag = bit.bor(iflag, flag)
          else
              if fio.c.flag[ flag ] == nil then
-                error(sprintf("Unknown flag: %s", flag))
+                error(sprintf("fio.open(): unknown flag: %s", flag))
              end
              iflag = bit.bor(iflag, fio.c.flag[ flag ])
          end
@@ -188,7 +188,7 @@ fio.open = function(path, flags, mode)
      for _, m in pairs(mode) do
          if type(m) == 'string' then
              if fio.c.mode[m] == nil then
-                error(sprintf("Unknown mode: %s", m))
+                error(sprintf("fio.open(): unknown mode: %s", m))
              end
              imode = bit.bor(imode, fio.c.mode[m])
          else
@@ -213,7 +213,7 @@ fio.pathjoin = function(...)
      while i <= len do
          local sp = select(i, ...)
          if sp == nil then
-            error("Undefined path part "..i)
+            error("fio.pathjoin() undefined path part "..i)
          end

          sp = tostring(sp)
@@ -233,7 +233,7 @@ fio.pathjoin = function(...)
      while i <= len do
          local sp = select(i, ...)
          if sp == nil then
-            error("Undefined path part")
+            error("fio.pathjoin() undefined path part "..i)
          end

          sp = tostring(sp)
diff --git a/test/app/fio.result b/test/app/fio.result
index b7a1f65c6..38d908276 100644
--- a/test/app/fio.result
+++ b/test/app/fio.result
@@ -55,6 +55,19 @@ fio.pathjoin('abc', 'awdeq///qweqwqwe///', "//asda//")
  ---
  - abc/awdeq/qweqwqwe/asda
  ...
+--gh-3580 Modify the error message in 'fio.pathjoin'.
+fio.pathjoin(nil)
+---
+- error: 'builtin/fio.lua:216: fio.pathjoin() undefined path part 1'
+...
+fio.pathjoin('abc', nil)
+---
+- error: 'builtin/fio.lua:236: fio.pathjoin() undefined path part 2'
+...
+fio.pathjoin('abc', 'cde', nil)
+---
+- error: 'builtin/fio.lua:236: fio.pathjoin() undefined path part 3'
+...
  -- basename
  st, err = pcall(fio.basename, nil)
  ---
diff --git a/test/app/fio.test.lua b/test/app/fio.test.lua
index 4f34fd11c..b6c79648c 100644
--- a/test/app/fio.test.lua
+++ b/test/app/fio.test.lua
@@ -18,6 +18,11 @@ fio.pathjoin('/', '/cde')
  fio.pathjoin('/a', '/')
  fio.pathjoin('abc', 'awdeq///qweqwqwe///', "//asda//")

+--gh-3580 Modify the error message in 'fio.pathjoin'.
+fio.pathjoin(nil)
+fio.pathjoin('abc', nil)
+fio.pathjoin('abc', 'cde', nil)
+
  -- basename
  st, err = pcall(fio.basename, nil)
  st

Re: [tarantool-patches] Re: [PATCH 0/2 v2] fio: show function name in all fio errors

[Alexander Turenko]

On Sat, Dec 08, 2018 at 04:41:08PM +0300, roman wrote:
> 
> Hi! Thanks for review.
> > I think within scope of this issue we should only add function names
> > into error messages and add path part number, but remove the second
> > parameter of the error() function where it is used across the fio
> > module.
> 
> Removed.

Now the code looks good to me.

But commit messages (of both commits) now do not reflect code changes.
Please, squash commits and fix the commit message.

WBR, Alexander Turenko.

Re: [tarantool-patches] Re: [PATCH 0/2 v2] fio: show function name in all fio errors

[roman]

> Now the code looks good to me.
>
> But commit messages (of both commits) now do not reflect code changes.
> Please, squash commits and fix the commit message.
>
> WBR, Alexander Turenko.
Done.

Re: [tarantool-patches] Re: [PATCH 0/2 v2] fio: show function name in all fio errors

[Alexander Turenko]

On Sat, Dec 08, 2018 at 06:02:43PM +0300, roman wrote:
> > Now the code looks good to me.
> > 
> > But commit messages (of both commits) now do not reflect code changes.
> > Please, squash commits and fix the commit message.
> > 
> > WBR, Alexander Turenko.
> Done.

Now ok.

Please, proceed further with Vova.

I see the patch is based on 1.10. Maybe maintainers will ask you to base
it on top of 2.1.

WBR, Alexander Turenko.

Re: [tarantool-patches] Re: [PATCH 0/2 v2] fio: show function name in all fio errors

[Vladimir Davydov]

On Sat, Dec 08, 2018 at 04:41:08PM +0300, roman wrote:
> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
> index 55faebdcb..2fee13e2d 100644
> --- a/src/lua/fio.lua
> +++ b/src/lua/fio.lua
> @@ -188,7 +188,7 @@ fio.open = function(path, flags, mode)
>      for _, m in pairs(mode) do
>          if type(m) == 'string' then
>              if fio.c.mode[m] == nil then
> -                error(sprintf("Unknown mode: %s", m))
> +                error(sprintf("fio.open(): unknown mode: %s", m))

Here you use a colon (:) ...

>              end
>              imode = bit.bor(imode, fio.c.mode[m])
>          else
> @@ -213,7 +213,7 @@ fio.pathjoin = function(...)
>      while i <= len do
>          local sp = select(i, ...)
>          if sp == nil then
> -            error("Undefined path part "..i)
> +            error("fio.pathjoin() undefined path part "..i)

... while here you don't. Please be consistent throughout the code.

Also, there's already some inconsistency among existing error messages:

        error("Usage open(path[, flags[, mode]])")
	             ^^^^ fio prefix missing

        error("Usage fio.basename(path[, suffix])")
	           ^^^ semicolon missing

        error("Usage: fio.chdir(path)")

Please fix them all in the same patch. I think they all should look
like:

        error("Usage: fio.function(args)")

Don't forget to update the commit message accordingly (something like
"fio: cleanup error messages") and reset the author (git commit --amend
--reset-author) - for some reason Vladislav authored this commit.

> diff --git a/test/app/fio.result b/test/app/fio.result
> index b7a1f65c6..38d908276 100644
> --- a/test/app/fio.result
> +++ b/test/app/fio.result
> @@ -55,6 +55,19 @@ fio.pathjoin('abc', 'awdeq///qweqwqwe///', "//asda//")
>  ---
>  - abc/awdeq/qweqwqwe/asda
>  ...
> +--gh-3580 Modify the error message in 'fio.pathjoin'.

This is a confusing comment IMO. Let's just say something like

-- gh-3580: Check that error messages are descriptive enough.

> +fio.pathjoin(nil)
> +---
> +- error: 'builtin/fio.lua:216: fio.pathjoin() undefined path part 1'
> +...
> +fio.pathjoin('abc', nil)
> +---
> +- error: 'builtin/fio.lua:236: fio.pathjoin() undefined path part 2'
> +...
> +fio.pathjoin('abc', 'cde', nil)
> +---
> +- error: 'builtin/fio.lua:236: fio.pathjoin() undefined path part 3'
> +...

You patched error messages returned by several fio functions, but tested
only one them. Please check all fio functions while we are at it.

If you agree with all my comments, please don't reply to this email.
Instead send v2 with a proper change log in a new mailing thread.

Thanks.