From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (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 3A18D4696C6 for ; Tue, 14 Apr 2020 04:12:13 +0300 (MSK) References: <767fd7dee482d61cda4e2c7c9e938628bbd7f84d.1586505741.git.lvasiliev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <38ee9237-e6a2-24a5-b418-8ca05d5e16e1@tarantool.org> Date: Tue, 14 Apr 2020 03:12:11 +0200 MIME-Version: 1.0 In-Reply-To: <767fd7dee482d61cda4e2c7c9e938628bbd7f84d.1586505741.git.lvasiliev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 5/5] iproto: Update error MsgPack encoding List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leonid Vasiliev Cc: tarantool-patches@dev.tarantool.org 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. I didn't have time to review it properly yet. Just some first comments below. See 12 of them below. On 10/04/2020 10:10, Leonid Vasiliev wrote: > New MsgPack encode format for error (MP_ERROR) has been added. > If the extended error format is enabled using iproto session settings > MP_ERROR type will be used for transferring error through network, > MP_STR was used before. 1. For such a big feature and not a single word of documentation. > Needed for #4398 2. Shouldn't that be 'Closes'? > diff --git a/src/box/error.cc b/src/box/error.cc > index 8179e52..f2e60c1 100644 > --- a/src/box/error.cc > +++ b/src/box/error.cc > @@ -253,6 +253,13 @@ XlogGapError::XlogGapError(const char *file, unsigned line, > (long long) vclock_sum(to), s_to ? s_to : ""); > } > > +XlogGapError::XlogGapError(const char *file, unsigned line, > + const char *msg) > + : XlogError(&type_XlogGapError, file, line) > +{ > + error_format_msg(this, "%s", msg); > +} > + > struct error * > BuildXlogGapError(const char *file, unsigned line, > const struct vclock *from, const struct vclock *to) > @@ -264,6 +271,16 @@ BuildXlogGapError(const char *file, unsigned line, > } > } > > +struct error * > +ReBuildXlogGapError(const char *file, unsigned line, const char *msg) 3. Wtf is Rebuild? Why normal Build does not work? If that is because of vclock, then just change vclock to const char * in the original Build in a separate preparatory commit. > +{ > + try { > + return new XlogGapError(file, line, msg); > + } catch (OutOfMemory *e) { > + return e; > + } > +} > + > struct rlist on_access_denied = RLIST_HEAD_INITIALIZER(on_access_denied); > > diff --git a/src/box/lua/call.c b/src/box/lua/call.c > index 5d3579e..6d1d247 100644 > --- a/src/box/lua/call.c > +++ b/src/box/lua/call.c > @@ -174,7 +175,7 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg, > */ > for (int i = 1; i <= nrets; ++i) { > struct luaL_field field; > - if (luaL_tofield(L, cfg, i, &field) < 0) > + if (luaL_tofield(L, cfg, NULL, i, &field) < 0) 4. I think addition of that parameter should be a part of the previous commit. So as not to pollute this commit with such minor changes. In the previous commit it can be added and ignored inside luamp_encode_r(). But it will absorb all these not interesting changes. > return luaT_error(L); > struct tuple *tuple; > if (field.type == MP_EXT && > @@ -379,6 +380,7 @@ execute_lua_eval(lua_State *L) > struct encode_lua_ctx { > struct port_lua *port; > struct mpstream *stream; > + struct luaL_serializer_ctx *serializer_ctx; 5. Not. A. Single. Word. Please, start writing descriptive comments. > }; > > static int > diff --git a/src/box/lua/mp_error.cc b/src/box/lua/mp_error.cc > new file mode 100644 > index 0000000..f99da0f > --- /dev/null > +++ b/src/box/lua/mp_error.cc > @@ -0,0 +1,454 @@ > +/* > + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file. > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * 1. Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the > + * following disclaimer. > + * > + * 2. Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED > + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL > + * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, > + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR > + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF > + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF > + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > +#include "box/lua/mp_error.h" > +#include "box/error.h" > +#include "mpstream.h" > +#include "msgpuck.h" > +#include "mp_extension_types.h" > + > +enum mp_error_details { > + MP_ERROR_DET_TYPE, > + MP_ERROR_DET_FILE, > + MP_ERROR_DET_LINE, > + MP_ERROR_DET_REASON, > + MP_ERROR_DET_ERRNO, > + MP_ERROR_DET_CODE, > + MP_ERROR_DET_BACKTRACE, > + MP_ERROR_DET_CUSTOM_TYPE, > + MP_ERROR_DET_AD_OBJ_TYPE, > + MP_ERROR_DET_AD_OBJ_NAME, > + MP_ERROR_DET_AD_ACCESS_TYPE > +}; > + > +enum mp_error_types { > + MP_ERROR_TYPE_UNKNOWN, > + MP_ERROR_TYPE_CLIENT, > + MP_ERROR_TYPE_CUSTOM, > + MP_ERROR_TYPE_ACCESS_DENIED, > + MP_ERROR_TYPE_XLOG, > + MP_ERROR_TYPE_XLOG_GAP, > + MP_ERROR_TyPE_SYSTEM, 6. I assume you use some kind of smart editor with autocompletion so as you missed this typo: TyPE -> TYPE 7. Why are these values in a enum? I thought we decided to use string types? > + MP_ERROR_TyPE_SOCKET, > + MP_ERROR_TyPE_OOM, > + MP_ERROR_TyPE_TIMED_OUT, > + MP_ERROR_TyPE_CHANNEL_IS_CLOSED, > + MP_ERROR_TyPE_FIBER_IS_CANCELLED, > + MP_ERROR_TyPE_LUAJIT, > + MP_ERROR_TyPE_ILLEGAL_PARAMS, > + MP_ERROR_TyPE_COLLATION, > + MP_ERROR_TyPE_SWIM, > + MP_ERROR_TyPE_CRYPTO > +}; > + > +struct mp_error { > + uint32_t error_code; > + uint8_t error_type; 8. If you assign enum to a variable, that variable should have type of that enum, usually. > + uint32_t line; > + uint32_t saved_errno; > + char *file; > + char *backtrace; > + char *reason; > + char *custom_type; > + char *ad_obj_type; > + char *ad_obj_name; > + char *ad_access_type; > +}; > diff --git a/src/box/lua/mp_error.h b/src/box/lua/mp_error.h > new file mode 100644 > index 0000000..9eab213 > --- /dev/null > +++ b/src/box/lua/mp_error.h > @@ -0,0 +1,49 @@ > +#pragma once > +/* > + * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file. > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * 1. Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the > + * following disclaimer. > + * > + * 2. Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED > + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL > + * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, > + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR > + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF > + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF > + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > + > +#if defined(__cplusplus) > +extern "C" { > +#endif /* defined(__cplusplus) */ > + > +#include "stdint.h" 9. Standard and other external headers should be in <>, not "". > + > +struct mpstream; > + > +void > +error_to_mpstream(struct error *error, struct mpstream *stream); > + > +struct error * > +error_unpack(const char **data, uint32_t len); > + > +#if defined(__cplusplus) > +} /* extern "C" */ > +#endif /* defined(__cplusplus) */ > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua > index c8f76b0..d3997af 100644 > --- a/src/box/lua/net_box.lua > +++ b/src/box/lua/net_box.lua > @@ -1050,7 +1050,7 @@ local function new_sm(host, port, opts, connection, greeting) > > -- Set extended error format for session. > if opts.error_extended then > - local ext_err_supported = version_at_least(remote.peer_version_id, 2, 4, 1) > + local ext_err_supported = version_at_least(remote.peer_version_id, 2, 4, 0) 10. Why? > if not ext_err_supported then > box.error(box.error.PROC_LUA, > "Server doesn't support extended error format") > diff --git a/src/lib/core/mp_extension_types.h b/src/lib/core/mp_extension_types.h > index bc9873f..a2a5079 100644 > --- a/src/lib/core/mp_extension_types.h > +++ b/src/lib/core/mp_extension_types.h > @@ -40,8 +40,9 @@ > * You may assign values in range [0, 127] > */ > enum mp_extension_type { > - MP_UNKNOWN_EXTENSION = 0, > - MP_DECIMAL = 1, > + MP_UNKNOWN_EXTENSION = 0, > + MP_DECIMAL = 1, > + MP_ERROR = 2 > }; 11. Please, rebase before applying any review fixes. This code is changed in the master. As well as session settings. > #endif > diff --git a/src/lua/error.c b/src/lua/error.c > index cd6ab54..109f947 100644 > --- a/src/lua/error.c > +++ b/src/lua/error.c > @@ -34,8 +34,6 @@ > #include > #include "utils.h" > > -static int CTID_CONST_STRUCT_ERROR_REF = 0; > - > struct error * > luaL_iserror(struct lua_State *L, int narg) > { > diff --git a/src/lua/error.h b/src/lua/error.h > index 16cdaf7..4e4dc04 100644 > --- a/src/lua/error.h > +++ b/src/lua/error.h > @@ -37,7 +37,6 @@ > extern "C" { > #endif /* defined(__cplusplus) */ > > - 12. Please, remove all unnecessary diff.