Re: [Tarantool-patches] [PATCH 3/7] box/error: don't set error created via box.error.new to diag

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

See 6 comments below.

On 19/02/2020 15:16, Nikita Pettik wrote:
> To achieve this let's refactor luaT_error_create() to return error
> object instead of setting it via box_error_set().
> luaT_error_created() is used both to handle box.error() and

1. luaT_error_created() -> luaT_error_create().

> box.error.new() invocations, and box.error() is still expected to set
> error to diagnostic area. So, luaT_error_call() which implements
> box.error() processing at the end calls diag_set_error().
> It is worth mentioning that net.box module relied on the fact that
> box.error.new() set error to diagnostic area: otherwise request errors
> don't get to diagnostic area on client side.
> 
> @TarantoolBot document
> Title: Don't promote error created via box.error.new to diagnostic area
> 
> Now box.error.new() only creates error object, but doesn't set it to
> Tarantool's diagnostic area:
> box.error.clear()
> e = box.error.new({code = 111, reason = "cause"})
> assert(box.error.last() == nil)
> ---
> - true
> ...

2. It is better to wrap the code into ```, because otherwise it will
look like tickets created by Peter, with weird formatting, huge headers,
etc.

> To set error in diagnostic area explicitly box.error.set() has been
> introduced. It accepts error object which is set as last system error
> (i.e. becomes available via box.error.last()).
> Finally, box.error.new() does not longer accept error object as an
> argument (this was undocumented feature).
> Note that patch does not affect box.error(), which still pushed error to
> diagnostic area. This fact is reflected in docs:
> '''
> Emulate a request error, with text based on one of the pre-defined
> Tarantool errors...
> '''
> 
> Needed for #1148
> Closes #4778

3. This should be above docbot request. Otherwise it will appear in the
doc ticket.

> ---
>  src/box/error.cc       | 16 +++++++++++++++
>  src/box/error.h        |  8 ++++++++
>  src/box/lua/error.cc   | 56 ++++++++++++++++++++++++++++----------------------
>  test/box/misc.result   | 30 ++++++++++++++++++++++++++-
>  test/box/misc.test.lua | 16 +++++++++++++++
>  5 files changed, 101 insertions(+), 25 deletions(-)
> 
> diff --git a/src/box/error.cc b/src/box/error.cc
> index 7dfe1b3ee..824a4617c 100644
> --- a/src/box/error.cc
> +++ b/src/box/error.cc
> @@ -86,6 +86,22 @@ box_error_set(const char *file, unsigned line, uint32_t code,
>  	return -1;
>  }
>  
> +struct error *
> +box_error_construct(const char *file, unsigned line, uint32_t code,
> +		    const char *fmt, ...)
> +{
> +	struct error *e = BuildClientError(file, line, ER_UNKNOWN);
> +	ClientError *client_error = type_cast(ClientError, e);
> +	if (client_error != NULL) {
> +		client_error->m_errcode = code;

4. You can now make box_error_set() call box_error_construct() +
diag_set(), to avoid code duplication.

> +		va_list ap;
> +		va_start(ap, fmt);
> +		error_vformat_msg(e, fmt, ap);
> +		va_end(ap);
> +	}
> +	return e;
> +}
> +
>  /* }}} */
>  
>  struct rmean *rmean_error = NULL;
> diff --git a/src/box/error.h b/src/box/error.h
> index b8c7cf73d..42043ef80 100644
> --- a/src/box/error.h
> +++ b/src/box/error.h
> @@ -129,6 +129,14 @@ int
>  box_error_set(const char *file, unsigned line, uint32_t code,
>  	      const char *format, ...);
>  
> +/**
> + * Construct error object without setting it in the diagnostics
> + * area. On the memory allocation fail returns NULL.
> + */
> +struct error *
> +box_error_construct(const char *file, unsigned line, uint32_t code,
> +		    const char *fmt, ...);

5. You added the method to the public C API (it is inside 'cond public'
comments). I don't think we should do that. At least until someone
asked so.

Also why not box_error_new()? 'Construct' seems to be a very strange name.

> +
>  /**
>   * A backward-compatible API define.
>   */
> diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
> index 7311cb2ce..17d4de653 100644
> --- a/src/box/lua/error.cc
> +++ b/src/box/lua/error.cc
> @@ -42,7 +42,14 @@ extern "C" {
>  #include "lua/utils.h"
>  #include "box/error.h"
>  
> -static void
> +/**
> + * Parse Lua arguments (they can come as single table
> + * f({code : number, reason : string}) or as separate members
> + * f(code, reason)) and construct struct error with given values.
> + * In case one of arguments is missing it corresponding field

6. it -> its.

> + * in struct error is filled with default value.
> + */
> +static struct error *
>  luaT_error_create(lua_State *L, int top_base)
>  {
>  	uint32_t code = 0;