From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 3AFD24696C3 for ; Mon, 6 Apr 2020 01:14:37 +0300 (MSK) References: <56090ffde2ff472e2add51df13ebac93f5ee66b5.1585053743.git.lvasiliev@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Mon, 6 Apr 2020 00:14:33 +0200 MIME-Version: 1.0 In-Reply-To: <56090ffde2ff472e2add51df13ebac93f5ee66b5.1585053743.git.lvasiliev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 2/6] error: Add the custom error type List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leonid Vasiliev , alexander.turenko@tarantool.org Cc: tarantool-patches@dev.tarantool.org Thanks for the patch! See 6 comments below. On 24/03/2020 13:46, 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 > 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. > > Example: > > err_custom = box.error.new(box.error.CUSTOM_ERROR, "My Custom Type", "Reason") > Now: > err_custom.type == "CustomError" > err_custom.custom_type == "My Custom Type" 1. Regarding types we discussed that verbally and then I wrote it in the ticket, that better return custom type in err.type. And return the original type in a new field. To simplify cascade error checking, when one type is compared to many possible types to make appropriate actions. Just for record. > err_custom.message == "User custom error: Reason" 2. The whole point of having custom errors - be fully customizable. I don't think it is a good idea to seal this prefix into every user defined error. After all, usually errors are created not for end users, but for systems built on top of tarantool. Hence it looks odd to even mention 'User'. 3. In the doc request you didn't say a word about custom error type length limit. > diff --git a/src/box/error.cc b/src/box/error.cc > index 47dce33..25e7eff 100644 > --- a/src/box/error.cc > +++ b/src/box/error.cc > @@ -253,3 +274,38 @@ 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); > + > +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, 63); > + m_custom_type[63] = '\0'; 4. Please, don't do that. Never use constants the same constant in several places, and depending on other constants. Use sizeof(m_custom_type) - 1. > + } else { > + m_custom_type[0] = '\0'; > + } > +} > diff --git a/src/box/error.h b/src/box/error.h > index b8c7cf7..3e0beb8 100644 > --- a/src/box/error.h > +++ b/src/box/error.h > @@ -70,6 +73,14 @@ const char * > box_error_type(const box_error_t *error); > > /** > + * Return the error custom type, > + * \param error > + * \return pointer to custom error type. On error, return NULL > + */ > +const char * > +box_custom_error_type(const box_error_t *e); > + 5. Please, move this out of '\cond public'. I don't think we are ready to release custom errors as part of the public C API. The same for box_custom_error_set(). > +/** > * Return IPROTO error code > * \param error error > * \return enum box_error_code > diff --git a/src/lua/error.lua b/src/lua/error.lua > index 765ce73..26abec8 100644 > --- a/src/lua/error.lua > +++ b/src/lua/error.lua > @@ -148,11 +152,16 @@ local function error_serialize(err) > return error_message(err) > end > > +local function error_is_custom(err) > + return ffi.string(err._type.name) == 'CustomError' > +end > + > local error_methods = { > - ["unpack"] = error_unpack; > - ["raise"] = error_raise; > - ["match"] = error_match; -- Tarantool 1.6 backward compatibility > - ["__serialize"] = error_serialize; > + ["unpack"] = error_unpack, > + ["raise"] = error_raise, > + ["match"] = error_match, -- Tarantool 1.6 backward compatibility > + ["is_custom"] = error_is_custom, > + ["__serialize"] = error_serialize 6. The same as in the previous patch - try not to change existing code when possible. Here it is possible. And below too. > } > > local function error_index(err, key) > @@ -181,11 +190,11 @@ local function error_concat(lhs, rhs) > end > > local error_mt = { > - __index = error_index; > - __tostring = error_message; > - __concat = error_concat; > + __index = error_index, > + __tostring = error_message, > + __concat = error_concat > }; > > -ffi.metatype('struct error', error_mt); > +ffi.metatype('struct error', error_mt) > > return error