[Tarantool-patches] [PATCH v2 2/5] error: Add the custom error type

lvasiliev lvasiliev at tarantool.org
Wed Apr 15 12:25:58 MSK 2020


Hi! Thanks for the review.

I applied your patch. Since I applied your patch, I did not write fixed 
on every comment. But I wrote a few questions where I did not understand 
your approach.

On 14.04.2020 4:11, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> Sorry, some of my comments may contain typos, because it is
> very late and I can't continue polishing it today.
> 
> See 21 comments below, review fixes in the end of the email,
> and on a new branch in a separate commit.
> 
> On 10/04/2020 10:10, Leonid Vasiliev wrote:
>> A possibility to create an error with a custom subtype was added.
>>
>> In accordance with https://github.com/tarantool/tarantool/issues/4398
>> a custom error type has been added to the box.error.
>> Now, it's possible to create an error with a custom subtype (string value)
>> for use it in applications.
>>
>> @TarantoolBot document
>> Title: error.custom_type
> 
> 1. custom_type field does not exist in any user visible API. Even
> after this patch.
> 
>> A custom error type has been added to the box.error.
>> Now, it's possible to create an error with a custom subtype (string value)
>> for use it in applications.
> 
> 2. When docbot duplicates commit message, you can omit the latter.
> 
>>
>> To API has been added:
>> - For create CustomError: box.error.new(type(str), format, ...)
>>                            box.error.new({type = str,
>>                                           reason = str})
>>
>> - error.type - CustomType error return custom type
>>      other errors return "base" error tybe
>> - base_type - return "base" errror type
>> Example:
>>
>> err_custom = box.error.new("My Custom Type", "Reason")
>> Now:
>> err_custom.type == "My Custom Type"
>> err_custom.base_type == "CustomType"
>> err_custom.message == "Reason"
> 
> 3. There is no a single word about name limit in 63 bytes.
> 
>>
>> Needed for #4398
>> ---
>>   src/box/errcode.h             |   1 +
>>   src/box/error.cc              |  59 +++++++++++++++++
>>   src/box/error.h               |  38 ++++++++++-
>>   src/box/lua/error.cc          | 147 ++++++++++++++++++++++++++++++------------
>>   src/lua/error.lua             |  11 ++++
>>   test/app/fiber.result         |   5 +-
>>   test/box/error.result         |  10 +--
>>   test/engine/func_index.result |  18 +++---
>>   8 files changed, 234 insertions(+), 55 deletions(-)
>>
>> diff --git a/src/box/error.cc b/src/box/error.cc
>> index 233b312..8179e52 100644
>> --- a/src/box/error.cc
>> +++ b/src/box/error.cc
>> @@ -125,6 +125,31 @@ box_error_add(const char *file, unsigned line, uint32_t code,
>>   
>>   /* }}} */
>>   
>> +const char *
>> +box_custom_error_type(const box_error_t *e)
> 
> 4. box_error_t is a type for the public API. This API is not public and
> therefore should use 'struct error' type name.
> 
> 5. Naming policy for functions is that there should be a prefix
> with the object type name in all functions. Or at least the prefix
> should be the same on all methods of one type.
> 
> Here the prefix should be box_error_*, so this method is
> box_error_custom_type(), and the method below is box_error_custom_new().
> 
>> +{
>> +	CustomError *custom_error = type_cast(CustomError, e);
>> +	if (custom_error)
>> +		return custom_error->custom_type();
>> +
>> +	return NULL;
>> +}
>> +
>> +struct error *
>> +box_custom_error_new(const char *file, unsigned line,
>> +		     const char *custom, const char *fmt, ...)
>> +{
>> +	struct error *e = BuildCustomError(file, line, custom);
>> +	CustomError *custom_error = type_cast(CustomError, e);
>> +	if (custom_error != NULL) {
>> +		va_list ap;
>> +		va_start(ap, fmt);
>> +		error_vformat_msg(e, fmt, ap);
>> +		va_end(ap);
>> +	}
>> +	return e;
>> +}
>> +
>>   struct rmean *rmean_error = NULL;
>>   
>>   const char *rmean_error_strings[RMEAN_ERROR_LAST] = {
>> @@ -290,3 +315,37 @@ BuildAccessDeniedError(const char *file, unsigned int line,
>>   		return e;
>>   	}
>>   }
>> +
>> +static struct method_info customerror_methods[] = {
>> +	make_method(&type_CustomError, "custom_type", &CustomError::custom_type),
>> +	METHODS_SENTINEL
>> +};
>> +
>> +const struct type_info type_CustomError =
>> +	make_type("CustomError", &type_ClientError,
>> +		  customerror_methods);
> 
> 6. Make_type() perfectly fits in one line.
> 
>> +
>> +CustomError::CustomError(const char *file, unsigned int line,
>> +			 const char *custom_type)
>> +	:ClientError(&type_CustomError, file, line, ER_CUSTOM_ERROR)
>> +{
>> +	error_format_msg(this, tnt_errcode_desc(m_errcode),
>> +			 custom_type ?: "");
> 
> 7. custom_type is never NULL.
Why? It's a constructor argument.
> 
>> +
>> +	if (custom_type) {
>> +		strncpy(m_custom_type, custom_type, sizeof(m_custom_type) - 1);
>> +		m_custom_type[sizeof(m_custom_type) - 1] = '\0';
>> +	} else {
>> +		m_custom_type[0] = '\0';
>> +	}
>> +}
>> +
>> +struct error *
>> +BuildCustomError(const char *file, unsigned int line, const char *custom_type)
>> +{
>> +	try {
>> +		return new CustomError(file, line, custom_type);
>> +	} catch (OutOfMemory *e) {
>> +		return e;
>> +	}
>> +}
>> diff --git a/src/box/error.h b/src/box/error.h
>> index ca5d5b2..5013488 100644
>> --- a/src/box/error.h
>> +++ b/src/box/error.h
>> @@ -53,6 +53,9 @@ struct error *
>>   BuildXlogGapError(const char *file, unsigned line,
>>   		  const struct vclock *from, const struct vclock *to);
>>   
>> +struct error *
>> +BuildCustomError(const char *file, unsigned int line, const char *custom_type);
>> +
>>   /** \cond public */
>>   
>>   struct error;
>> @@ -138,6 +141,14 @@ box_error_set(const char *file, unsigned line, uint32_t code,
>>   /** \endcond public */
>>   
>>   /**
>> + * Return the error custom type,
>> + * \param error
> 
> 8. \param or @param takes two arguments - parameter name and its
> description. You provided only name, and the name does not match
> the actual parameter, which is 'e'.
> 
>> + * \return pointer to custom error type. On error, return NULL
> 
> 9. Please, try to start sentences from a capital letter, and finish
> them with a dot.
> 
>> + */
>> +const char *
>> +box_custom_error_type(const box_error_t *e);
>> +
>> +/**
>>    * Add error to the diagnostic area. In contrast to box_error_set()
>>    * it does not replace previous error being set, but rather link
>>    * them into list.
>> @@ -155,15 +166,24 @@ box_error_add(const char *file, unsigned line, uint32_t code,
>>   
>>   /**
>>    * Construct error object without setting it in the diagnostics
>> - * area. On the memory allocation fail returns NULL.
>> + * area. On the memory allocation fail returns  OutOfMemory error.
> 
> 10. Unneccessary diff.
The comment is wrong. How I can fix it?
> 
>>    */
>>   struct error *
>>   box_error_new(const char *file, unsigned line, uint32_t code,
>>   	      const char *fmt, ...);
>>   
>> +/**
>> + * Construct error object without setting it in the diagnostics
>> + * area. On the memory allocation fail returns OutOfMemory error.
> 
> 11. This is a copy-paste box box_error_new() comment. Better write
> here a couple of words how is it different. Or better merge box_error_new()
> and box_custom_error_new(). Because in Lua I can specify both code
> and type. Don't see why can't I do this in C.
I thought we can’t change public API.
> 
> Honestly, I think we should use kind of struct error_args like you
> tried to do for lua/error.cc for the C API. Because with more
> arguments it will be hard to maintain them all as function paramters.
> But not now.
> 
>> + */
>> +struct error *
>> +box_custom_error_new(const char *file, unsigned line,
>> +		     const char *custom, const char *fmt, ...);
>> +
>>   extern const struct type_info type_ClientError;
>>   extern const struct type_info type_XlogError;
>>   extern const struct type_info type_AccessDeniedError;
>> +extern const struct type_info type_CustomError;
>>   
>>   #if defined(__cplusplus)
>>   } /* extern "C" */
>> @@ -290,6 +310,22 @@ struct XlogGapError: public XlogError
>>   	virtual void raise() { throw this; }
>>   };
>>   
>> +class CustomError: public ClientError
>> +{
>> +public:
>> +	CustomError(const char *file, unsigned int line,
>> +		    const char *custom_type);
>> +
>> +	const char*
>> +	custom_type()
>> +	{
>> +		return m_custom_type;
>> +	}
>> +private:
>> +	/** Custom type name */
>> +	char m_custom_type[64];
>> +};
>> +
>>   #endif /* defined(__cplusplus) */
>>   
>>   #endif /* TARANTOOL_BOX_ERROR_H_INCLUDED */
>> diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
>> index 4432bc8..1bcce1f 100644
>> --- a/src/box/lua/error.cc
>> +++ b/src/box/lua/error.cc
>> @@ -42,64 +42,101 @@ extern "C" {
>>   #include "lua/utils.h"
>>   #include "box/error.h"
>>   
>> -/**
>> - * 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 its corresponding field
>> - * in struct error is filled with default value.
>> - */
>> -static struct error *
> 
> 12. Most of changes in this file does not really realate to the
> feature you are trying to introduce. It is just refactoring. If
> you want it, it should be done in a separate commit, because
> otherwise it is hard to see which changes are functional and need
> more attention.
Discarded
> 
>> -luaT_error_create(lua_State *L, int top_base)
>> +struct error_args {
>> +	uint32_t code;
>> +	const char *reason;
>> +	const char *custom;
>> +	bool tb_mode;
>> +	bool tb_parsed;
> 
> 13. All structures and their attributes usually have a comment
> explaining their purpose. In this structure it is hard to
> understand what is 'tb', what is 'tb_mode', what is 'tb_parsed',
> if you are not in the context of the patch.
> 
>> +};
>> +
>> +static int
>> +luaT_error_parse_args(struct lua_State *L, int top_base,
>> +		      struct error_args *args)
>>   {
>> -	uint32_t code = 0;
>> -	const char *reason = NULL;
>> -	bool tb_parsed = false;
>> -	bool tb_mode = false;
>> -	const char *file = "";
>> -	unsigned line = 0;
>> -	lua_Debug info;
>>   	int top = lua_gettop(L);
>> -	if (top >= top_base && lua_type(L, top_base) == LUA_TNUMBER) {
>> -		code = lua_tonumber(L, top_base);
>> -		reason = tnt_errcode_desc(code);
>> +	int top_type = lua_type(L, top_base);
>> +	if (top >= top_base &&
>> +	    (top_type == LUA_TNUMBER || top_type == LUA_TSTRING)) {
>> +		if (top_type == LUA_TNUMBER) {
>> +			args->code = lua_tonumber(L, top_base);
>> +		} else if (top_type == LUA_TSTRING) {
>> +			args->code = ER_CUSTOM_ERROR;
>> +			args->custom = lua_tostring(L, top_base);
>> +		} else {
>> +			return -1;
>> +		}
>> +		args->reason = tnt_errcode_desc(args->code);
>>   		if (top > top_base) {
>>   			/* Call string.format(reason, ...) to format message */
>>   			lua_getglobal(L, "string");
>>   			if (lua_isnil(L, -1))
>> -				goto raise;
>> +				return 0;
>>   			lua_getfield(L, -1, "format");
>>   			if (lua_isnil(L, -1))
>> -				goto raise;
>> -			lua_pushstring(L, reason);
>> +				return 0;
>> +			lua_pushstring(L, args->reason);
>>   			for (int i = top_base + 1; i <= top; i++)
>>   				lua_pushvalue(L, i);
>>   			lua_call(L, top - top_base + 1, 1);
>> -			reason = lua_tostring(L, -1);
>> -		} else if (strchr(reason, '%') != NULL) {
>> +			args->reason = lua_tostring(L, -1);
>> +		} else if (strchr(args->reason, '%') != NULL) {
>>   			/* Missing arguments to format string */
>> -			return NULL;
>> +			return -1;
>>   		}
>> -	} else if (top == top_base && lua_istable(L, top_base)) {
>> +	} else if (top == top_base && top_type == LUA_TTABLE) {
>>   		lua_getfield(L, top_base, "code");
>> -		code = lua_tonumber(L, -1);
>> +		if (lua_isnil(L, -1) == 0)
>> +			args->code = lua_tonumber(L, -1);
> 
> 14. I reverted this change and didn't get any problems
> in the tests. Why?
> 
>>   		lua_pop(L, 1);
>>   		lua_getfield(L, top_base, "reason");
>> -		reason = lua_tostring(L, -1);
>> -		if (reason == NULL)
>> -			reason = "";
>> +		if (lua_isnil(L, -1) == 0)
>> +			args->reason = lua_tostring(L, -1);
> 
> 15. Previously it was not checked for nil, and all worked fine.
> Why did you change that?
> 
>> +		lua_pop(L, 1);
>> +		lua_getfield(L, top_base, "type");
>> +		if (lua_isnil(L, -1) == 0)
>> +			args->custom = lua_tostring(L, -1);
>>   		lua_pop(L, 1);
>>   		lua_getfield(L, top_base, "traceback");
>>   		if (lua_isboolean(L, -1)) {
>> -			tb_parsed = true;
>> -			tb_mode = lua_toboolean(L, -1);
>> +			args->tb_parsed = true;
>> +			args->tb_mode = lua_toboolean(L, -1);
>>   		}
>>   		lua_pop(L, -1);
>>   	} else {
>> +		return -1;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * 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.
> 
> 16. The comment does not describe new syntax with 'type' argument.
> 
>> + * In case one of arguments is missing its corresponding field
>> + * in struct error is filled with default value.
>> + */
>> +static struct error *
>> +luaT_error_create(lua_State *L, int top_base)
>> +{
>> +	struct error_args args;
>> +	args.code = UINT32_MAX;
> 
> 17. Before your patch default error code was 0. Why is it
> changed to UINT32_MAX?
> 
>> +	args.reason = NULL;
>> +	args.custom = NULL;
>> +	args.tb_mode = false;
>> +	args.tb_parsed = false;
>> +
>> +	if (luaT_error_parse_args(L, top_base, &args) != 0) {
>>   		return NULL;
>>   	}
>>   
>> -raise:
>> +	if (args.reason == NULL)
>> +		args.reason = "";
>> +
>> +	const char *file = "";
>> +	unsigned line = 0;
>> +	lua_Debug info;
>>   	if (lua_getstack(L, 1, &info) && lua_getinfo(L, "Sl", &info)) {
>>   		if (*info.short_src) {
>>   			file = info.short_src;
>> @@ -111,9 +148,16 @@ raise:
>>   		line = info.currentline;
>>   	}
>>   
>> -	struct error *err = box_error_new(file, line, code, "%s", reason);
>> -	if (tb_parsed)
>> -		err->traceback_mode = tb_mode;
>> +	struct error *err = NULL;
>> +	if (args.custom) {
>> +		err = box_custom_error_new(file, line, args.custom,
>> +					   "%s", args.reason);
> 
> 18. Code argument is ignored here. Although I am not sure it shouldn't
> be ignored. From what I remember, we decided that code should stay,
> but I may be wrong.
CustomError type has a predefinded code. Why it shouldn't be ignored?
> 
>> +	} else {
>> +		err = box_error_new(file, line, args.code, "%s", args.reason);
>> +	}
>> +
>> +	if (args.tb_parsed)
>> +		err->traceback_mode = args.tb_mode;
>>   
>>   	return err;
>>   }
>> @@ -162,17 +206,36 @@ luaT_error_last(lua_State *L)
>>   static int
>>   luaT_error_new(lua_State *L)
>>   {
>> -	if (lua_gettop(L) == 0)
>> -		return luaL_error(L, "Usage: box.error.new(code, args)");
>> +	if (lua_gettop(L) == 0) {
>> +		return luaL_error(L, "Usage: box.error.new(code, args) or "\
>> +				     "box.error.new(type, args)");
>> +	}
>>   	struct error *e = luaT_error_create(L, 1);
>> -	if (e == NULL)
>> -		return luaL_error(L, "Usage: box.error.new(code, args)");
>> +	if (e == NULL) {
>> +		return luaL_error(L, "Usage: box.error.new(code, args) or "\
>> +				     "box.error.new(type, args)");
>> +	}
>>   	lua_settop(L, 0);
>>   	luaT_pusherror(L, e);
>>   	return 1;
>>   }
>>   
>>   static int
>> +luaT_error_custom_type(lua_State *L)
> 
> 19. This method is not needed. We don't have 'global' getters
> like box.error.prev, or box.error.type, and so on. Every
> error object provides API to get its members without calling
> global functions.
> 
>> +{
>> +	struct error *e = luaL_checkerror(L, -1);
>> +
>> +	const char *custom_type = box_custom_error_type(e);
>> +	if (custom_type == NULL) {
>> +		lua_pushfstring(L, "The error has't custom type");
>> +		return 1;
>> +	}
>> +
>> +	lua_pushstring(L, custom_type);
>> +	return 1;
>> +}
>> +
>> +static int
>>   luaT_error_clear(lua_State *L)
>>   {
>>   	if (lua_gettop(L) >= 1)
>> diff --git a/src/lua/error.lua b/src/lua/error.lua
>> index 46d2866..5b32607 100644
>> --- a/src/lua/error.lua
>> +++ b/src/lua/error.lua
>> @@ -78,6 +78,16 @@ local function reflection_get(err, method)
>>   end
>>   
>>   local function error_type(err)
>> +    local res
>> +    if ffi.string(err._type.name) == 'CustomError' then
>> +        res = box.error.custom_type(err)
>> +    else
>> +        res = ffi.string(err._type.name)
> 
> 20. ffi.string() is called on one string 2 times. But it
> is not free, it allocates a new string each time.
> 
>> +    end
>> +    return res
>> +end
> 
> 21. There is again not a single test.
That's my mistake. After our discussion for testing I used the test 
added in the last commit and deleted the ones added before in misc.
> 
> Consider my review fixes below and on a new branch in a
> separate commit. Note, it is not entirely separated
> from the original commit in a couple of places, since I had
> rebase conflicts.
I applied your patch and added you to Co-authored.
> 
> Branch is:
> 
>      lvasiliev/gh-4398-expose-error-module-4-review
> 
> ====================
>      Review fixes
>      
>      New commit message proposal:
>      
>      error: add custom error type
>      
>      Part of #4398
>      
>      @TarantoolBot document
>      Title: Custom error types for Lua errors
>      
>      Errors can be created in 2 ways: `box.error.new()` and `box.error()`.
>      
>      Both used to take either `code, reason, <reason string args>` or
>      `{code = code, reason = reason, ...}` arguments.
>      
>      Now in the first option instead of code a user can specify a
>      string as its own error type. In the second option a user can
>      specify both code and type. For example:
>      
>      ```Lua
>      box.error('MyErrorType', 'Message')
>      box.error({type = 'MyErrorType', code = 1024, reason = 'Message'})
>      ```
>      Or no-throw version:
>      ```Lua
>      box.error.new('MyErrorType', 'Message')
>      box.error.new({type = 'MyErrorType', code = 1024, reason = 'Message'})
>      ```
>      When a custom type is specified, it is shown in `err.type`
>      attribute. When it is not specified, `err.type` shows one of
>      built-in errors such as 'ClientError', 'OurOfMemory', etc.
>      
>      Name length limit on the custom type is 63 bytes. All is longer
>      is truncated.
>      
>      Original error type can be checked using `err.base_type` member,
>      although normally it should not be used. For user-defined types
>      base type is 'CustomError'.
>      
>      For example:
>      ```
>      tarantool> e = box.error.new({type = 'MyErrorType', code = 1024, reason = 'Message'})
>      ---
>      ...
>      
>      tarantool> e:unpack()
>      ---
>      - code: 1024
>        trace:
>        - file: '[string "e = box.error.new({type = ''MyErrorType'', code..."]'
>          line: 1
>        type: MyErrorType
>        custom_type: MyErrorType
>        message: Message
>        base_type: CustomError
>      ...
>      ```
> 
> diff --git a/extra/exports b/extra/exports
> index ffd11145d..a5ebe0884 100644
> --- a/extra/exports
> +++ b/extra/exports
> @@ -234,6 +234,7 @@ box_index_min
>   box_index_max
>   box_index_count
>   box_error_type
> +box_error_custom_type
>   box_error_code
>   box_error_message
>   box_error_last
> diff --git a/src/box/error.cc b/src/box/error.cc
> index 8179e5287..d854b86e9 100644
> --- a/src/box/error.cc
> +++ b/src/box/error.cc
> @@ -86,35 +86,51 @@ box_error_set(const char *file, unsigned line, uint32_t code,
>   	return -1;
>   }
>   
> +static struct error *
> +box_error_new_va(const char *file, unsigned line, uint32_t code,
> +		 const char *custom_type, const char *fmt, va_list ap)
> +{
> +	if (custom_type == NULL) {
> +		struct error *e = BuildClientError(file, line, ER_UNKNOWN);
> +		ClientError *client_error = type_cast(ClientError, e);
> +		if (client_error != NULL) {
> +			client_error->m_errcode = code;
> +			error_vformat_msg(e, fmt, ap);
> +		}
> +		return e;
> +	} else {
> +		struct error *e = BuildCustomError(file, line, custom_type);
> +		CustomError *custom_error = type_cast(CustomError, e);
> +		if (custom_error != NULL) {
> +			custom_error->m_errcode = code;
> +			error_vformat_msg(e, fmt, ap);
> +		}
> +		return e;
> +	}
> +}
> +
>   struct error *
>   box_error_new(const char *file, unsigned line, uint32_t code,
> -	      const char *fmt, ...)
> +	      const char *custom_type, 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;
> -		va_list ap;
> -		va_start(ap, fmt);
> -		error_vformat_msg(e, fmt, ap);
> -		va_end(ap);
> -	}
> +	va_list ap;
> +	va_start(ap, fmt);
> +	struct error *e = box_error_new_va(file, line, code, custom_type,
> +					   fmt, ap);
> +	va_end(ap);
>   	return e;
>   }
>   
>   int
>   box_error_add(const char *file, unsigned line, uint32_t code,
> -	      const char *fmt, ...)
> +	      const char *custom_type, const char *fmt, ...)
>   {
> -	struct error *e = BuildClientError(file, line, ER_UNKNOWN);
> -	ClientError *client_error = type_cast(ClientError, e);
> -	if (client_error) {
> -		client_error->m_errcode = code;
> -		va_list ap;
> -		va_start(ap, fmt);
> -		error_vformat_msg(e, fmt, ap);
> -		va_end(ap);
> -	}
> +	va_list ap;
> +	va_start(ap, fmt);
> +	struct error *e = box_error_new_va(file, line, code, custom_type,
> +					   fmt, ap);
> +	va_end(ap);
> +
>   	struct diag *d = &fiber()->diag;
>   	if (diag_is_empty(d))
>   		diag_set_error(d, e);
> @@ -126,7 +142,7 @@ box_error_add(const char *file, unsigned line, uint32_t code,
>   /* }}} */
>   
>   const char *
> -box_custom_error_type(const box_error_t *e)
> +box_error_custom_type(const struct error *e)
>   {
>   	CustomError *custom_error = type_cast(CustomError, e);
>   	if (custom_error)
> @@ -135,21 +151,6 @@ box_custom_error_type(const box_error_t *e)
>   	return NULL;
>   }
>   
> -struct error *
> -box_custom_error_new(const char *file, unsigned line,
> -		     const char *custom, const char *fmt, ...)
> -{
> -	struct error *e = BuildCustomError(file, line, custom);
> -	CustomError *custom_error = type_cast(CustomError, e);
> -	if (custom_error != NULL) {
> -		va_list ap;
> -		va_start(ap, fmt);
> -		error_vformat_msg(e, fmt, ap);
> -		va_end(ap);
> -	}
> -	return e;
> -}
> -
>   struct rmean *rmean_error = NULL;
>   
>   const char *rmean_error_strings[RMEAN_ERROR_LAST] = {
> @@ -322,22 +323,15 @@ static struct method_info customerror_methods[] = {
>   };
>   
>   const struct type_info type_CustomError =
> -	make_type("CustomError", &type_ClientError,
> -		  customerror_methods);
> +	make_type("CustomError", &type_ClientError, customerror_methods);
>   
>   CustomError::CustomError(const char *file, unsigned int line,
>   			 const char *custom_type)
>   	:ClientError(&type_CustomError, file, line, ER_CUSTOM_ERROR)
>   {
> -	error_format_msg(this, tnt_errcode_desc(m_errcode),
> -			 custom_type ?: "");
> -
> -	if (custom_type) {
> -		strncpy(m_custom_type, custom_type, sizeof(m_custom_type) - 1);
> -		m_custom_type[sizeof(m_custom_type) - 1] = '\0';
> -	} else {
> -		m_custom_type[0] = '\0';
> -	}
> +	error_format_msg(this, tnt_errcode_desc(m_errcode), custom_type);
> +	strncpy(m_custom_type, custom_type, sizeof(m_custom_type) - 1);
> +	m_custom_type[sizeof(m_custom_type) - 1] = '\0';
>   }
>   
>   struct error *
> diff --git a/src/box/error.h b/src/box/error.h
> index 501348885..540008f35 100644
> --- a/src/box/error.h
> +++ b/src/box/error.h
> @@ -142,11 +142,11 @@ box_error_set(const char *file, unsigned line, uint32_t code,
>   
>   /**
>    * Return the error custom type,
> - * \param error
> - * \return pointer to custom error type. On error, return NULL
> + * \param e Error object.
> + * \return Pointer to custom error type. On error return NULL.
>    */
>   const char *
> -box_custom_error_type(const box_error_t *e);
> +box_error_custom_type(const struct error *e);
>   
>   /**
>    * Add error to the diagnostic area. In contrast to box_error_set()
> @@ -154,6 +154,8 @@ box_custom_error_type(const box_error_t *e);
>    * them into list.
>    *
>    * \param code IPROTO error code (enum \link box_error_code \endlink)
> + * \param custom_type User-defined error type which will be
> + *       displayed instead of ClientError.
>    * \param format (const char * ) - printf()-like format string
>    * \param ... - format arguments
>    * \returns -1 for convention use
> @@ -162,23 +164,15 @@ box_custom_error_type(const box_error_t *e);
>    */
>   int
>   box_error_add(const char *file, unsigned line, uint32_t code,
> -	      const char *fmt, ...);
> +	      const char *custom_type, const char *fmt, ...);
>   
>   /**
>    * Construct error object without setting it in the diagnostics
> - * area. On the memory allocation fail returns  OutOfMemory error.
> + * area. On the memory allocation fail returns NULL.
>    */
>   struct error *
>   box_error_new(const char *file, unsigned line, uint32_t code,
> -	      const char *fmt, ...);
> -
> -/**
> - * Construct error object without setting it in the diagnostics
> - * area. On the memory allocation fail returns OutOfMemory error.
> - */
> -struct error *
> -box_custom_error_new(const char *file, unsigned line,
> -		     const char *custom, const char *fmt, ...);
> +	      const char *custom_type, const char *fmt, ...);
>   
>   extern const struct type_info type_ClientError;
>   extern const struct type_info type_XlogError;
> diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
> index dd7068a83..cf771935e 100644
> --- a/src/box/lua/error.cc
> +++ b/src/box/lua/error.cc
> @@ -42,95 +42,83 @@ extern "C" {
>   #include "lua/utils.h"
>   #include "box/error.h"
>   
> -struct error_args {
> -	uint32_t code;
> -	const char *reason;
> -	const char *custom;
> -	const char *traceback;
> -};
> -
> -static int
> -luaT_error_parse_args(struct lua_State *L, int top_base,
> -		      struct error_args *args)
> +/**
> + * 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.
> + *
> + * Instead of 'code' it is possible to specify a string name of
> + * the error object's type:
> + *
> + *     box.error(type, reason, ...)
> + *     box.error({type = string, reason = string, ...})
> + *
> + * In case one of arguments is missing its corresponding field
> + * in struct error is filled with default value.
> + */
> +static struct error *
> +luaT_error_create(lua_State *L, int top_base)
>   {
> +	uint32_t code = 0;
> +	const char *custom_type = NULL;
> +	const char *reason = NULL;
> +	bool is_traceback_enabled = false;
> +	bool is_traceback_specified = false;
> +	const char *file = "";
> +	unsigned line = 0;
> +	lua_Debug info;
>   	int top = lua_gettop(L);
>   	int top_type = lua_type(L, top_base);
> -	if (top >= top_base &&
> -	    (top_type == LUA_TNUMBER || top_type == LUA_TSTRING)) {
> +	if (top >= top_base && (top_type == LUA_TNUMBER ||
> +				top_type == LUA_TSTRING)) {
>   		if (top_type == LUA_TNUMBER) {
> -			args->code = lua_tonumber(L, top_base);
> -		} else if (top_type == LUA_TSTRING) {
> -			args->code = ER_CUSTOM_ERROR;
> -			args->custom = lua_tostring(L, top_base);
> +			code = lua_tonumber(L, top_base);
>   		} else {
> -			return -1;
> +			code = ER_CUSTOM_ERROR;
> +			custom_type = lua_tostring(L, top_base);
>   		}
> -		args->reason = tnt_errcode_desc(args->code);
> +		reason = tnt_errcode_desc(code);
>   		if (top > top_base) {
>   			/* Call string.format(reason, ...) to format message */
>   			lua_getglobal(L, "string");
>   			if (lua_isnil(L, -1))
> -				return 0;
> +				goto raise;
>   			lua_getfield(L, -1, "format");
>   			if (lua_isnil(L, -1))
> -				return 0;
> -			lua_pushstring(L, args->reason);
> +				goto raise;
> +			lua_pushstring(L, reason);
>   			for (int i = top_base + 1; i <= top; i++)
>   				lua_pushvalue(L, i);
>   			lua_call(L, top - top_base + 1, 1);
> -			args->reason = lua_tostring(L, -1);
> -		} else if (strchr(args->reason, '%') != NULL) {
> +			reason = lua_tostring(L, -1);
> +		} else if (strchr(reason, '%') != NULL) {
>   			/* Missing arguments to format string */
> -			return -1;
> +			return NULL;
>   		}
>   	} else if (top == top_base && top_type == LUA_TTABLE) {
>   		lua_getfield(L, top_base, "code");
> -		if (lua_isnil(L, -1) == 0)
> -			args->code = lua_tonumber(L, -1);
> +		if (!lua_isnil(L, -1))
> +			code = lua_tonumber(L, -1);
> +		else
> +			code = ER_CUSTOM_ERROR;
>   		lua_getfield(L, top_base, "reason");
> -		if (lua_isnil(L, -1) == 0)
> -			args->reason = lua_tostring(L, -1);
> +		reason = lua_tostring(L, -1);
> +		if (reason == NULL)
> +			reason = "";
>   		lua_getfield(L, top_base, "type");
> -		if (lua_isnil(L, -1) == 0)
> -			args->custom = lua_tostring(L, -1);
> +		if (!lua_isnil(L, -1))
> +			custom_type = lua_tostring(L, -1);
>   		lua_getfield(L, top_base, "traceback");
> -		if (lua_isboolean(L, -1) && lua_toboolean(L, -1)) {
> -			luaL_traceback(L, L, NULL, 0);
> -			if (lua_isstring(L, -1))
> -				args->traceback = lua_tostring(L, -1);
> +		if (lua_isboolean(L, -1)) {
> +			is_traceback_enabled = lua_toboolean(L, -1);
> +			is_traceback_specified = true;
>   		}
>   		lua_pop(L, 1);
>   	} else {
> -		return -1;
> -	}
> -	return 0;
> -}
> -
> -/**
> - * 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 its corresponding field
> - * in struct error is filled with default value.
> - */
> -static struct error *
> -luaT_error_create(lua_State *L, int top_base)
> -{
> -	struct error_args args;
> -	args.code = UINT32_MAX;
> -	args.reason = NULL;
> -	args.custom = NULL;
> -	args.traceback = NULL;
> -
> -	if (luaT_error_parse_args(L, top_base, &args) != 0)
>   		return NULL;
> +	}
>   
> -	if (args.reason == NULL)
> -		args.reason = "";
> -
> -	const char *file = "";
> -	unsigned line = 0;
> -	lua_Debug info;
> +raise:
>   	if (lua_getstack(L, 1, &info) && lua_getinfo(L, "Sl", &info)) {
>   		if (*info.short_src) {
>   			file = info.short_src;
> @@ -142,15 +130,13 @@ luaT_error_create(lua_State *L, int top_base)
>   		line = info.currentline;
>   	}
>   
> -	struct error *err;
> -	if (args.custom) {
> -		err = box_custom_error_new(file, line, args.custom,
> -					   "%s", args.reason);
> -	} else {
> -		err = box_error_new(file, line, args.code, "%s", args.reason);
> -	}
> -	if (args.traceback != NULL && err != NULL)
> -		error_set_traceback(err, args.traceback);
> +	struct error *err = box_error_new(file, line, code, custom_type,
> +					  "%s", reason);
> +	/*
> +	 * Explicit traceback option overrides the global setting.
> +	 */
> +	if (err != NULL && is_traceback_specified)
> +		err->is_traceback_enabled = is_traceback_enabled;
>   	return err;
>   }
>   
> @@ -198,35 +184,16 @@ luaT_error_last(lua_State *L)
>   static int
>   luaT_error_new(lua_State *L)
>   {
> -	if (lua_gettop(L) == 0) {
> -		return luaL_error(L, "Usage: box.error.new(code, args) or "\
> -				     "box.error.new(type, args)");
> -	}
> -	struct error *e = luaT_error_create(L, 1);
> -	if (e == NULL) {
> +	struct error *e;
> +	if (lua_gettop(L) == 0 || (e = luaT_error_create(L, 1)) == NULL) {
>   		return luaL_error(L, "Usage: box.error.new(code, args) or "\
> -				     "box.error.new(type, args)");
> +				  "box.error.new(type, args)");
>   	}
>   	lua_settop(L, 0);
>   	luaT_pusherror(L, e);
>   	return 1;
>   }
>   
> -static int
> -luaT_error_custom_type(lua_State *L)
> -{
> -	struct error *e = luaL_checkerror(L, -1);
> -
> -	const char *custom_type = box_custom_error_type(e);
> -	if (custom_type == NULL) {
> -		lua_pushfstring(L, "The error has't custom type");
> -		return 1;
> -	}
> -
> -	lua_pushstring(L, custom_type);
> -	return 1;
> -}
> -
>   static int
>   luaT_error_clear(lua_State *L)
>   {
> @@ -381,10 +348,6 @@ box_lua_error_init(struct lua_State *L) {
>   			lua_pushcfunction(L, luaT_error_cfg);
>   			lua_setfield(L, -2, "cfg");
>   		}
> -		{
> -			lua_pushcfunction(L, luaT_error_custom_type);
> -			lua_setfield(L, -2, "custom_type");
> -		}
>   		lua_setfield(L, -2, "__index");
>   	}
>   	lua_setmetatable(L, -2);
> diff --git a/src/box/xrow.c b/src/box/xrow.c
> index a494d1f46..5494b41cd 100644
> --- a/src/box/xrow.c
> +++ b/src/box/xrow.c
> @@ -1123,7 +1123,7 @@ iproto_decode_error_stack(const char **pos)
>   				continue;
>   			}
>   		}
> -		box_error_add(__FILE__, __LINE__, code, reason);
> +		box_error_add(__FILE__, __LINE__, code, NULL, reason);
>   	}
>   	return 0;
>   }
> diff --git a/src/lua/error.lua b/src/lua/error.lua
> index 1f9ae9375..1d3413b19 100644
> --- a/src/lua/error.lua
> +++ b/src/lua/error.lua
> @@ -37,6 +37,9 @@ exception_get_int(struct error *e, const struct method_info *method);
>   
>   int
>   error_set_prev(struct error *e, struct error *prev);
> +
> +const char *
> +box_error_custom_type(const struct error *e);
>   ]]
>   
>   local REFLECTION_CACHE = {}
> @@ -77,20 +80,18 @@ local function reflection_get(err, method)
>       end
>   end
>   
> -local function error_type(err)
> -    local res
> -    if ffi.string(err._type.name) == 'CustomError' then
> -        res = box.error.custom_type(err)
> -    else
> -        res = ffi.string(err._type.name)
> -    end
> -    return res
> -end
> -
>   local function error_base_type(err)
>       return ffi.string(err._type.name)
>   end
>   
> +local function error_type(err)
> +    local res = ffi.C.box_error_custom_type(err)
> +    if res ~= nil then
> +        return ffi.string(res)
> +    end
> +    return error_base_type(err)
> +end
> +
>   local function error_message(err)
>       return ffi.string(err._errmsg)
>   end
> @@ -148,7 +149,7 @@ local error_fields = {
>       ["errno"]       = error_errno;
>       ["prev"]        = error_prev;
>       ["traceback"]   = error_traceback;
> -    ["base_type"]    = error_base_type
> +    ["base_type"]   = error_base_type,
>   }
>   
>   local function error_unpack(err)
> diff --git a/test/box/error.result b/test/box/error.result
> index 2b837e434..b717a4ff4 100644
> --- a/test/box/error.result
> +++ b/test/box/error.result
> @@ -431,11 +431,8 @@ t;
>    |   210: box.error.SQL_PREPARE
>    |   211: box.error.WRONG_QUERY_ID
>    |   212: box.error.SEQUENCE_NOT_STARTED
> -<<<<<<< HEAD
>    |   213: box.error.NO_SUCH_SESSION_SETTING
> -=======
> - |   213: box.error.CUSTOM_ERROR
> ->>>>>>> error: Add the custom error type
> + |   214: box.error.CUSTOM_ERROR
>    | ...
>   
>   test_run:cmd("setopt delimiter ''");
> @@ -895,3 +892,60 @@ check_trace(e:unpack().traceback)
>    | ---
>    | - true
>    | ...
> +
> +--
> +-- gh-4398: custom error type.
> +--
> +-- Try no code.
> +e = box.error.new({type = 'TestType', reason = 'Test reason'})
> + | ---
> + | ...
> +e:unpack()
> + | ---
> + | - code: 214
> + |   trace:
> + |   - file: '[string "e = box.error.new({type = ''TestType'', reason ..."]'
> + |     line: 1
> + |   type: TestType
> + |   custom_type: TestType
> + |   message: Test reason
> + |   base_type: CustomError
> + | ...
> +-- Try code not the same as used by default.
> +e = box.error.new({type = 'TestType', reason = 'Test reason', code = 123})
> + | ---
> + | ...
> +e:unpack()
> + | ---
> + | - code: 123
> + |   trace:
> + |   - file: '[string "e = box.error.new({type = ''TestType'', reason ..."]'
> + |     line: 1
> + |   type: TestType
> + |   custom_type: TestType
> + |   message: Test reason
> + |   base_type: CustomError
> + | ...
> +-- Try to omit message.
> +e = box.error.new({type = 'TestType'})
> + | ---
> + | ...
> +e:unpack()
> + | ---
> + | - code: 214
> + |   trace:
> + |   - file: '[string "e = box.error.new({type = ''TestType''}) "]'
> + |     line: 1
> + |   type: TestType
> + |   custom_type: TestType
> + |   message:
> + |   base_type: CustomError
> + | ...
> +-- Try too long type name.
> +e = box.error.new({type = string.rep('a', 128)})
> + | ---
> + | ...
> +#e.type
> + | ---
> + | - 63
> + | ...
> diff --git a/test/box/error.test.lua b/test/box/error.test.lua
> index 6f1271630..fe4051899 100644
> --- a/test/box/error.test.lua
> +++ b/test/box/error.test.lua
> @@ -262,3 +262,19 @@ check_trace(t3(nil, false):unpack().traceback)
>   box.error.cfg{traceback_enable = false}
>   _, e = pcall(t3, true, true)
>   check_trace(e:unpack().traceback)
> +
> +--
> +-- gh-4398: custom error type.
> +--
> +-- Try no code.
> +e = box.error.new({type = 'TestType', reason = 'Test reason'})
> +e:unpack()
> +-- Try code not the same as used by default.
> +e = box.error.new({type = 'TestType', reason = 'Test reason', code = 123})
> +e:unpack()
> +-- Try to omit message.
> +e = box.error.new({type = 'TestType'})
> +e:unpack()
> +-- Try too long type name.
> +e = box.error.new({type = string.rep('a', 128)})
> +#e.type
> 


More information about the Tarantool-patches mailing list