Re: [Tarantool-patches] [PATCH V5 6/6] error: add error MsgPack encoding

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

On 18/04/2020 17:29, Leonid Vasiliev wrote:
> Co-authored-by: Vladislav Shpilevoy<v.shpilevoy@tarantool.org>
> 
> 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 |
>         +--------+----------+========+
>     ```
> 
>  MP_ERROR: <MP_MAP> {
>      MP_ERROR_STACK: <MP_ARRAY> {
>          <MP_MAP> {
>              MP_ERROR_TYPE:   <MP_STR>,
>              MP_ERROR_FILE:   <MP_STR>,
>              MP_ERROR_LINE:   <MP_UINT>,
>              MP_ERROR_REASON: <MP_STR>,
>              MP_ERROR_ERRNO:  <MP_STR>,
>              MP_ERROR_FIELDS: <MP_MAP> {
>                  <MP_STR>: ...,
>                  <MP_STR>: ...,
>                  ...
>              },
>          },
>      }
>  }
> 
> MP_ERROR_STACK = 0x00
> 
> At the map passes all necessary error details (depending
> from the error type) to create a copy of the error
> on the client side.
> The necessary fields:
> MP_ERROR_TYPE   = 0x00 - error type(string).
> MP_ERROR_FILE   = 0x01 - file name from trace.
> MP_ERROR_LINE   = 0x02 - line of the file from trace.
> MP_ERROR_REASON = 0x03 - error message.
> MP_ERROR_ERRNO  = 0x04 - saved errno.
> MP_ERROR_FIELDS = 0x05 - additional fields passes at MAP (MP_ERROR_FIELDS).
> Now additional fields may content:
> - ClientError code
> - Custom type
> - Access Denied object type
> - Access Denied object name
> - Access Denied access type

I rewrote the commit message, force pushed. Also I added
MP_ERROR_CODE to the root map. Because I realized a code is
available for every error via public box_error_code()
function in C.

Also I renamed REASON to MESSAGE. Reason is a legacy name
used only in Lua and only in error constructor. It is
visible as <error_object>.message after error creation.

====================

@TarantoolBot document
Title: Error objects encoding in MessagePack

Until now an error sent over IProto, or serialized into
MessagePack was turned into a string consisting of the error
message. As a result, all other error object attributes were lost,
including type of the object. On client side seeing a string it
was not possible to tell whether the string is a real string, or
it is a serialized error.

To deal with that the error objects encoding is reworked from the
scratch. Now, when session setting `error_marshaling_enabled` is
true, all fibers of that session will encode error objects as a
new MP_EXT type - MP_ERROR (0x03).

```
    +--------+----------+========+
    | MP_EXT | MP_ERROR | MP_MAP |
    +--------+----------+========+

    MP_ERROR: <MP_MAP> {
        MP_ERROR_STACK: <MP_ARRAY> [
            <MP_MAP> {
                MP_ERROR_TYPE: <MP_STR>,
                MP_ERROR_FILE: <MP_STR>,
                MP_ERROR_LINE: <MP_UINT>,
                MP_ERROR_MESSAGE: <MP_STR>,
                MP_ERROR_ERRNO: <MP_UINT>,
                MP_ERROR_CODE: <MP_UINT>,
                MP_ERROR_FIELDS: <MP_MAP> {
                    <MP_STR>: ...,
                    <MP_STR>: ...,
                    ...
                },
                ...
            },
            ...
        ]
    }
