Re: [Tarantool-patches] [PATCH 3/9] error: move code to struct error from ClientError

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

06.11.2021 02:56, Vladislav Shpilevoy пишет:
> All optional fields soon will be moved into error_payload. Code
> was optional too. But it is needed too often, the most used field.
> The patch moves it into struct error to make it more accessible.
>
> Also in future it should allow to drop the hack
> ClientError::get_errcode() which tries to return error code
> depending on error type. But could just store the code right away.
>
> As a consequence of the patch, errors which didn't have an error
> code at all before, such as LuajitError, now have it 0 in Lua.
>
> Part of #5568

Thanks for the patch! Please, find 2 minor comments below.

> ---
>   src/box/error.cc              | 15 +++++++--------
>   src/box/error.h               |  4 +---
>   src/box/index.cc              |  4 ++--
>   src/box/mp_error.cc           |  7 +++----
>   src/lib/core/diag.c           |  1 +
>   src/lib/core/diag.h           |  2 ++
>   src/lua/error.lua             |  3 ++-
>   test/engine/func_index.result |  3 ++-
>   test/unit/mp_error.cc         |  2 +-
>   9 files changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/src/box/error.cc b/src/box/error.cc
> index 225b377b7..bc0d46601 100644
> --- a/src/box/error.cc
> +++ b/src/box/error.cc
> @@ -76,7 +76,7 @@ box_error_set(const char *file, unsigned line, uint32_t code,
>   	struct error *e = BuildClientError(file, line, ER_UNKNOWN);
>   	ClientError *client_error = type_cast(ClientError, e);
>   	if (client_error) {
> -		client_error->m_errcode = code;
> +		client_error->code = code;
>   		va_list ap;
>   		va_start(ap, fmt);
>   		error_vformat_msg(e, fmt, ap);
> @@ -94,7 +94,7 @@ box_error_new_va(const char *file, unsigned line, uint32_t code,
>   		struct error *e = BuildClientError(file, line, ER_UNKNOWN);
>   		ClientError *client_error = type_cast(ClientError, e);
>   		if (client_error != NULL) {
> -			client_error->m_errcode = code;
> +			client_error->code = code;
>   			error_vformat_msg(e, fmt, ap);
>   		}
>   		return e;
> @@ -170,7 +170,7 @@ ClientError::ClientError(const type_info *type, const char *file, unsigned line,
>   			 uint32_t errcode)
>   	:Exception(type, file, line)
>   {
> -	m_errcode = errcode;
> +	code = errcode;
>   	if (rmean_error)
>   		rmean_collect(rmean_error, RMEAN_ERROR, 1);
>   }
> @@ -181,7 +181,7 @@ ClientError::ClientError(const char *file, unsigned line,
>   {
>   	va_list ap;
>   	va_start(ap, errcode);
> -	error_vformat_msg(this, tnt_errcode_desc(m_errcode), ap);
> +	error_vformat_msg(this, tnt_errcode_desc(errcode), ap);
>   	va_end(ap);
>   }
>   
> @@ -194,7 +194,7 @@ BuildClientError(const char *file, unsigned line, uint32_t errcode, ...)
>   		va_start(ap, errcode);
>   		error_vformat_msg(e, tnt_errcode_desc(errcode), ap);
>   		va_end(ap);
> -		e->m_errcode = errcode;
> +		e->code = errcode;
>   		return e;
>   	} catch (OutOfMemory *e) {
>   		return e;
> @@ -204,8 +204,7 @@ BuildClientError(const char *file, unsigned line, uint32_t errcode, ...)
>   void
>   ClientError::log() const
>   {
> -	say_file_line(S_ERROR, file, line, errmsg, "%s",
> -		      tnt_errcode_str(m_errcode));
> +	say_file_line(S_ERROR, file, line, errmsg, "%s", tnt_errcode_str(code));
>   }
>   
>   
> @@ -296,7 +295,7 @@ AccessDeniedError::AccessDeniedError(const char *file, unsigned int line,
>   				     bool run_trigers)
>   	:ClientError(&type_AccessDeniedError, file, line, ER_ACCESS_DENIED)
>   {
> -	error_format_msg(this, tnt_errcode_desc(m_errcode),
> +	error_format_msg(this, tnt_errcode_desc(code),
>   			 access_type, object_type, object_name, user_name);
>   
>   	struct on_access_denied_ctx ctx = {access_type, object_type, object_name};
> diff --git a/src/box/error.h b/src/box/error.h
> index 338121dd9..30ab36a97 100644
> --- a/src/box/error.h
> +++ b/src/box/error.h
> @@ -208,14 +208,12 @@ public:
>   	int
>   	errcode() const
>   	{
> -		return m_errcode;
> +		return code;
>   	}
>   
>   	ClientError(const char *file, unsigned line, uint32_t errcode, ...);
>   
>   	static uint32_t get_errcode(const struct error *e);
> -	/* client errno code */
> -	int m_errcode;
>   protected:
>   	ClientError(const type_info *type, const char *file, unsigned line,
>   		    uint32_t errcode);
> diff --git a/src/box/index.cc b/src/box/index.cc
> index 0651868d3..be14827be 100644
> --- a/src/box/index.cc
> +++ b/src/box/index.cc
> @@ -47,8 +47,8 @@ UnsupportedIndexFeature::UnsupportedIndexFeature(const char *file,
>   	: ClientError(file, line, ER_UNKNOWN)
>   {
>   	struct space *space = space_cache_find_xc(index_def->space_id);
> -	m_errcode = ER_UNSUPPORTED_INDEX_FEATURE;
> -	error_format_msg(this, tnt_errcode_desc(m_errcode), index_def->name,
> +	code = ER_UNSUPPORTED_INDEX_FEATURE;
> +	error_format_msg(this, tnt_errcode_desc(code), index_def->name,
>   			 index_type_strs[index_def->type],
>   			 space->def->name, space->def->engine_name, what);
>   }
> diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
> index 059f9bc85..35c770400 100644
> --- a/src/box/mp_error.cc
> +++ b/src/box/mp_error.cc
> @@ -261,10 +261,8 @@ missing_fields:
>   	}
>   
>   	if (strcmp(mp_error->type, "ClientError") == 0) {
> -		ClientError *e = new ClientError(mp_error->file, mp_error->line,
> -						 ER_UNKNOWN);
> -		e->m_errcode = mp_error->code;
> -		err = e;
> +		err = new ClientError(mp_error->file, mp_error->line,
> +				      ER_UNKNOWN);
>   	} else if (strcmp(mp_error->type, "CustomError") == 0) {
>   		if (mp_error->custom_type == NULL)
>   			goto missing_fields;
> @@ -320,6 +318,7 @@ missing_fields:
>   		err = new ClientError(mp_error->file, mp_error->line,
>   				      ER_UNKNOWN);
>   	}
> +	err->code = mp_error->code;
>   	err->saved_errno = mp_error->saved_errno;
>   	error_format_msg(err, "%s", mp_error->message);
>   	return err;
> diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
> index f872be6a7..b557ca667 100644
> --- a/src/lib/core/diag.c
> +++ b/src/lib/core/diag.c
> @@ -116,6 +116,7 @@ error_create(struct error *e,
>   	e->type = type;
>   	e->refs = 0;
>   	e->saved_errno = 0;
> +	e->code = 0;
>   	if (file != NULL) {
>   		snprintf(e->file, sizeof(e->file), "%s", file);
>   		e->line = line;
> diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
> index d7d09f6fb..30b8d2f5d 100644
> --- a/src/lib/core/diag.h
> +++ b/src/lib/core/diag.h
> @@ -87,6 +87,8 @@ struct error {
>   	 * to the standard library, then it is 0.
>   	 */
>   	int saved_errno;
> +	/** Error code. Shortest possible description of error's reason. */
> +	int code;
>   	/** Line number. */
>   	unsigned line;
>   	/* Source file name. */
> diff --git a/src/lua/error.lua b/src/lua/error.lua
> index ebc784f07..9fb227df4 100644
> --- a/src/lua/error.lua
> +++ b/src/lua/error.lua
> @@ -18,6 +18,7 @@ struct error {
>       const struct type_info *_type;
>       int64_t _refs;
>       int _saved_errno;
> +    int code;

1. All the other fields are prefixed with "_" for some reason.
    Let's do the same for "code", just to comply.

>       /** Line number. */
>       unsigned _line;
>       /* Source file name. */
> @@ -158,7 +159,7 @@ local function error_unpack(err)
>       if not ffi.istype('struct error', err) then
>           error("Usage: error:unpack()")
>       end
> -    local result = {}
> +    local result = {code = err.code}

2. Why not add 'code' to 'error_fields' ?

>       for key, getter in pairs(error_fields)  do
>           result[key] = getter(err)
>       end
> diff --git a/test/engine/func_index.result b/test/engine/func_index.result
> index b689c68ec..d3f44f7be 100644
> --- a/test/engine/func_index.result
> +++ b/test/engine/func_index.result
> @@ -292,7 +292,8 @@ e = e.prev
>    | ...
>   e:unpack()
>    | ---
> - | - base_type: LuajitError
> + | - code: 0
> + |   base_type: LuajitError
>    |   type: LuajitError
>    |   message: '[string "return function(tuple)                 local ..."]:1: attempt
>    |     to call global ''require'' (a nil value)'
> diff --git a/test/unit/mp_error.cc b/test/unit/mp_error.cc
> index e85d282e7..fe0cd49b9 100644
> --- a/test/unit/mp_error.cc
> +++ b/test/unit/mp_error.cc
> @@ -349,7 +349,7 @@ test_decode_unknown_type()
>   	const char *pos = buffer;
>   	struct error *unpacked = error_unpack(&pos, len);
>   	error_ref(unpacked);
> -	err.code = 0;
> +	err.code = 1;
>   	err.type = "ClientError";
>   	ok(error_is_eq_mp_error(unpacked, &err), "check SomeNewError");
>   	error_unref(unpacked);

-- 
Serge Petrenko