[Tarantool-patches] [PATCH V4 6/6] error: add error MsgPack encoding
lvasiliev
lvasiliev at tarantool.org
Sat Apr 18 20:46:07 MSK 2020
Hi! Thanks for the review.
On 17.04.2020 3:58, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> One major note: mpstream_iproto_encode_error() is not
> updated at all. So the marshaling is still not complete.
>
I tried. If we add mp_ext to IPROTO_ERROR, we’ll break old clients.
> Generally the patch is relatively fine. Still some
> comments exist, but almost no major ones, except the
> one above.
>
> On 16/04/2020 19:38, Leonid Vasiliev wrote:
>> Closes #4398
>>
>> @TarantoolBot document
>> Title: msgpack format for MP_ERROR
>>
>> For sent an error over IProto MP_ERROR(0x03) subtype
>> has been added to MsgPack MP_EXT.
>> Now, if session setting 'error_marshaling_enabled'
>> is true error will be encoded as:
>> ```
>> +--------+----------+========+
>> | MP_EXT | MP_ERROR | MP_MAP |
>> +--------+----------+========+
>> ```
>> At the map passes all necessary parameters (depending
>> from the error type) to create a copy of the error
>> on the client side.
>> The necessary fields:
>> - Error type
>> - Trace(file name and line)
>
> Since we decided to drop traceback for now, we should omit
> this too, until it is designed properly. Better not to add them
> now and add later with a thorough design, than add in a hurry
> now, and then not be able to change it.
>
>> - Error message
>> - Errno
>
> You need to document the exact numbers and types of every
> of these fields.
>
Added
>> Additional fields:
>> - ClientError code
>> - Traceback (now only Lua traceback)
>
> Traceback is not here anymore.
>
Discussed
>> - Custom type
>> - Access Denied object type
>> - Access Denied object name
>> - Access Denied access type
>
> Your error structure is flat, and is based entirely on number
> keys. Probably a better option would be to store all type-specific
> optional attributes with MP_STR keys and in a separate map.
>
> Separate map would make it clear what options are type-specific,
> and would simplify decoding on the client a bit.
>
> Talking of numbers, there is the problem of introduction of new
> attributes and of compatibility again. About compatibility all
> my arguments are the same as for error types. If you will use
> string keys for type-specific and potentially extendible fields,
> it will simplify support in connectors. About problem of new
> attributes introduction see example below:
>
> Assume, we have AccessDeniedError, XlogGapError, and a SwimError.
> Now assume they have some attributes encoded with these keys:
>
> MP_ERROR_ATTR_ACCESS_DENIED_OBJ_TYPE = 0,
> MP_ERROR_ATTR_ACCESS_DENIED_OBJ_NAME = 1,
> MP_ERROR_ATTR_ACCESS_DENIED_ACC_TYPE = 2,
> MP_ERROR_ATTR_XLOG_GAP_VCLOCK_START = 3,
> MP_ERROR_ATTR_XLOG_GAP_VCLOCK_END = 4,
> MP_ERROR_ATTR_SWIM_NODE_DEAD_UUID = 5,
>
> And now we want to add a new attribute to the AccessDeniedError
> object. We will need to make it having key 6. That separates "access
> denied" attributes from each other. After a few more mutations
> this will be a mess.
>
> My proposal is this:
>
> MP_ERROR: {
> MP_ERROR_TYPE: ...,
> MP_ERROR_CUSTOM_TYPE: ...,
> MP_ERROR_TRACE: ...,
> MP_ERROR_MESSAGE: ...,
> MP_ERRNO: ...,
> MP_ERROR_CODE: ...,
> MP_ERROR_FIELDS: {
> <MP_STR>: ...,
> <MP_STR>: ...,
> ...
> }
> }
>
Implemented
> As you can see, I added MP_ERROR_FIELDS, and all type-specific
> fields are present here as string key + some arbitrary values.
> Easy to add new type-specific attributes.
>
> But this is arguable. I rely on my recent experience with working
> with Google Cloud where they return all information as 'string key' +
> 'string value' except HTTP codes, and it appeared to be easy to
> handle. I advice you get a second opinion from Alexander T. and
> probably from Kostja Nazarov. Maybe from Mons, he knows how to do
> right lots of things.
>
> Keep in mind this is not perf critical part, so we need to be on
> the best compatibility side here.
>
> See 5 comments below, and a commit on top of the branch. Looks like
> I broke something in my commit so that the new test does not pass.
> Could you please find a reason?
>
>> diff --git a/src/box/lua/init.c b/src/box/lua/init.c
>> index 63e8b82..de09389 100644
>> --- a/src/box/lua/init.c
>> +++ b/src/box/lua/init.c
>> @@ -386,6 +391,54 @@ static const struct luaL_Reg boxlib_backup[] = {
>> {NULL, NULL}
>> };
>>
>> +/**
>> + * A MsgPack extensions handler that supports tuples and errors encode.
>> + */
>> +static enum mp_type
>> +luamp_encode_extension_box(struct lua_State *L, int idx,
>> + struct mpstream *stream)
>
> 1. Could you please move this function to a separate commit?
> I see you do some tuple-related things here. Would be better to
> make it beforehand to simplify this commit.
>
With a separate commit, we move one function from file A to file B for
modify it in the next commit. Sounds weird.
>> +{
>> + struct tuple *tuple = luaT_istuple(L, idx);
>> + if (tuple != NULL) {
>> + tuple_to_mpstream(tuple, stream);
>> + return MP_ARRAY;
>> + }
>> +
>> + if (current_session()->meta.serializer_opts.error_marshaling_enabled) {
>> + struct error *err = luaL_iserror(L, idx);
>> + if (err != NULL)
>> + error_to_mpstream(err, stream);
>> + }
>> +
>> + return MP_EXT;
>> +}
>> diff --git a/src/box/lua/mp_error.cc b/src/box/lua/mp_error.cc
>> new file mode 100644
>> index 0000000..2e15206
>> --- /dev/null
>> +++ b/src/box/lua/mp_error.cc
>> @@ -0,0 +1,373 @@
>> +static struct error *
>> +build_error(struct mp_error *mp_error)
>> +{
>> + /*
>> + * For create an error the "raw" constructor using
>> + * because OOM error must be throwed in OOM case.
>> + * Bulders returns a pointer to the static OOM error
>> + * in OOM case.
>
> 2. Why do you want an exception out? That function is called from
> error_unpack(), which is called from C. You can't throw here. The
> function should be wrapped into a try-catch block. Am I missing
> something?
>
> In case there was OOM, should we return the OOM, or return NULL?
Null and set OOM to diag.
>
>> + */
>> + struct error *err = NULL;
>> +
>> + if (strcmp(mp_error->error_type, "ClientError") == 0) {
>> + ClientError *e = new ClientError(mp_error->file, mp_error->line,
>> + ER_UNKNOWN);
>> + e->m_errcode = mp_error->error_code;
>> + err = e;
>> + } else if (strcmp(mp_error->error_type, "CustomError") == 0) {
>> + err = new CustomError(mp_error->file, mp_error->line,
>> + mp_error->custom_type);
>> + } else if (strcmp(mp_error->error_type, "AccessDeniedError") == 0) {
>> + err = new AccessDeniedError(mp_error->file, mp_error->line,
>> + mp_error->ad_access_type,
>> + mp_error->ad_obj_type,
>> + mp_error->ad_obj_name, "");
>> + } else if (strcmp(mp_error->error_type, "XlogError") == 0) {
>> + err = new XlogError(&type_XlogError, mp_error->file,
>> + mp_error->line);
>> + error_format_msg(err, "%s", mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "XlogGapError") == 0) {
>> + err = new XlogGapError(mp_error->file, mp_error->line,
>> + mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "SystemError") == 0) {
>> + err = new SystemError(mp_error->file, mp_error->line,
>> + "%s", mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "SocketError") == 0) {
>> + err = new SocketError(mp_error->file, mp_error->line, "", "");
>> + error_format_msg(err, "%s", mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "OutOfMemory") == 0) {
>> + err = new OutOfMemory(mp_error->file, mp_error->line,
>> + 0, "", "");
>> + error_format_msg(err, "%s", mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "TimedOut") == 0) {
>> + err = new TimedOut(mp_error->file, mp_error->line);
>> + } else if (strcmp(mp_error->error_type, "ChannelIsClosed") == 0) {
>> + err = new ChannelIsClosed(mp_error->file, mp_error->line);
>> + } else if (strcmp(mp_error->error_type, "FiberIsCancelled") == 0) {
>> + err = new FiberIsCancelled(mp_error->file, mp_error->line);
>> + } else if (strcmp(mp_error->error_type, "LuajitError") == 0) {
>> + err = new LuajitError(mp_error->file, mp_error->line,
>> + mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "IllegalParams") == 0) {
>> + err = new IllegalParams(mp_error->file, mp_error->line,
>> + "%s", mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "CollationError") == 0) {
>> + err = new CollationError(mp_error->file, mp_error->line,
>> + "%s", mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "SwimError") == 0) {
>> + err = new SwimError(mp_error->file, mp_error->line,
>> + "%s", mp_error->reason);
>> + } else if (strcmp(mp_error->error_type, "CryptoError") == 0) {
>> + err = new CryptoError(mp_error->file, mp_error->line,
>> + "%s", mp_error->reason);
>> + }
>> +
>> + if (err) {
>> + err->saved_errno = mp_error->saved_errno;
>> + error_format_msg(err, "%s", mp_error->reason);
>> + }
>> +
>> + return err;
>> +}
>> +
>> +struct error *
>> +error_unpack(const char **data, uint32_t len)
>> +{
>> + const char *end = *data + len;
>> + if (mp_typeof(**data) != MP_MAP) {
>> + diag_set(ClientError, ER_INVALID_MSGPACK,
>> + "Invalid MP_ERROR format");
>> + return NULL;
>> + }
>> +
>> + struct mp_error mp_err;
>> + mp_error_init(&mp_err);
>> +
>> + uint32_t map_size = mp_decode_map(data);
>> +
>> + struct error *err = NULL;
>> + for (uint32_t i = 0; i < map_size; ++i) {
>> + if (mp_typeof(**data) != MP_UINT) {
>> + diag_set(ClientError, ER_INVALID_MSGPACK,
>> + "Invalid MP_ERROR MsgPack format");
>> + return NULL;
>> + }
>> +
>> + uint8_t key = mp_decode_uint(data);
>> + const char *str;
>> + uint32_t str_len;
>> + switch(key) {
>> + case MP_ERROR_DET_TYPE:
>> + if (mp_typeof(**data) != MP_STR)
>> + goto error;
>
> 3. That is actually an open question whether we should validate
> MessagePack here. Currently none of MP_EXT decoders do that in
> luamp_decode. Normal types also skip validation.
>
> I am not saying it should not be fixed, but probably everywhere
> at once, in a centralized way.
>
>> + str = mp_decode_str(data, &str_len);
>> + mp_err.error_type = strndup(str, str_len);
>> + break;
>> + case MP_ERROR_DET_FILE:
>> + if (mp_typeof(**data) != MP_STR)
>> + goto error;
>> + str = mp_decode_str(data, &str_len);
>> + mp_err.file = strndup(str, str_len);
>> + break;
>> 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) */
>>
>> -
>
> 4. Please, omit unnecessary diff.
>
>> /** \cond public */
>> struct error;
>>
>> diff --git a/test/box-tap/extended_error.test.lua b/test/box-tap/extended_error.test.lua
>> new file mode 100755
>> index 0000000..eb86bdc
>> --- /dev/null
>> +++ b/test/box-tap/extended_error.test.lua
>
> 5. Not sure how does it work now, taking into account that you
> didn't update iproto part.
>
> Please, add tests which use msgpack and msgpackffi. Without
> network. Marshaling should affect it too.
>
> Also add tests when happens when marshaling is disabled, and
> yet a session tries to decode MP_ERROR. That may happen, if
> it obtains the MessagePack generated in another session, or
> if you turn marshaling on and off for one session.
Tests inprogress.
>
> ====================
>
> diff --git a/src/box/lua/init.c b/src/box/lua/init.c
> index de09389ef..5e04987fe 100644
> --- a/src/box/lua/init.c
> +++ b/src/box/lua/init.c
> @@ -423,7 +423,7 @@ luamp_decode_extension_box(struct lua_State *L, const char **data)
> int8_t ext_type;
> uint32_t len = mp_decode_extl(data, &ext_type);
>
> - if(ext_type != MP_ERROR) {
> + if (ext_type != MP_ERROR) {
> luaL_error(L, "Unsuported MsgPack extension type: %u",
> ext_type);
> return;
> diff --git a/src/box/lua/mp_error.cc b/src/box/lua/mp_error.cc
> index 2e1520621..38a6f6e98 100644
> --- a/src/box/lua/mp_error.cc
> +++ b/src/box/lua/mp_error.cc
> @@ -33,21 +33,22 @@
> #include "mpstream/mpstream.h"
> #include "msgpuck.h"
> #include "mp_extension_types.h"
> +#include "fiber.h"
>
> /**
> * Keys used for encode/decode MP_ERROR.
> */
> -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_CUSTOM_TYPE,
> - MP_ERROR_DET_AD_OBJ_TYPE,
> - MP_ERROR_DET_AD_OBJ_NAME,
> - MP_ERROR_DET_AD_ACCESS_TYPE
> +enum {
> + MP_ERROR_TYPE,
> + MP_ERROR_FILE,
> + MP_ERROR_LINE,
> + MP_ERROR_REASON,
> + MP_ERROR_ERRNO,
> + MP_ERROR_CODE,
> + MP_ERROR_CUSTOM_TYPE,
> + MP_ERROR_AD_OBJ_TYPE,
> + MP_ERROR_AD_OBJ_NAME,
> + MP_ERROR_AD_ACCESS_TYPE
> };
>
> /**
> @@ -58,40 +59,19 @@ struct mp_error {
> uint32_t error_code;
> uint32_t line;
> uint32_t saved_errno;
> - char *error_type;
> - char *file;
> - char *reason;
> - char *custom_type;
> - char *ad_obj_type;
> - char *ad_obj_name;
> - char *ad_access_type;
> + const char *error_type;
> + const char *file;
> + const char *reason;
> + const char *custom_type;
> + const char *ad_obj_type;
> + const char *ad_obj_name;
> + const char *ad_access_type;
> };
>
> static void
> -mp_error_init(struct mp_error *mp_error)
> +mp_error_create(struct mp_error *mp_error)
> {
> - mp_error->error_code = 0;
> - mp_error->line = 0;
> - mp_error->saved_errno = 0;
> - mp_error->error_type = NULL;
> - mp_error->file = NULL;
> - mp_error->reason = NULL;
> - mp_error->custom_type = NULL;
> - mp_error->ad_obj_type = NULL;
> - mp_error->ad_obj_name = NULL;
> - mp_error->ad_access_type = NULL;
> -}
> -
> -static void
> -mp_error_cleanup(struct mp_error *mp_error)
> -{
> - free(mp_error->error_type);
> - free(mp_error->file);
> - free(mp_error->reason);
> - free(mp_error->custom_type);
> - free(mp_error->ad_obj_type);
> - free(mp_error->ad_obj_name);
> - free(mp_error->ad_access_type);
> + memset(mp_error, 0, sizeof(*mp_error));
> }
>
> void
> @@ -118,25 +98,25 @@ error_to_mpstream(struct error *error, struct mpstream *stream)
> uint32_t details_num = 5;
> uint32_t data_size = 0;
>
> - data_size += mp_sizeof_uint(MP_ERROR_DET_TYPE);
> + data_size += mp_sizeof_uint(MP_ERROR_TYPE);
> data_size += mp_sizeof_str(strlen(error->type->name));
> - data_size += mp_sizeof_uint(MP_ERROR_DET_LINE);
> + data_size += mp_sizeof_uint(MP_ERROR_LINE);
> data_size += mp_sizeof_uint(error->line);
> - data_size += mp_sizeof_uint(MP_ERROR_DET_FILE);
> + data_size += mp_sizeof_uint(MP_ERROR_FILE);
> data_size += mp_sizeof_str(strlen(error->file));
> - data_size += mp_sizeof_uint(MP_ERROR_DET_REASON);
> + data_size += mp_sizeof_uint(MP_ERROR_REASON);
> data_size += mp_sizeof_str(strlen(error->errmsg));
> - data_size += mp_sizeof_uint(MP_ERROR_DET_ERRNO);
> + data_size += mp_sizeof_uint(MP_ERROR_ERRNO);
> data_size += mp_sizeof_uint(error->saved_errno);
>
> if (is_client || is_access_denied || is_custom) {
> ++details_num;
> errcode = box_error_code(error);
> - data_size += mp_sizeof_uint(MP_ERROR_DET_CODE);
> + data_size += mp_sizeof_uint(MP_ERROR_CODE);
> data_size += mp_sizeof_uint(errcode);
> if (is_custom) {
> ++details_num;
> - data_size += mp_sizeof_uint(MP_ERROR_DET_CUSTOM_TYPE);
> + data_size += mp_sizeof_uint(MP_ERROR_CUSTOM_TYPE);
> custom_type = box_error_custom_type(error);
> data_size += mp_sizeof_str(strlen(custom_type));
> } else if (is_access_denied) {
> @@ -146,11 +126,11 @@ error_to_mpstream(struct error *error, struct mpstream *stream)
> ad_obj_type = ad_err->object_type();
> ad_obj_name = ad_err->object_name();
> ad_access_type = ad_err->access_type();
> - data_size += mp_sizeof_uint(MP_ERROR_DET_AD_OBJ_TYPE);
> + data_size += mp_sizeof_uint(MP_ERROR_AD_OBJ_TYPE);
> data_size += mp_sizeof_str(strlen(ad_obj_type));
> - data_size += mp_sizeof_uint(MP_ERROR_DET_AD_OBJ_NAME);
> + data_size += mp_sizeof_uint(MP_ERROR_AD_OBJ_NAME);
> data_size += mp_sizeof_str(strlen(ad_obj_name));
> - data_size += mp_sizeof_uint(MP_ERROR_DET_AD_ACCESS_TYPE);
> + data_size += mp_sizeof_uint(MP_ERROR_AD_ACCESS_TYPE);
> data_size += mp_sizeof_str(strlen(ad_access_type));
> }
> }
> @@ -162,33 +142,33 @@ error_to_mpstream(struct error *error, struct mpstream *stream)
> char *data = ptr;
> data = mp_encode_extl(data, MP_ERROR, data_size);
> data = mp_encode_map(data, details_num);
> - data = mp_encode_uint(data, MP_ERROR_DET_TYPE);
> + data = mp_encode_uint(data, MP_ERROR_TYPE);
> data = mp_encode_str(data, error->type->name,
> strlen(error->type->name));
> - data = mp_encode_uint(data, MP_ERROR_DET_LINE);
> + data = mp_encode_uint(data, MP_ERROR_LINE);
> data = mp_encode_uint(data, error->line);
> - data = mp_encode_uint(data, MP_ERROR_DET_FILE);
> + data = mp_encode_uint(data, MP_ERROR_FILE);
> data = mp_encode_str(data, error->file, strlen(error->file));
> - data = mp_encode_uint(data, MP_ERROR_DET_REASON);
> + data = mp_encode_uint(data, MP_ERROR_REASON);
> data = mp_encode_str(data, error->errmsg, strlen(error->errmsg));
> - data = mp_encode_uint(data, MP_ERROR_DET_ERRNO);
> + data = mp_encode_uint(data, MP_ERROR_ERRNO);
> data = mp_encode_uint(data, error->saved_errno);
>
> if (is_client || is_access_denied || is_custom) {
> - data = mp_encode_uint(data, MP_ERROR_DET_CODE);
> + data = mp_encode_uint(data, MP_ERROR_CODE);
> data = mp_encode_uint(data, errcode);
> if (is_custom) {
> - data = mp_encode_uint(data, MP_ERROR_DET_CUSTOM_TYPE);
> + data = mp_encode_uint(data, MP_ERROR_CUSTOM_TYPE);
> data = mp_encode_str(data, custom_type,
> strlen(custom_type));
> } else if (is_access_denied) {
> - data = mp_encode_uint(data, MP_ERROR_DET_AD_OBJ_TYPE);
> + data = mp_encode_uint(data, MP_ERROR_AD_OBJ_TYPE);
> data = mp_encode_str(data, ad_obj_type,
> strlen(ad_obj_type));
> - data = mp_encode_uint(data, MP_ERROR_DET_AD_OBJ_NAME);
> + data = mp_encode_uint(data, MP_ERROR_AD_OBJ_NAME);
> data = mp_encode_str(data, ad_obj_name,
> strlen(ad_obj_name));
> - data = mp_encode_uint(data, MP_ERROR_DET_AD_ACCESS_TYPE);
> + data = mp_encode_uint(data, MP_ERROR_AD_ACCESS_TYPE);
> data = mp_encode_str(data, ad_access_type,
> strlen(ad_access_type));
> }
> @@ -202,9 +182,9 @@ static struct error *
> build_error(struct mp_error *mp_error)
> {
> /*
> - * For create an error the "raw" constructor using
> - * because OOM error must be throwed in OOM case.
> - * Bulders returns a pointer to the static OOM error
> + * To create an error the "raw" constructor using
> + * because OOM error must be thrown in OOM case.
> + * Builders returns a pointer to the static OOM error
> * in OOM case.
> */
> struct error *err = NULL;
> @@ -270,8 +250,21 @@ build_error(struct mp_error *mp_error)
> return err;
> }
>
> +static inline const char *
> +region_strdup(struct region *region, const char *str, uint32_t len)
> +{
> + char *res = (char *)region_alloc(region, len + 1);
> + if (res == NULL) {
> + diag_set(OutOfMemory, len + 1, "region_alloc", "res");
> + return NULL;
> + }
> + memcpy(res, str, len);
> + res[len] = 0;
> + return res;
> +}
> +
> struct error *
> -error_unpack(const char **data, uint32_t len)
> +error_unpack_xc(const char **data, uint32_t len)
> {
> const char *end = *data + len;
> if (mp_typeof(**data) != MP_MAP) {
> @@ -281,8 +274,9 @@ error_unpack(const char **data, uint32_t len)
> }
>
> struct mp_error mp_err;
> - mp_error_init(&mp_err);
> -
> + mp_error_create(&mp_err);
> + struct region *region = &fiber()->gc;
> + uint32_t region_svp = region_used(region);
> uint32_t map_size = mp_decode_map(data);
>
> struct error *err = NULL;
> @@ -290,69 +284,87 @@ error_unpack(const char **data, uint32_t len)
> if (mp_typeof(**data) != MP_UINT) {
> diag_set(ClientError, ER_INVALID_MSGPACK,
> "Invalid MP_ERROR MsgPack format");
> - return NULL;
> + goto finish;
> }
>
> - uint8_t key = mp_decode_uint(data);
> + uint64_t key = mp_decode_uint(data);
> const char *str;
> uint32_t str_len;
> switch(key) {
> - case MP_ERROR_DET_TYPE:
> + case MP_ERROR_TYPE:
> if (mp_typeof(**data) != MP_STR)
> goto error;
> str = mp_decode_str(data, &str_len);
> - mp_err.error_type = strndup(str, str_len);
> + mp_err.error_type = region_strdup(region, str, str_len);
> + if (mp_err.error_type == NULL)
> + goto finish;
> break;
> - case MP_ERROR_DET_FILE:
> + case MP_ERROR_FILE:
> if (mp_typeof(**data) != MP_STR)
> goto error;
> str = mp_decode_str(data, &str_len);
> - mp_err.file = strndup(str, str_len);
> + mp_err.file = region_strdup(region, str, str_len);
> + if (mp_err.file == NULL)
> + goto finish;
> break;
> - case MP_ERROR_DET_LINE:
> + case MP_ERROR_LINE:
> if (mp_typeof(**data) != MP_UINT)
> goto error;
> mp_err.line = mp_decode_uint(data);
> break;
> - case MP_ERROR_DET_REASON:
> + case MP_ERROR_REASON:
> if (mp_typeof(**data) != MP_STR)
> goto error;
> str = mp_decode_str(data, &str_len);
> - mp_err.reason = strndup(str, str_len);
> + mp_err.reason = region_strdup(region, str, str_len);
> + if (mp_err.reason == NULL)
> + goto finish;
> break;
> - case MP_ERROR_DET_ERRNO:
> + case MP_ERROR_ERRNO:
> if (mp_typeof(**data) != MP_UINT)
> goto error;
> mp_err.saved_errno = mp_decode_uint(data);
> break;
> - case MP_ERROR_DET_CODE:
> + case MP_ERROR_CODE:
> if (mp_typeof(**data) != MP_UINT)
> goto error;
> mp_err.error_code = mp_decode_uint(data);
> break;
> - case MP_ERROR_DET_CUSTOM_TYPE:
> + case MP_ERROR_CUSTOM_TYPE:
> if (mp_typeof(**data) != MP_STR)
> goto error;
> str = mp_decode_str(data, &str_len);
> - mp_err.custom_type = strndup(str, str_len);
> + mp_err.custom_type = region_strdup(region, str,
> + str_len);
> + if (mp_err.custom_type == NULL)
> + goto finish;
> break;
> - case MP_ERROR_DET_AD_OBJ_TYPE:
> + case MP_ERROR_AD_OBJ_TYPE:
> if (mp_typeof(**data) != MP_STR)
> goto error;
> str = mp_decode_str(data, &str_len);
> - mp_err.ad_obj_type = strndup(str, str_len);
> + mp_err.ad_obj_type = region_strdup(region, str,
> + str_len);
> + if (mp_err.ad_obj_type == NULL)
> + goto finish;
> break;
> - case MP_ERROR_DET_AD_OBJ_NAME:
> + case MP_ERROR_AD_OBJ_NAME:
> if (mp_typeof(**data) != MP_STR)
> goto error;
> str = mp_decode_str(data, &str_len);
> - mp_err.ad_obj_name = strndup(str, str_len);
> + mp_err.ad_obj_name = region_strdup(region, str,
> + str_len);
> + if (mp_err.ad_obj_name == NULL)
> + goto finish;
> break;
> - case MP_ERROR_DET_AD_ACCESS_TYPE:
> + case MP_ERROR_AD_ACCESS_TYPE:
> if (mp_typeof(**data) != MP_STR)
> goto error;
> str = mp_decode_str(data, &str_len);
> - mp_err.ad_access_type = strndup(str, str_len);
> + mp_err.ad_access_type = region_strdup(region, str,
> + str_len);
> + if (mp_err.ad_access_type == NULL)
> + goto finish;
> break;
> default:
> mp_next(data);
> @@ -363,11 +375,22 @@ error_unpack(const char **data, uint32_t len)
> assert(*data == end);
>
> err = build_error(&mp_err);
> - mp_error_cleanup(&mp_err);
> +finish:
> + region_truncate(region, region_svp);
> return err;
>
> error:
> diag_set(ClientError, ER_INVALID_MSGPACK,
> "Invalid MP_ERROR MsgPack format");
> - return NULL;
> + goto finish;
> +}
> +
> +struct error *
> +error_unpack(const char **data, uint32_t len)
> +{
> + try {
> + return error_unpack_xc(data, len);
> + } catch (OutOfMemory *e) {
> + return e;
> + }
> }
> diff --git a/test/box-tap/extended_error.test.lua b/test/box-tap/extended_error.test.lua
> index eb86bdc76..8a4f79c04 100755
> --- a/test/box-tap/extended_error.test.lua
> +++ b/test/box-tap/extended_error.test.lua
> @@ -3,12 +3,10 @@
> local netbox = require('net.box')
> local os = require('os')
> local tap = require('tap')
> -local uri = require('uri')
>
> local test = tap.test('check an extended error')
> test:plan(4)
>
> -
> function error_func()
> box.error(box.error.PROC_LUA, "An old good error")
> end
> @@ -22,22 +20,6 @@ function custom_error_func_2()
> return err
> end
>
> -local function version_at_least(peer_version_str, major, minor, patch)
> - local major_p, minor_p, patch_p = string.match(peer_version_str,
> - "(%d)%.(%d)%.(%d)")
> - major_p = tonumber(major_p)
> - minor_p = tonumber(minor_p)
> - patch_p = tonumber(patch_p)
> -
> - if major_p < major
> - or (major_p == major and minor_p < minor)
> - or (major_p == major and minor_p == minor and patch_p < patch) then
> - return false
> - end
> -
> - return true
> -end
> -
> local function grant_func(user, name)
> box.schema.func.create(name, {if_not_exists = true})
> box.schema.user.grant(user, 'execute', 'function', name, {
> @@ -85,15 +67,14 @@ local function check_error(err, check_list)
> return true
> end
>
> -local function test_old_transmission(host, port)
> +local function test_old_transmission()
> grant_func('guest', 'error_func')
> grant_func('guest', 'custom_error_func_2')
>
> - local connection = netbox.connect(host, port)
> + local connection = netbox.connect(box.cfg.listen)
> local _, err = pcall(connection.call, connection, 'error_func')
> local err_2 = connection:call('custom_error_func_2')
>
> -
> local check_list = {
> type = 'ClientError',
> message = 'An old good error',
> @@ -111,12 +92,12 @@ local function test_old_transmission(host, port)
> connection:close()
> end
>
> -local function test_extended_transmission(host, port)
> +local function test_extended_transmission()
> grant_func('guest', 'custom_error_func')
> grant_func('guest', 'custom_error_func_2')
> box.schema.user.grant('guest','read,write', 'space', '_session_settings')
>
> - local connection = netbox.connect(host, port)
> + local connection = netbox.connect(box.cfg.listen)
> connection.space._session_settings:update('error_marshaling_enabled',
> {{'=', 2, true}})
> local _, err = pcall(connection.call, connection, 'custom_error_func')
> @@ -148,10 +129,8 @@ end
> box.cfg{
> listen = os.getenv('LISTEN')
> }
> -local host= uri.parse(box.cfg.listen).host or 'localhost'
> -local port = uri.parse(box.cfg.listen).service
>
> -test_extended_transmission(host, port)
> -test_old_transmission(host, port)
> +test_extended_transmission()
> +test_old_transmission()
>
> os.exit(test:check() and 0 or 1)
>
More information about the Tarantool-patches
mailing list