Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Leonid Vasiliev <lvasiliev@tarantool.org>,
	alexander.turenko@tarantool.org
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 1/6] error: Add a Lua backtrace to error
Date: Mon, 6 Apr 2020 00:14:34 +0200	[thread overview]
Message-ID: <4f1cec3e-2e30-bb81-24aa-98f5b66a5c34@tarantool.org> (raw)
In-Reply-To: <3a14bd84545d33eaded01ca0f23e12caba7eba9f.1585053742.git.lvasiliev@tarantool.org>

Thanks for the patch!

When you will work on the new patchset version, don't forget
to rebase on the latest master. Lots of things changed in errors.

See 7 comments below.

On 24/03/2020 13:45, Leonid Vasiliev wrote:
> Lua bactrace was added to a tarantool error.
> 
> In accordance with https://github.com/tarantool/tarantool/issues/4398
> we want to have a Lua backtrace for the box.error
> 
> @TarantoolBot document
> Title: error.bt
> 
> Lua backtrace was added to a tarantool error.
> 
> Needed for #4398
> ---
> diff --git a/src/lua/error.c b/src/lua/error.c
> index d82e78d..3a12e20 100644
> --- a/src/lua/error.c
> +++ b/src/lua/error.c
> @@ -34,8 +34,44 @@
>  #include <fiber.h>
>  #include "utils.h"
>  
> +#include <string.h>
> +
>  static int CTID_CONST_STRUCT_ERROR_REF = 0;
>  
> +/*
> + * Memory for the traceback string is obtained with malloc,
> + * and can be freed with free.
> +*/
> +static char*
> +traceback (lua_State *L) {

1. Please, rename to lua_traceback(), remove whitespace between
function name and arguments, add 'struct' before lua_State,
add whitespace after 'char', add second '*' in the begining of
the comment, because we always use /** for comments not inside of
a function body.

Also the comment is not really useful. It does not say which trace
it collects - Lua, C, both? Does it resolve symbols in case of C,
or pushes raw addresses?

> +	int top = lua_gettop(L);
> +
> +	lua_getfield(L, LUA_GLOBALSINDEX, "debug");
> +	if (!lua_istable(L, -1)) {
> +		lua_settop(L, top);
> +		return NULL;
> +	}
> +	lua_getfield(L, -1, "traceback");
> +	if (!lua_isfunction(L, -1)) {
> +		lua_settop(L, top);
> +		return NULL;
> +	}
> +
> +	// call debug.traceback

2. Sorry, omit such comments please. They are really useless.
It is like to say:

    // Condition is true if it is true.
    if (true) {
        // Condition was true.
        ...

Also please ask Igor if it is possible to get Lua traceback
from C without accessing LUA_GLOBALSINDEX, 'debug.traceback'.

> +	lua_call(L, 0, 1);
> +
> +	// get result of the debug.traceback call
> +	if (!lua_isstring(L, -1)) {
> +		lua_settop(L, top);
> +		return NULL;
> +	}
> +
> +	char *bt = strdup(lua_tostring(L, -1));
> +	lua_settop(L, top);
> +
> +	return bt;
> +}
> +
>  struct error *
>  luaL_iserror(struct lua_State *L, int narg)
>  {
> @@ -85,6 +121,13 @@ luaT_pusherror(struct lua_State *L, struct error *e)
>  	 * then set the finalizer.
>  	 */
>  	error_ref(e);
> +
> +	if (e->lua_bt == NULL) {
> +		char *lua_bt = traceback(L);
> +		error_set_lua_bt(e, lua_bt);

3. This is double copying. traceback() uses strdup(),
error_set_lua_bt() copies onto a realloced string again.
You could pass traceback result directly to error_set_lua_bt().

> +		free(lua_bt);
> +	}
> +
>  	assert(CTID_CONST_STRUCT_ERROR_REF != 0);
>  	struct error **ptr = (struct error **)
>  		luaL_pushcdata(L, CTID_CONST_STRUCT_ERROR_REF);
> diff --git a/src/lua/error.h b/src/lua/error.h
> index 64fa5eb..16cdaf7 100644
> --- a/src/lua/error.h
> +++ b/src/lua/error.h
> @@ -65,6 +65,9 @@ luaT_pusherror(struct lua_State *L, struct error *e);
>  struct error *
>  luaL_iserror(struct lua_State *L, int narg);
>  
> +struct error *
> +luaL_checkerror(struct lua_State *L, int narg);

4. Why? This function is still used only inside lua/error.c.

> +
>  void
>  tarantool_lua_error_init(struct lua_State *L);
>  
> diff --git a/src/lua/error.lua b/src/lua/error.lua
> index 7f24986..765ce73 100644
> --- a/src/lua/error.lua
> +++ b/src/lua/error.lua> @@ -83,10 +84,18 @@ local function error_trace(err)
>          return {}
>      end
>      return {
> -        { file = ffi.string(err._file), line = tonumber(err._line) };
> +        { file = ffi.string(err._file), line = tonumber(err._line) }

5. Please, omit not necessary diff.

>      }
>  end
>  
> +local function error_backtrace(err)
> +    local result = "Backtrace is absent"
> +    if err.lua_bt ~= ffi.nullptr then
> +        result = ffi.string(err.lua_bt)
> +    end
> +    return result
> +end
> +
>  local function error_errno(err)
>      local e = err._saved_errno
>      if e == 0 then
> @@ -96,10 +105,11 @@ local function error_errno(err)
>  end
>  
>  local error_fields = {
> -    ["type"]        = error_type;
> -    ["message"]     = error_message;
> -    ["trace"]       = error_trace;
> -    ["errno"]       = error_errno;
> +    ["type"]        = error_type,
> +    ["message"]     = error_message,
> +    ["trace"]       = error_trace,
> +    ["errno"]       = error_errno,
> +    ["bt"]          = error_backtrace

6. Please, keep the old fields with ';' to avoid
unnecessary changes. It does not prevent you from
adding backtrace.

Also seems like 'bt' is the only contraction here. Please,
use a full name. I thing traceback would be fine, similar
to debug.traceback. Although backtrace also looks good.
The only thing is that you should be consistent. And
currently you use both backtrace and traceback in different
places.

>  }
>  
>  local function error_unpack(err)
> diff --git a/test/app/fiber.result b/test/app/fiber.result
> index 7331f61..3908733 100644
> --- a/test/app/fiber.result
> +++ b/test/app/fiber.result
> @@ -1036,14 +1036,33 @@ st;
>  ---
>  - false
>  ...
> -e:unpack();
> ----
> -- type: ClientError
> -  code: 1
> -  message: Illegal parameters, oh my
> -  trace:
> -  - file: '[string "function err() box.error(box.error.ILLEGAL_PA..."]'
> -    line: 1
> +unpack_res = e:unpack();

7. Please, introduce new tests. Don't change existing. The
same for misc.test.lua.

  reply	other threads:[~2020-04-05 22:14 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-24 12:45 [Tarantool-patches] [PATCH 0/6] Extending error functionality Leonid Vasiliev
2020-03-24 12:45 ` [Tarantool-patches] [PATCH 1/6] error: Add a Lua backtrace to error Leonid Vasiliev
2020-04-05 22:14   ` Vladislav Shpilevoy [this message]
2020-04-08 13:56     ` Igor Munkin
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 2/6] error: Add the custom error type Leonid Vasiliev
2020-04-05 22:14   ` Vladislav Shpilevoy
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 3/6] iproto: Add negotiation phase Leonid Vasiliev
2020-03-24 20:02   ` Konstantin Osipov
2020-03-25  7:35     ` lvasiliev
2020-03-25  8:42       ` Konstantin Osipov
2020-03-25 10:56         ` Eugene Leonovich
2020-03-25 11:13           ` Konstantin Osipov
2020-03-26 11:37           ` lvasiliev
2020-03-26 11:18         ` lvasiliev
2020-03-26 12:16           ` Konstantin Osipov
2020-03-26 12:54             ` Kirill Yukhin
2020-03-26 13:19               ` Konstantin Osipov
2020-03-26 13:31                 ` Konstantin Osipov
2020-03-26 21:13       ` Alexander Turenko
2020-03-26 21:53         ` Alexander Turenko
2020-03-27  8:28         ` Konstantin Osipov
2020-03-26 23:35       ` Alexander Turenko
2020-03-27  8:39         ` Konstantin Osipov
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 4/6] error: Add extended error transfer format Leonid Vasiliev
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 5/6] error: Add test for extended error Leonid Vasiliev
2020-03-24 12:46 ` [Tarantool-patches] [PATCH 6/6] error: Transmit an error through IPROTO_OK as object Leonid Vasiliev
2020-03-27 23:11 ` [Tarantool-patches] [PATCH 0/6] Extending error functionality lvasiliev
2020-03-28 13:54   ` Alexander Turenko
2020-03-30 10:48     ` lvasiliev
2020-04-01 15:35 ` Alexander Turenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4f1cec3e-2e30-bb81-24aa-98f5b66a5c34@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=alexander.turenko@tarantool.org \
    --cc=lvasiliev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 1/6] error: Add a Lua backtrace to error' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox