Re: [Tarantool-patches] [PATCH v2 5/5] iproto: Update error MsgPack encoding

[lvasiliev <lvasiliev@tarantool.org>]

Hi! Thanks you for the review.

On 14.04.2020 4:12, 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.
> 
> 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.
fixed in new patch
> 
>> Needed for #4398
> 
> 2. Shouldn't that be 'Closes'?
fixed in new patch
> 
>> 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.
fixed in new patch
> 
>> +{
>> +	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.
fixed in new patch
> 
>>   				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.
Ok. I will try.
> 
>>   };
>>   
>>   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 <COPYRIGHT HOLDER> ``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
>> + * <COPYRIGHT HOLDER> 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
Fixed.
> 
> 7. Why are these values in a enum? I thought we
> decided to use string types?
Now we compare a string with error type when encode and use number 
further. If we encode error type as string we still have to compare the 
strings when decoding to create the error of the desired type. But 
number takes up less space versus string  when transmitting over a 
network and makes it possible to use a switch when create an error after 
decoding (which looks much nicer).
> 
>> +	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.
fixed
> 
>> +	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 <COPYRIGHT HOLDER> ``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
>> + * <COPYRIGHT HOLDER> 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 "".
fixed
> 
>> +
>> +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?
It's mistake. Now deleted.
> 
>>           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 <fiber.h>
>>   #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.
fixed

I have a some additional question:
"Do I understand correctly that MP_ERROR should not be added to 
field_def.h/field_def.c or I am wrong?"
>