```

On the top level there is a single key: `MP_ERROR_STACK = 0x00`.
More keys can be added in future, and a client should ignore all
unknown keys to keep compatibility with new versions.

Every error in the stack is a map with the following keys:
* `MP_ERROR_TYPE = 0x00` - error type. This is what is visible in
  `<error_object>.base_type` field;
* `MP_ERROR_FILE = 0x01` - file name from `<error_object>.trace`;
* `MP_ERROR_LINE = 0x02` - line from `<error_object>.trace`;
* `MP_ERROR_MESSAGE = 0x03` - error message from
  `<error_object>.message`;
* `MP_ERROR_ERRNO = 0x04` - errno saved at the moment of the error
  creation. Visible in `<error_object>.errno`;
* `MP_ERROR_CODE = 0x05` - error code. Visible in
  `<error_object>.code` and in C function `box_error_code()`.
* `MP_ERROR_FIELDS = 0x06` - additional fields depending on error
  type. For example, AccessDenied error type stores here fields
  `access_type`, `object_type`, `object_name`. Connector's code
  should ignore unknown keys met here, and be ready, that for some
  existing errors new fields can be added, old can be dropped.

====================

> ---
>  src/box/CMakeLists.txt               |   1 +
>  src/box/lua/init.c                   |  56 ++++
>  src/box/lua/tuple.c                  |  16 -
>  src/box/mp_error.cc                  | 552 +++++++++++++++++++++++++++++++++++
>  src/box/mp_error.h                   |  60 ++++
>  src/lib/core/mp_extension_types.h    |   1 +
>  src/lua/error.c                      |   2 -
>  src/lua/error.h                      |   3 +-
>  src/lua/msgpack.c                    |   3 +
>  src/lua/utils.c                      |   8 +-
>  test/box-tap/extended_error.test.lua | 136 +++++++++
>  11 files changed, 817 insertions(+), 21 deletions(-)
>  create mode 100644 src/box/mp_error.cc
>  create mode 100644 src/box/mp_error.h
>  create mode 100755 test/box-tap/extended_error.test.lua
> 
> diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
> index 6688303..81f6400 100644
> --- a/src/box/CMakeLists.txt
> +++ b/src/box/CMakeLists.txt
> @@ -136,6 +136,7 @@ add_library(box STATIC
>      wal.c
>      call.c
>      merger.c
> +    mp_error.cc

I moved it to box_error library.

====================
diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 81f64001a..c82ceb56c 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -26,7 +26,7 @@ set_property(DIRECTORY PROPERTY ADDITIONAL_MAKE_CLEAN_FILES ${lua_sources})
 include_directories(${ZSTD_INCLUDE_DIRS})
 include_directories(${CMAKE_BINARY_DIR}/src/box/sql)
 
-add_library(box_error STATIC error.cc errcode.c vclock.c)
+add_library(box_error STATIC error.cc errcode.c vclock.c mp_error.cc)
-target_link_libraries(box_error core stat)
+target_link_libraries(box_error core stat mpstream)
 
 add_library(vclock STATIC vclock.c)
 target_link_libraries(vclock core)
 
 add_library(xrow STATIC xrow.c iproto_constants.c)
 target_link_libraries(xrow server core small vclock misc box_error
-                      scramble mpstream ${MSGPUCK_LIBRARIES})
+                      scramble ${MSGPUCK_LIBRARIES})
@@ -136,7 +136,6 @@ add_library(box STATIC
     wal.c
     call.c
     merger.c
-    mp_error.cc
     ${lua_sources}
     lua/init.c
     lua/call.c

====================

I reviewed rest of the code and it looks mostly fine except
things I mentioned above and which I fixed silently. See my diff
below. It is already applied and force pushed.

====================
diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
index 348b7a70b..86cb8dc23 100644
--- a/src/box/mp_error.cc
+++ b/src/box/mp_error.cc
@@ -39,20 +39,23 @@
  * MP_ERROR format:
  *
  * MP_ERROR: <MP_MAP> {
- *     MP_ERROR_STACK: <MP_ARRAY> {
+ *     MP_ERROR_STACK: <MP_ARRAY> [
  *         <MP_MAP> {
- *             MP_ERROR_TYPE:   <MP_STR>,
- *             MP_ERROR_FILE:   <MP_STR>,
- *             MP_ERROR_LINE:   <MP_UINT>,
- *             MP_ERROR_REASON: <MP_STR>,
- *             MP_ERROR_ERRNO:  <MP_STR>,
+ *             MP_ERROR_TYPE:  <MP_STR>,
+ *             MP_ERROR_FILE: <MP_STR>,
+ *             MP_ERROR_LINE: <MP_UINT>,
+ *             MP_ERROR_MESSAGE: <MP_STR>,
+ *             MP_ERROR_ERRNO: <MP_UINT>,
+ *             MP_ERROR_CODE: <MP_UINT>,
  *             MP_ERROR_FIELDS: <MP_MAP> {
  *                 <MP_STR>: ...,
  *                 <MP_STR>: ...,
  *                 ...
  *             },
+ *             ...
  *         },
- *     }
+ *         ...
+ *     ]
  * }
  */
 
@@ -64,24 +67,26 @@ enum {
 };
 
 /**
- * Keys used for error encode/decode to MP_ERROR.
+ * Keys of individual error in the stack.
  */
 enum {
-	/* Error type */
+	/** Error type. */
 	MP_ERROR_TYPE = 0x00,
-	/* File name from trace */
+	/** File name from trace. */
 	MP_ERROR_FILE = 0x01,
-	/* Line from trace */
+	/** Line from trace. */
 	MP_ERROR_LINE = 0x02,
-	/* Error message */
-	MP_ERROR_REASON = 0x03,
-	/* Saved errno */
+	/** Error message. */
+	MP_ERROR_MESSAGE = 0x03,
+	/** Errno at the moment of error creation. */
 	MP_ERROR_ERRNO = 0x04,
+	/** Error code. */
+	MP_ERROR_CODE = 0x05,
 	/*
-	 * Key uses for error type-specific fields which
-	 * presents here as map: string key - value.
+	 * Type-specific fields stored as a map
+	 * {string key = value}.
 	 */
-	MP_ERROR_FIELDS = 0x05
+	MP_ERROR_FIELDS = 0x06
 };
 
 /**
@@ -89,15 +94,15 @@ enum {
  * during decoding MP_ERROR.
  */
 struct mp_error {
-	uint32_t error_code;
+	uint32_t code;
 	uint32_t line;
 	uint32_t saved_errno;
-	const char *error_type;
+	const char *type;
 	const char *file;
-	const char *reason;
+	const char *message;
 	const char *custom_type;
-	const char *ad_obj_type;
-	const char *ad_obj_name;
+	const char *ad_object_type;
+	const char *ad_object_name;
 	const char *ad_access_type;
 };
 
@@ -110,21 +115,18 @@ mp_error_create(struct mp_error *mp_error)
 static uint32_t
 encode_error_size(const struct error *error)
 {
-	uint32_t errcode = 0;
+	uint32_t errcode = box_error_code(error);
 
-	bool is_client = false;
 	bool is_custom = false;
 	bool is_access_denied = false;
 
-	if (strcmp(error->type->name, "ClientError") == 0) {
-		is_client = true;
-	} else if (strcmp(error->type->name, "CustomError") == 0) {
+	if (strcmp(error->type->name, "CustomError") == 0) {
 		is_custom = true;
 	} else if (strcmp(error->type->name, "AccessDeniedError") == 0) {
 		is_access_denied = true;
 	}
 
-	uint32_t details_num = 5;
+	uint32_t details_num = 6;
 	uint32_t data_size = 0;
 
 	data_size += mp_sizeof_uint(MP_ERROR_TYPE);
@@ -133,40 +135,29 @@ encode_error_size(const struct error *error)
 	data_size += mp_sizeof_uint(error->line);
 	data_size += mp_sizeof_uint(MP_ERROR_FILE);
 	data_size += mp_sizeof_str(strlen(error->file));
-	data_size += mp_sizeof_uint(MP_ERROR_REASON);
+	data_size += mp_sizeof_uint(MP_ERROR_MESSAGE);
 	data_size += mp_sizeof_str(strlen(error->errmsg));
 	data_size += mp_sizeof_uint(MP_ERROR_ERRNO);
 	data_size += mp_sizeof_uint(error->saved_errno);
+	data_size += mp_sizeof_uint(MP_ERROR_CODE);
+	data_size += mp_sizeof_uint(errcode);
 
-	if (is_client) {
+	if (is_access_denied) {
 		++details_num;
 		data_size += mp_sizeof_uint(MP_ERROR_FIELDS);
-		errcode = box_error_code(error);
-		data_size += mp_sizeof_map(1);
-		data_size += mp_sizeof_str(strlen("code"));
-		data_size += mp_sizeof_uint(errcode);
-	} else if (is_access_denied) {
-		++details_num;
-		data_size += mp_sizeof_uint(MP_ERROR_FIELDS);
-		errcode = box_error_code(error);
-		data_size += mp_sizeof_map(4);
-		data_size += mp_sizeof_str(strlen("code"));
-		data_size += mp_sizeof_uint(errcode);
+		data_size += mp_sizeof_map(3);
 		AccessDeniedError *ad_err = type_cast(AccessDeniedError, error);
-		data_size += mp_sizeof_str(strlen("AD_OBJ_TYPE"));
+		data_size += mp_sizeof_str(strlen("object_type"));
 		data_size += mp_sizeof_str(strlen(ad_err->object_type()));
-		data_size += mp_sizeof_str(strlen("AD_OBJ_NAME"));
+		data_size += mp_sizeof_str(strlen("object_name"));
 		data_size += mp_sizeof_str(strlen(ad_err->object_name()));
-		data_size += mp_sizeof_str(strlen("AD_ACCESS_TYPE"));
+		data_size += mp_sizeof_str(strlen("access_type"));
 		data_size += mp_sizeof_str(strlen(ad_err->access_type()));
 	} else if (is_custom) {
 		++details_num;
 		data_size += mp_sizeof_uint(MP_ERROR_FIELDS);
-		errcode = box_error_code(error);
-		data_size += mp_sizeof_map(2);
-		data_size += mp_sizeof_str(strlen("code"));
-		data_size += mp_sizeof_uint(errcode);
-		data_size += mp_sizeof_str(strlen("custom"));
+		data_size += mp_sizeof_map(1);
+		data_size += mp_sizeof_str(strlen("custom_type"));
 		data_size +=
 			mp_sizeof_str(strlen(box_error_custom_type(error)));
 	}
@@ -179,23 +170,19 @@ encode_error_size(const struct error *error)
 static void
 encode_error(const struct error *error, char **data)
 {
-	uint32_t errcode = 0;
+	uint32_t errcode = box_error_code(error);
 
-	bool is_client = false;
 	bool is_custom = false;
 	bool is_access_denied = false;
 
-	if (strcmp(error->type->name, "ClientError") == 0) {
-		is_client = true;
-	} else if (strcmp(error->type->name, "CustomError") == 0) {
+	if (strcmp(error->type->name, "CustomError") == 0) {
 		is_custom = true;
 	} else if (strcmp(error->type->name, "AccessDeniedError") == 0) {
 		is_access_denied = true;
 	}
 
-	uint32_t details_num = 5;
-
-	if (is_client || is_access_denied || is_custom)
+	uint32_t details_num = 6;
+	if (is_access_denied || is_custom)
 		++details_num;
 
 	*data = mp_encode_map(*data, details_num);
@@ -206,43 +193,34 @@ encode_error(const struct error *error, char **data)
 	*data = mp_encode_uint(*data, error->line);
 	*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_REASON);
+	*data = mp_encode_uint(*data, MP_ERROR_MESSAGE);
 	*data = mp_encode_str(*data, error->errmsg, strlen(error->errmsg));
 	*data = mp_encode_uint(*data, MP_ERROR_ERRNO);
 	*data = mp_encode_uint(*data, error->saved_errno);
+	*data = mp_encode_uint(*data, MP_ERROR_CODE);
+	*data = mp_encode_uint(*data, errcode);
 
-	if (is_client) {
-		*data = mp_encode_uint(*data, MP_ERROR_FIELDS);
-		errcode = box_error_code(error);
-		*data = mp_encode_map(*data, 1);
-		*data = mp_encode_str(*data, "code",strlen("code"));
-		*data = mp_encode_uint(*data, errcode);
-	} else if (is_access_denied) {
+	if (is_access_denied) {
 		*data = mp_encode_uint(*data, MP_ERROR_FIELDS);
-		errcode = box_error_code(error);
-		*data = mp_encode_map(*data, 4);
-		*data = mp_encode_str(*data, "code",strlen("code"));
-		*data = mp_encode_uint(*data, errcode);
+		*data = mp_encode_map(*data, 3);
 		AccessDeniedError *ad_err = type_cast(AccessDeniedError, error);
-		*data = mp_encode_str(*data, "AD_OBJ_TYPE",
-				      strlen("AD_OBJ_TYPE"));
+		*data = mp_encode_str(*data, "object_type",
+				      strlen("object_type"));
 		*data = mp_encode_str(*data, ad_err->object_type(),
 				      strlen(ad_err->object_type()));
-		*data = mp_encode_str(*data, "AD_OBJ_NAME",
-				      strlen("AD_OBJ_NAME"));
+		*data = mp_encode_str(*data, "object_name",
+				      strlen("object_name"));
 		*data = mp_encode_str(*data, ad_err->object_name(),
 				      strlen(ad_err->object_name()));
-		*data = mp_encode_str(*data, "AD_ACCESS_TYPE",
-				      strlen("AD_ACCESS_TYPE"));
+		*data = mp_encode_str(*data, "access_type",
+				      strlen("access_type"));
 		*data = mp_encode_str(*data, ad_err->access_type(),
 				      strlen(ad_err->access_type()));
 	} else if (is_custom) {
 		*data = mp_encode_uint(*data, MP_ERROR_FIELDS);
-		errcode = box_error_code(error);
-		*data = mp_encode_map(*data, 2);
-		*data = mp_encode_str(*data, "code",strlen("code"));
-		*data = mp_encode_uint(*data, errcode);
-		*data = mp_encode_str(*data, "custom", strlen("custom"));
+		*data = mp_encode_map(*data, 1);
+		*data = mp_encode_str(*data, "custom_type",
+				      strlen("custom_type"));
 		const char *custom = box_error_custom_type(error);
 		*data = mp_encode_str(*data, custom, strlen(custom));
 	}
@@ -252,75 +230,69 @@ static struct error *
 build_error_xc(struct mp_error *mp_error)
 {
 	/*
-	 * To create an error the "raw" constructor using
+	 * To create an error the "raw" constructor is used
 	 * 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;
 
-	if (strcmp(mp_error->error_type, "ClientError") == 0) {
+	if (strcmp(mp_error->type, "ClientError") == 0) {
 		ClientError *e = new ClientError(mp_error->file, mp_error->line,
 						 ER_UNKNOWN);
-		e->m_errcode = mp_error->error_code;
+		e->m_errcode = mp_error->code;
 		err = e;
-	} else if (strcmp(mp_error->error_type, "CustomError") == 0) {
+	} else if (strcmp(mp_error->type, "CustomError") == 0) {
 		err = new CustomError(mp_error->file, mp_error->line,
-				      mp_error->custom_type,
-				      mp_error->error_code);
-	} else if (strcmp(mp_error->error_type, "AccessDeniedError") == 0) {
+				      mp_error->custom_type, mp_error->code);
+	} else if (strcmp(mp_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, "", false);
-	} else if (strcmp(mp_error->error_type, "XlogError") == 0) {
+					    mp_error->ad_object_type,
+					    mp_error->ad_object_name, "",
+					    false);
+	} else if (strcmp(mp_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) {
+	} else if (strcmp(mp_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) {
+				       mp_error->message);
+	} else if (strcmp(mp_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) {
+				      "%s", mp_error->message);
+	} else if (strcmp(mp_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) {
+		error_format_msg(err, "%s", mp_error->message);
+	} else if (strcmp(mp_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) {
+	} else if (strcmp(mp_error->type, "TimedOut") == 0) {
 		err = new TimedOut(mp_error->file, mp_error->line);
-	} else if (strcmp(mp_error->error_type, "ChannelIsClosed") == 0) {
+	} else if (strcmp(mp_error->type, "ChannelIsClosed") == 0) {
 		err = new ChannelIsClosed(mp_error->file, mp_error->line);
-	} else if (strcmp(mp_error->error_type, "FiberIsCancelled") == 0) {
+	} else if (strcmp(mp_error->type, "FiberIsCancelled") == 0) {
 		err = new FiberIsCancelled(mp_error->file, mp_error->line);
-	} else if (strcmp(mp_error->error_type, "LuajitError") == 0) {
+	} else if (strcmp(mp_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) {
+				      mp_error->message);
+	} else if (strcmp(mp_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) {
+					"%s", mp_error->message);
+	} else if (strcmp(mp_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) {
+					 "%s", mp_error->message);
+	} else if (strcmp(mp_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) {
+				    "%s", mp_error->message);
+	} else if (strcmp(mp_error->type, "CryptoError") == 0) {
 		err = new CryptoError(mp_error->file, mp_error->line,
-				      "%s", mp_error->reason);
+				      "%s", mp_error->message);
 	} else {
 		err = new ClientError(mp_error->file, mp_error->line,
 				      ER_UNKNOWN);
 	}
-
-	if (err) {
-		err->saved_errno = mp_error->saved_errno;
-		error_format_msg(err, "%s", mp_error->reason);
-	}
-
+	err->saved_errno = mp_error->saved_errno;
+	error_format_msg(err, "%s", mp_error->message);
 	return err;
 }
 
@@ -337,6 +309,25 @@ region_strdup(struct region *region, const char *str, uint32_t len)
 	return res;
 }
 
+static inline const char *
+decode_and_copy_str(struct region *region, const char **data)
+{
+	if (mp_typeof(**data) != MP_STR) {
+		diag_set(ClientError, ER_INVALID_MSGPACK,
+			 "Invalid MP_ERROR MsgPack format");
+		return NULL;
+	}
+	uint32_t str_len;
+	const char *str = mp_decode_str(data, &str_len);
+	return region_strdup(region, str, str_len);;
+}
+
+static inline bool
+str_nonterm_is_eq(const char *l, const char *r, uint32_t r_len)
+{
+	return r_len == strlen(l) && memcmp(l, r, r_len) == 0;
+}
+
 static int
 decode_additional_fields(const char **data, struct region *region,
 			 struct mp_error *mp_err)
@@ -344,47 +335,28 @@ decode_additional_fields(const char **data, struct region *region,
 	uint32_t map_sz = mp_decode_map(data);
 	const char *key;
 	uint32_t key_len;
-	const char *str;
-	uint32_t str_len;
 	for (uint32_t i = 0; i < map_sz; ++i) {
 		if (mp_typeof(**data) != MP_STR)
 			return -1;
 		key = mp_decode_str(data, &key_len);
-
-		if (strncmp(key, "code", key_len) == 0) {
-			if (mp_typeof(**data) != MP_UINT)
-				return -1;
-			mp_err->error_code = mp_decode_uint(data);
-		} else if (strncmp(key, "AD_OBJ_TYPE", key_len) == 0) {
-			if (mp_typeof(**data) != MP_STR)
+		if (str_nonterm_is_eq("object_type", key, key_len)) {
+			mp_err->ad_object_type =
+				decode_and_copy_str(region, data);
+			if (mp_err->ad_object_type == NULL)
 				return -1;
-			str = mp_decode_str(data, &str_len);
-			mp_err->ad_obj_type = region_strdup(region, str,
-							    str_len);
-			if (mp_err->ad_obj_type == NULL)
+		} else if (str_nonterm_is_eq("object_name", key, key_len)) {
+			mp_err->ad_object_name =
+				decode_and_copy_str(region, data);
+			if (mp_err->ad_object_name == NULL)
 				return -1;
-		} else if (strncmp(key, "AD_OBJ_NAME", key_len) == 0) {
-			if (mp_typeof(**data) != MP_STR)
-				return -1;
-			str = mp_decode_str(data, &str_len);
-			mp_err->ad_obj_name = region_strdup(region, str,
-							    str_len);
-			if (mp_err->ad_obj_name == NULL)
-				return -1;
-		} else if (strncmp(key, "AD_ACCESS_TYPE", key_len) == 0) {
-			if (mp_typeof(**data) != MP_STR)
-				return -1;
-			str = mp_decode_str(data, &str_len);
-			mp_err->ad_access_type = region_strdup(region, str,
-							       str_len);
+		} else if (str_nonterm_is_eq("access_type", key, key_len)) {
+			mp_err->ad_access_type =
+				decode_and_copy_str(region, data);
 			if (mp_err->ad_access_type == NULL)
 				return -1;
-		} else if (strncmp(key, "custom", key_len) == 0) {
-			if (mp_typeof(**data) != MP_STR)
-				return -1;
-			str = mp_decode_str(data, &str_len);
-			mp_err->custom_type = region_strdup(region, str,
-							    str_len);
+		} else if (str_nonterm_is_eq("custom_type", key, key_len)) {
+			mp_err->custom_type =
+				decode_and_copy_str(region, data);
 			if (mp_err->custom_type == NULL)
 				return -1;
 		} else {
@@ -397,43 +369,30 @@ decode_additional_fields(const char **data, struct region *region,
 static struct error *
 decode_error(const char **data)
 {
-	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_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;
+	uint32_t map_size;
+
+	if (mp_typeof(**data) != MP_MAP)
+		goto error;
+
+	map_size = mp_decode_map(data);
 	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");
-			goto finish;
-		}
+		if (mp_typeof(**data) != MP_UINT)
+			goto error;
 
 		uint64_t key = mp_decode_uint(data);
-		const char *str;
-		uint32_t str_len;
 		switch(key) {
 		case MP_ERROR_TYPE:
-			if (mp_typeof(**data) != MP_STR)
-				goto error;
-			str = mp_decode_str(data, &str_len);
-			mp_err.error_type = region_strdup(region, str, str_len);
-			if (mp_err.error_type == NULL)
+			mp_err.type = decode_and_copy_str(region, data);
+			if (mp_err.type == NULL)
 				goto finish;
 			break;
 		case MP_ERROR_FILE:
-			if (mp_typeof(**data) != MP_STR)
-				goto error;
-			str = mp_decode_str(data, &str_len);
-			mp_err.file = region_strdup(region, str, str_len);
+			mp_err.file = decode_and_copy_str(region, data);
 			if (mp_err.file == NULL)
 				goto finish;
 			break;
@@ -442,12 +401,9 @@ decode_error(const char **data)
 				goto error;
 			mp_err.line = mp_decode_uint(data);
 			break;
-		case MP_ERROR_REASON:
-			if (mp_typeof(**data) != MP_STR)
-				goto error;
-			str = mp_decode_str(data, &str_len);
-			mp_err.reason = region_strdup(region, str, str_len);
-			if (mp_err.reason == NULL)
+		case MP_ERROR_MESSAGE:
+			mp_err.message = decode_and_copy_str(region, data);
+			if (mp_err.message == NULL)
 				goto finish;
 			break;
 		case MP_ERROR_ERRNO:
@@ -455,10 +411,15 @@ decode_error(const char **data)
 				goto error;
 			mp_err.saved_errno = mp_decode_uint(data);
 			break;
-		case MP_ERROR_FIELDS:
-			if (decode_additional_fields(data, region, &mp_err) != 0) {
+		case MP_ERROR_CODE:
+			if (mp_typeof(**data) != MP_UINT)
 				goto error;
-			}
+			mp_err.code = mp_decode_uint(data);
+			break;
+		case MP_ERROR_FIELDS:
+			if (decode_additional_fields(data, region,
+						     &mp_err) != 0)
+				goto finish;
 			break;
 		default:
 			mp_next(data);
@@ -468,7 +429,7 @@ decode_error(const char **data)
 	try {
 		err = build_error_xc(&mp_err);
 	} catch (OutOfMemory *e) {
-		diag_set_error(&fiber()->diag, e);
+		assert(err == NULL && !diag_is_empty(diag_get()));
 	}
 finish:
 	region_truncate(region, region_svp);
@@ -522,8 +483,7 @@ error_unpack(const char **data, uint32_t len)
 		}
 		uint64_t key = mp_decode_uint(data);
 		switch(key) {
-		case MP_ERROR_STACK:
-		{
+		case MP_ERROR_STACK: {
 			uint32_t stack_sz = mp_decode_array(data);
 			struct error *effect = NULL;
 			for (uint32_t i = 0; i < stack_sz; i++) {
diff --git a/src/lua/error.h b/src/lua/error.h
index 4e4dc048b..efaaa63fd 100644
--- a/src/lua/error.h
+++ b/src/lua/error.h
@@ -37,6 +37,7 @@
 extern "C" {
 #endif /* defined(__cplusplus) */
 
+
 /** \cond public */
 struct error;
 
diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
index 91560eff0..f2ae2d8c3 100644
--- a/src/lua/msgpack.c
+++ b/src/lua/msgpack.c
@@ -239,7 +239,6 @@ luamp_encode(struct lua_State *L, struct luaL_serializer *cfg,
 	return top_type;
 }
 
-
 void
 luamp_decode(struct lua_State *L, struct luaL_serializer *cfg,
 	     const char **data)