[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