From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (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 8E4C74696C3 for ; Sat, 18 Apr 2020 20:46:08 +0300 (MSK) References: <5b97369d12305b68155e6a46dd961e13f66600ca.1587058424.git.lvasiliev@tarantool.org> <03c8e932-31d7-d0be-fd71-06fa60598ac9@tarantool.org> From: lvasiliev Message-ID: <72812ce4-1b24-9719-65d5-7f2cbde72139@tarantool.org> Date: Sat, 18 Apr 2020 20:46:07 +0300 MIME-Version: 1.0 In-Reply-To: <03c8e932-31d7-d0be-fd71-06fa60598ac9@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH V4 6/6] error: add error MsgPack encoding 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. 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: { > : ..., > : ..., > ... > } > } > 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) >