From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp10.mail.ru (smtp10.mail.ru [94.100.181.92]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B17AC4696C5 for ; Wed, 15 Apr 2020 12:25:59 +0300 (MSK) References: <2bb678a8aed7e1533ca2c626048080c4c1c5520b.1586505741.git.lvasiliev@tarantool.org> <5688aaca-1326-aed0-9dbc-a13e616539fe@tarantool.org> From: lvasiliev Message-ID: <40152977-6032-60e4-0396-f773d3dd395b@tarantool.org> Date: Wed, 15 Apr 2020 12:25:58 +0300 MIME-Version: 1.0 In-Reply-To: <5688aaca-1326-aed0-9dbc-a13e616539fe@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH v2 2/5] error: Add the custom error type List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org 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, ` 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 >