* [Tarantool-patches] [PATCH V6 00/10] Extending error functionality
@ 2020-04-19 22:25 Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 01/10] error: add custom error type Leonid Vasiliev
` (11 more replies)
0 siblings, 12 replies; 18+ messages in thread
From: Leonid Vasiliev @ 2020-04-19 22:25 UTC (permalink / raw)
To: v.shpilevoy, alexander.turenko; +Cc: tarantool-patches
https://github.com/tarantool/tarantool/issues/4398
https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4398-expose-error-module-5
Changes from previous:
- traceback patch has been removed
- Vlad patches have been applied
- Update old tests and add new
- Make iproto errors reuse mp_error module
According to https://github.com/tarantool/tarantool/issues/4398
(and after some discussion) we would like box.error to have:
* Ability to create new error types
* Transparent marshalling through net.box
@Changelog
Added:
Posibility to create errors of a custom user type
Transparent marshalling error through net.box(gh-4398)
Leonid Vasiliev (5):
error: add custom error type
error: add session setting for error type marshaling
error: update constructors of some errors
error: add error MsgPack encoding
error: fix iproto error stack overlapped by old error
Vladislav Shpilevoy (5):
session: add offset to SQL session settings array
box: move Lua MP_EXT decoder from tuple.c
error: export error_unref() function
error: make iproto errors reuse mp_error module
iproto: rename IPROTO_ERROR and IPROTO_ERROR_STACK
extra/exports | 3 +
src/box/CMakeLists.txt | 6 +-
src/box/error.cc | 125 ++++++--
src/box/error.h | 51 +++-
src/box/iproto_constants.h | 9 +-
src/box/lua/call.c | 25 +-
src/box/lua/error.cc | 42 ++-
src/box/lua/execute.c | 2 +-
src/box/lua/init.c | 54 ++++
src/box/lua/net_box.lua | 103 +++++--
src/box/lua/tuple.c | 28 +-
src/box/mp_error.cc | 554 +++++++++++++++++++++++++++++++++++
src/box/mp_error.h | 74 +++++
src/box/session.cc | 5 +-
src/box/session.h | 5 +-
src/box/session_settings.c | 56 ++++
src/box/session_settings.h | 1 +
src/box/sql/build.c | 18 +-
src/box/sql/func.c | 4 +-
src/box/xrow.c | 81 +----
src/lib/core/diag.c | 20 ++
src/lib/core/diag.h | 21 +-
src/lib/core/mp_extension_types.h | 1 +
src/lua/error.c | 2 +-
src/lua/error.h | 1 +
src/lua/error.lua | 14 +-
src/lua/msgpack.c | 27 +-
src/lua/msgpack.h | 8 +-
src/lua/utils.c | 15 +-
src/lua/utils.h | 7 +-
src/serializer_opts.h | 44 +++
test/app/fiber.result | 5 +-
test/box-tap/extended_error.test.lua | 150 ++++++++++
test/box/error.result | 65 +++-
test/box/error.test.lua | 15 +
test/box/iproto.result | 43 +--
test/box/iproto.test.lua | 17 +-
test/box/session_settings.result | 3 +-
test/engine/func_index.result | 14 +-
test/unit/CMakeLists.txt | 2 +
test/unit/mp_error.cc | 473 ++++++++++++++++++++++++++++++
test/unit/mp_error.result | 44 +++
test/unit/xrow.cc | 183 +-----------
test/unit/xrow.result | 27 +-
44 files changed, 1933 insertions(+), 514 deletions(-)
create mode 100644 src/box/mp_error.cc
create mode 100644 src/box/mp_error.h
create mode 100644 src/serializer_opts.h
create mode 100755 test/box-tap/extended_error.test.lua
create mode 100644 test/unit/mp_error.cc
create mode 100644 test/unit/mp_error.result
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH V6 01/10] error: add custom error type
2020-04-19 22:25 [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Leonid Vasiliev
@ 2020-04-19 22:25 ` Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 02/10] session: add offset to SQL session settings array Leonid Vasiliev
` (10 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Leonid Vasiliev @ 2020-04-19 22:25 UTC (permalink / raw)
To: v.shpilevoy, alexander.turenko; +Cc: tarantool-patches
Co-authored-by: Vladislav Shpilevoy<v.shpilevoy@tarantool.org>
Part of #4398
@TarantoolBot document
Title: Custom error types for Lua errors
Errors can be created in 2 ways: `box.error.new()` and `box.error()`.
Both used to take either `code, reason, <reason string args>` or
`{code = code, reason = reason, ...}` arguments.
Now in the first option instead of code a user can specify a
string as its own error type. In the second option a user can
specify both code and type. For example:
```Lua
box.error('MyErrorType', 'Message')
box.error({type = 'MyErrorType', code = 1024, reason = 'Message'})
```
Or no-throw version:
```Lua
box.error.new('MyErrorType', 'Message')
box.error.new({type = 'MyErrorType', code = 1024, reason = 'Message'})
```
When a custom type is specified, it is shown in `err.type`
attribute. When it is not specified, `err.type` shows one of
built-in errors such as 'ClientError', 'OurOfMemory', etc.
Name length limit on the custom type is 63 bytes. All what is
longer is truncated.
Original error type can be checked using `err.base_type` member,
although normally it should not be used. For user-defined types
base type is 'CustomError'.
For example:
```
tarantool> e = box.error.new({type = 'MyErrorType', code = 1024, reason = 'Message'})
---
...
tarantool> e:unpack()
---
- code: 1024
trace:
- file: '[string "e = box.error.new({type = ''MyErrorType'', code..."]'
line: 1
type: MyErrorType
custom_type: MyErrorType
message: Message
base_type: CustomError
...
```
---
| 1 +
src/box/error.cc | 101 +++++++++++++++++++++++++++++++++---------
src/box/error.h | 40 +++++++++++++++--
src/box/lua/error.cc | 42 +++++++++++++-----
src/box/xrow.c | 2 +-
src/lua/error.lua | 14 +++++-
test/app/fiber.result | 5 ++-
test/box/error.result | 65 +++++++++++++++++++++++++--
test/box/error.test.lua | 15 +++++++
test/engine/func_index.result | 14 +++---
10 files changed, 250 insertions(+), 49 deletions(-)
--git a/extra/exports b/extra/exports
index a9add2c..9467398 100644
--- a/extra/exports
+++ b/extra/exports
@@ -235,6 +235,7 @@ box_index_min
box_index_max
box_index_count
box_error_type
+box_error_custom_type
box_error_code
box_error_message
box_error_last
diff --git a/src/box/error.cc b/src/box/error.cc
index 233b312..277727d 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -86,35 +86,52 @@ box_error_set(const char *file, unsigned line, uint32_t code,
return -1;
}
+static struct error *
+box_error_new_va(const char *file, unsigned line, uint32_t code,
+ const char *custom_type, const char *fmt, va_list ap)
+{
+ if (custom_type == NULL) {
+ struct error *e = BuildClientError(file, line, ER_UNKNOWN);
+ ClientError *client_error = type_cast(ClientError, e);
+ if (client_error != NULL) {
+ client_error->m_errcode = code;
+ error_vformat_msg(e, fmt, ap);
+ }
+ return e;
+ } else {
+ struct error *e = BuildCustomError(file, line, custom_type,
+ code);
+ CustomError *custom_error = type_cast(CustomError, e);
+ if (custom_error != NULL) {
+ error_vformat_msg(e, fmt, ap);
+ }
+
+ return e;
+ }
+}
+
struct error *
box_error_new(const char *file, unsigned line, uint32_t code,
- const char *fmt, ...)
+ const char *custom_type, const char *fmt, ...)
{
- struct error *e = BuildClientError(file, line, ER_UNKNOWN);
- ClientError *client_error = type_cast(ClientError, e);
- if (client_error != NULL) {
- client_error->m_errcode = code;
- va_list ap;
- va_start(ap, fmt);
- error_vformat_msg(e, fmt, ap);
- va_end(ap);
- }
+ va_list ap;
+ va_start(ap, fmt);
+ struct error *e = box_error_new_va(file, line, code, custom_type,
+ fmt, ap);
+ va_end(ap);
return e;
}
int
box_error_add(const char *file, unsigned line, uint32_t code,
- const char *fmt, ...)
+ const char *custom_type, const char *fmt, ...)
{
- struct error *e = BuildClientError(file, line, ER_UNKNOWN);
- ClientError *client_error = type_cast(ClientError, e);
- if (client_error) {
- client_error->m_errcode = code;
- va_list ap;
- va_start(ap, fmt);
- error_vformat_msg(e, fmt, ap);
- va_end(ap);
- }
+ va_list ap;
+ va_start(ap, fmt);
+ struct error *e = box_error_new_va(file, line, code, custom_type,
+ fmt, ap);
+ va_end(ap);
+
struct diag *d = &fiber()->diag;
if (diag_is_empty(d))
diag_set_error(d, e);
@@ -125,6 +142,16 @@ box_error_add(const char *file, unsigned line, uint32_t code,
/* }}} */
+const char *
+box_error_custom_type(const struct error *e)
+{
+ CustomError *custom_error = type_cast(CustomError, e);
+ if (custom_error)
+ return custom_error->custom_type();
+
+ return NULL;
+}
+
struct rmean *rmean_error = NULL;
const char *rmean_error_strings[RMEAN_ERROR_LAST] = {
@@ -290,3 +317,37 @@ BuildAccessDeniedError(const char *file, unsigned int line,
return e;
}
}
+
+static struct method_info customerror_methods[] = {
+ make_method(&type_CustomError, "custom_type", &CustomError::custom_type),
+ METHODS_SENTINEL
+};
+
+const struct type_info type_CustomError =
+ make_type("CustomError", &type_ClientError, customerror_methods);
+
+CustomError::CustomError(const char *file, unsigned int line,
+ const char *custom_type, uint32_t errcode)
+ :ClientError(&type_CustomError, file, line, errcode)
+{
+ strncpy(m_custom_type, custom_type, sizeof(m_custom_type) - 1);
+ m_custom_type[sizeof(m_custom_type) - 1] = '\0';
+}
+
+void
+CustomError::log() const
+{
+ say_file_line(S_ERROR, file, line, errmsg, "%s",
+ "Custom type %s", m_custom_type);
+}
+
+struct error *
+BuildCustomError(const char *file, unsigned int line, const char *custom_type,
+ uint32_t errcode)
+{
+ try {
+ return new CustomError(file, line, custom_type, errcode);
+ } catch (OutOfMemory *e) {
+ return e;
+ }
+}
diff --git a/src/box/error.h b/src/box/error.h
index ca5d5b2..461ca0f 100644
--- a/src/box/error.h
+++ b/src/box/error.h
@@ -53,6 +53,10 @@ struct error *
BuildXlogGapError(const char *file, unsigned line,
const struct vclock *from, const struct vclock *to);
+struct error *
+BuildCustomError(const char *file, unsigned int line, const char *custom_type,
+ uint32_t errcode);
+
/** \cond public */
struct error;
@@ -138,11 +142,22 @@ box_error_set(const char *file, unsigned line, uint32_t code,
/** \endcond public */
/**
+ * Return the error custom type. It is NULL in case the error
+ * does not have it.
+ * @param e Error object.
+ * @return Pointer to custom error type.
+ */
+const char *
+box_error_custom_type(const struct error *e);
+
+/**
* Add error to the diagnostic area. In contrast to box_error_set()
* it does not replace previous error being set, but rather link
* them into list.
*
* \param code IPROTO error code (enum \link box_error_code \endlink)
+ * \param custom_type User-defined error type which will be
+ * displayed instead of ClientError.
* \param format (const char * ) - printf()-like format string
* \param ... - format arguments
* \returns -1 for convention use
@@ -151,19 +166,20 @@ box_error_set(const char *file, unsigned line, uint32_t code,
*/
int
box_error_add(const char *file, unsigned line, uint32_t code,
- const char *fmt, ...);
+ const char *custom_type, const char *fmt, ...);
/**
* Construct error object without setting it in the diagnostics
- * area. On the memory allocation fail returns NULL.
+ * area. On the memory allocation fail returns OutOfMemory error.
*/
struct error *
box_error_new(const char *file, unsigned line, uint32_t code,
- const char *fmt, ...);
+ const char *custom_type, const char *fmt, ...);
extern const struct type_info type_ClientError;
extern const struct type_info type_XlogError;
extern const struct type_info type_AccessDeniedError;
+extern const struct type_info type_CustomError;
#if defined(__cplusplus)
} /* extern "C" */
@@ -290,6 +306,24 @@ struct XlogGapError: public XlogError
virtual void raise() { throw this; }
};
+class CustomError: public ClientError
+{
+public:
+ CustomError(const char *file, unsigned int line,
+ const char *custom_type, uint32_t errcode);
+
+ virtual void log() const;
+
+ const char*
+ custom_type()
+ {
+ return m_custom_type;
+ }
+private:
+ /** Custom type name. */
+ char m_custom_type[64];
+};
+
#endif /* defined(__cplusplus) */
#endif /* TARANTOOL_BOX_ERROR_H_INCLUDED */
diff --git a/src/box/lua/error.cc b/src/box/lua/error.cc
index b2625bf..e0a3e73 100644
--- a/src/box/lua/error.cc
+++ b/src/box/lua/error.cc
@@ -46,6 +46,13 @@ extern "C" {
* Parse Lua arguments (they can come as single table
* f({code : number, reason : string}) or as separate members
* f(code, reason)) and construct struct error with given values.
+ *
+ * Instead of 'code' it is possible to specify a string name of
+ * the error object's type:
+ *
+ * box.error(type, reason, ...)
+ * box.error({type = string, reason = string, ...})
+ *
* In case one of arguments is missing its corresponding field
* in struct error is filled with default value.
*/
@@ -53,14 +60,22 @@ static struct error *
luaT_error_create(lua_State *L, int top_base)
{
uint32_t code = 0;
+ const char *custom_type = NULL;
const char *reason = NULL;
const char *file = "";
unsigned line = 0;
lua_Debug info;
int top = lua_gettop(L);
- if (top >= top_base && lua_type(L, top_base) == LUA_TNUMBER) {
- code = lua_tonumber(L, top_base);
- reason = tnt_errcode_desc(code);
+ int top_type = lua_type(L, top_base);
+ if (top >= top_base && (top_type == LUA_TNUMBER ||
+ top_type == LUA_TSTRING)) {
+ if (top_type == LUA_TNUMBER) {
+ code = lua_tonumber(L, top_base);
+ reason = tnt_errcode_desc(code);
+ } else {
+ custom_type = lua_tostring(L, top_base);
+ reason = "%s";
+ }
if (top > top_base) {
/* Call string.format(reason, ...) to format message */
lua_getglobal(L, "string");
@@ -78,14 +93,17 @@ luaT_error_create(lua_State *L, int top_base)
/* Missing arguments to format string */
return NULL;
}
- } else if (top == top_base && lua_istable(L, top_base)) {
+ } else if (top == top_base && top_type == LUA_TTABLE) {
lua_getfield(L, top_base, "code");
- code = lua_tonumber(L, -1);
- lua_pop(L, 1);
+ if (!lua_isnil(L, -1))
+ code = lua_tonumber(L, -1);
lua_getfield(L, top_base, "reason");
reason = lua_tostring(L, -1);
if (reason == NULL)
reason = "";
+ lua_getfield(L, top_base, "type");
+ if (!lua_isnil(L, -1))
+ custom_type = lua_tostring(L, -1);
lua_pop(L, 1);
} else {
return NULL;
@@ -102,7 +120,7 @@ raise:
}
line = info.currentline;
}
- return box_error_new(file, line, code, "%s", reason);
+ return box_error_new(file, line, code, custom_type, "%s", reason);
}
static int
@@ -149,11 +167,11 @@ luaT_error_last(lua_State *L)
static int
luaT_error_new(lua_State *L)
{
- if (lua_gettop(L) == 0)
- return luaL_error(L, "Usage: box.error.new(code, args)");
- struct error *e = luaT_error_create(L, 1);
- if (e == NULL)
- return luaL_error(L, "Usage: box.error.new(code, args)");
+ struct error *e;
+ if (lua_gettop(L) == 0 || (e = luaT_error_create(L, 1)) == NULL) {
+ return luaL_error(L, "Usage: box.error.new(code, args) or "\
+ "box.error.new(type, args)");
+ }
lua_settop(L, 0);
luaT_pusherror(L, e);
return 1;
diff --git a/src/box/xrow.c b/src/box/xrow.c
index da2ad5c..97a604a 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -1133,7 +1133,7 @@ iproto_decode_error_stack(const char **pos)
continue;
}
}
- box_error_add(__FILE__, __LINE__, code, reason);
+ box_error_add(__FILE__, __LINE__, code, NULL, reason);
}
return 0;
}
diff --git a/src/lua/error.lua b/src/lua/error.lua
index bdc9c71..a5f7435 100644
--- a/src/lua/error.lua
+++ b/src/lua/error.lua
@@ -35,6 +35,9 @@ exception_get_int(struct error *e, const struct method_info *method);
int
error_set_prev(struct error *e, struct error *prev);
+
+const char *
+box_error_custom_type(const struct error *e);
]]
local REFLECTION_CACHE = {}
@@ -75,10 +78,18 @@ local function reflection_get(err, method)
end
end
-local function error_type(err)
+local function error_base_type(err)
return ffi.string(err._type.name)
end
+local function error_type(err)
+ local res = ffi.C.box_error_custom_type(err)
+ if res ~= nil then
+ return ffi.string(res)
+ end
+ return error_base_type(err)
+end
+
local function error_message(err)
return ffi.string(err._errmsg)
end
@@ -131,6 +142,7 @@ local error_fields = {
["trace"] = error_trace;
["errno"] = error_errno;
["prev"] = error_prev;
+ ["base_type"] = error_base_type
}
local function error_unpack(err)
diff --git a/test/app/fiber.result b/test/app/fiber.result
index debfc67..9b82790 100644
--- a/test/app/fiber.result
+++ b/test/app/fiber.result
@@ -1038,8 +1038,9 @@ st;
...
e:unpack();
---
-- type: ClientError
- code: 1
+- code: 1
+ base_type: ClientError
+ type: ClientError
message: Illegal parameters, oh my
trace:
- file: '[string "function err() box.error(box.error.ILLEGAL_PA..."]'
diff --git a/test/box/error.result b/test/box/error.result
index df6d0eb..0192017 100644
--- a/test/box/error.result
+++ b/test/box/error.result
@@ -34,8 +34,9 @@ e
| ...
e:unpack()
| ---
- | - type: ClientError
- | code: 1
+ | - code: 1
+ | base_type: ClientError
+ | type: ClientError
| message: Illegal parameters, bla bla
| trace:
| - file: '[C]'
@@ -105,7 +106,7 @@ e
| ...
box.error.new()
| ---
- | - error: 'Usage: box.error.new(code, args)'
+ | - error: 'Usage: box.error.new(code, args) or box.error.new(type, args)'
| ...
--
@@ -489,7 +490,7 @@ assert(box.error.last() == nil)
--
box.error.new(err)
| ---
- | - error: 'Usage: box.error.new(code, args)'
+ | - error: 'Usage: box.error.new(code, args) or box.error.new(type, args)'
| ...
-- box.error() is supposed to re-throw last diagnostic error.
@@ -831,3 +832,59 @@ assert(box.error.last() == e1)
| ---
| - true
| ...
+--
+-- gh-4398: custom error type.
+--
+-- Try no code.
+e = box.error.new({type = 'TestType', reason = 'Test reason'})
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - code: 0
+ | base_type: CustomError
+ | type: TestType
+ | custom_type: TestType
+ | message: Test reason
+ | trace:
+ | - file: '[string "e = box.error.new({type = ''TestType'', reason ..."]'
+ | line: 1
+ | ...
+-- Try code not the same as used by default.
+e = box.error.new({type = 'TestType', reason = 'Test reason', code = 123})
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - code: 123
+ | base_type: CustomError
+ | type: TestType
+ | custom_type: TestType
+ | message: Test reason
+ | trace:
+ | - file: '[string "e = box.error.new({type = ''TestType'', reason ..."]'
+ | line: 1
+ | ...
+-- Try to omit message.
+e = box.error.new({type = 'TestType'})
+ | ---
+ | ...
+e:unpack()
+ | ---
+ | - code: 0
+ | base_type: CustomError
+ | type: TestType
+ | custom_type: TestType
+ | message:
+ | trace:
+ | - file: '[string "e = box.error.new({type = ''TestType''}) "]'
+ | line: 1
+ | ...
+-- Try too long type name.
+e = box.error.new({type = string.rep('a', 128)})
+ | ---
+ | ...
+#e.type
+ | ---
+ | - 63
+ | ...
diff --git a/test/box/error.test.lua b/test/box/error.test.lua
index 41baed5..38f7406 100644
--- a/test/box/error.test.lua
+++ b/test/box/error.test.lua
@@ -229,3 +229,18 @@ box.error({code = 111, reason = "err"})
box.error.last()
box.error(e1)
assert(box.error.last() == e1)
+--
+-- gh-4398: custom error type.
+--
+-- Try no code.
+e = box.error.new({type = 'TestType', reason = 'Test reason'})
+e:unpack()
+-- Try code not the same as used by default.
+e = box.error.new({type = 'TestType', reason = 'Test reason', code = 123})
+e:unpack()
+-- Try to omit message.
+e = box.error.new({type = 'TestType'})
+e:unpack()
+-- Try too long type name.
+e = box.error.new({type = string.rep('a', 128)})
+#e.type
diff --git a/test/engine/func_index.result b/test/engine/func_index.result
index a827c92..e70e591 100644
--- a/test/engine/func_index.result
+++ b/test/engine/func_index.result
@@ -277,21 +277,23 @@ e = box.error.last()
e:unpack()
| ---
| - code: 198
- | trace:
- | - file: <filename>
- | line: <line>
+ | base_type: ClientError
| type: ClientError
- | message: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
- | can''t evaluate function'
| prev: '[string "return function(tuple) local ..."]:1: attempt to
| call global ''require'' (a nil value)'
+ | message: 'Failed to build a key for functional index ''idx'' of space ''withdata'':
+ | can''t evaluate function'
+ | trace:
+ | - file: <filename>
+ | line: <line>
| ...
e = e.prev
| ---
| ...
e:unpack()
| ---
- | - type: LuajitError
+ | - base_type: LuajitError
+ | type: LuajitError
| message: '[string "return function(tuple) local ..."]:1: attempt
| to call global ''require'' (a nil value)'
| trace:
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH V6 02/10] session: add offset to SQL session settings array
2020-04-19 22:25 [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 01/10] error: add custom error type Leonid Vasiliev
@ 2020-04-19 22:25 ` Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 03/10] error: add session setting for error type marshaling Leonid Vasiliev
` (9 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Leonid Vasiliev @ 2020-04-19 22:25 UTC (permalink / raw)
To: v.shpilevoy, alexander.turenko; +Cc: tarantool-patches
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Session settings are stored in a monolithic array. Submodules
can define a range of settings in there. For example, SQL. It
occupies settings from 0 to 8. There is a second array of only
SQL settings in build.c, of the same size, and it uses the same
indexes.
But if something will be added before SQL settings, so as they
won't start from 0, it will break the equal indexes assumption.
SQL should normalize all setting identifiers by
SESSION_SETTING_SQL_BEGIN.
---
src/box/sql/build.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index aecba41..b1f9fed 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3367,7 +3367,8 @@ sql_session_setting_get(int id, const char **mp_pair, const char **mp_pair_end)
assert(id >= SESSION_SETTING_SQL_BEGIN && id < SESSION_SETTING_SQL_END);
struct session *session = current_session();
uint32_t flags = session->sql_flags;
- struct sql_option_metadata *opt = &sql_session_opts[id];
+ struct sql_option_metadata *opt =
+ &sql_session_opts[id - SESSION_SETTING_SQL_BEGIN];
uint32_t mask = opt->mask;
const char *name = session_setting_strs[id];
size_t name_len = strlen(name);
@@ -3404,7 +3405,8 @@ static int
sql_set_boolean_option(int id, bool value)
{
struct session *session = current_session();
- struct sql_option_metadata *option = &sql_session_opts[id];
+ struct sql_option_metadata *option =
+ &sql_session_opts[id - SESSION_SETTING_SQL_BEGIN];
assert(option->field_type == FIELD_TYPE_BOOLEAN);
#ifdef NDEBUG
if ((session->sql_flags & SQL_SqlTrace) == 0) {
@@ -3431,7 +3433,8 @@ sql_set_boolean_option(int id, bool value)
static int
sql_set_string_option(int id, const char *value)
{
- assert(sql_session_opts[id].field_type = FIELD_TYPE_STRING);
+ assert(sql_session_opts[id - SESSION_SETTING_SQL_BEGIN].field_type =
+ FIELD_TYPE_STRING);
assert(id == SESSION_SETTING_SQL_DEFAULT_ENGINE);
(void)id;
enum sql_storage_engine engine = STR2ENUM(sql_storage_engine, value);
@@ -3448,7 +3451,8 @@ sql_session_setting_set(int id, const char *mp_value)
{
assert(id >= SESSION_SETTING_SQL_BEGIN && id < SESSION_SETTING_SQL_END);
enum mp_type mtype = mp_typeof(*mp_value);
- enum field_type stype = sql_session_opts[id].field_type;
+ enum field_type stype =
+ sql_session_opts[id - SESSION_SETTING_SQL_BEGIN].field_type;
uint32_t len;
const char *tmp;
switch(stype) {
@@ -3473,10 +3477,10 @@ sql_session_setting_set(int id, const char *mp_value)
void
sql_session_settings_init()
{
- for (int id = SESSION_SETTING_SQL_BEGIN; id < SESSION_SETTING_SQL_END;
- ++id) {
+ for (int i = 0, id = SESSION_SETTING_SQL_BEGIN;
+ id < SESSION_SETTING_SQL_END; ++id, ++i) {
struct session_setting *setting = &session_settings[id];
- setting->field_type = sql_session_opts[id].field_type;
+ setting->field_type = sql_session_opts[i].field_type;
setting->get = sql_session_setting_get;
setting->set = sql_session_setting_set;
}
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH V6 03/10] error: add session setting for error type marshaling
2020-04-19 22:25 [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 01/10] error: add custom error type Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 02/10] session: add offset to SQL session settings array Leonid Vasiliev
@ 2020-04-19 22:25 ` Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 04/10] error: update constructors of some errors Leonid Vasiliev
` (8 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Leonid Vasiliev @ 2020-04-19 22:25 UTC (permalink / raw)
To: v.shpilevoy, alexander.turenko; +Cc: tarantool-patches
Errors are encoded as a string when serialized to MessagePack to
be sent over IProto or when just saved into a buffer via Lua
modules msgpackffi and msgpack.
That is not very useful on client-side, because most of the error
metadata is lost: code, type, trace - everything except the
message.
Next commits are going to dedicate a new MP_EXT type to error
objects so as everything could be encoded, and on client side it
would be possible to restore types.
But this is a breaking change in case some users use old
connectors when work with newer Tarantool instances. So to smooth
the upgrade there is a new session setting -
'error_marshaling_enabled'.
By default it is false. When it is true, all fibers of the given
session will serialize error objects as MP_EXT.
Co-authored-by: Vladislav Shpilevoy<v.shpilevoy@tarantool.org>
Needed for #4398
---
src/box/lua/call.c | 25 ++++++++++--------
src/box/lua/execute.c | 2 +-
src/box/lua/tuple.c | 12 ++++-----
src/box/session.cc | 5 +++-
src/box/session.h | 5 ++--
src/box/session_settings.c | 56 ++++++++++++++++++++++++++++++++++++++++
src/box/session_settings.h | 1 +
src/box/sql/func.c | 4 ++-
src/lua/msgpack.c | 25 +++++++++---------
src/lua/msgpack.h | 8 +++---
src/lua/utils.c | 11 +++++---
src/lua/utils.h | 7 +++--
src/serializer_opts.h | 44 +++++++++++++++++++++++++++++++
test/box/session_settings.result | 3 ++-
14 files changed, 164 insertions(+), 44 deletions(-)
create mode 100644 src/serializer_opts.h
diff --git a/src/box/lua/call.c b/src/box/lua/call.c
index 1d29494..6588ec2 100644
--- a/src/box/lua/call.c
+++ b/src/box/lua/call.c
@@ -46,6 +46,7 @@
#include "small/obuf.h"
#include "trivia/util.h"
#include "mpstream/mpstream.h"
+#include "box/session.h"
/**
* A helper to find a Lua function by name and put it
@@ -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)
return luaT_error(L);
struct tuple *tuple;
if (field.type == MP_EXT &&
@@ -188,11 +189,11 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
*/
lua_pushvalue(L, i);
mpstream_encode_array(stream, 1);
- luamp_encode_r(L, cfg, stream, &field, 0);
+ luamp_encode_r(L, cfg, NULL, stream, &field, 0);
lua_pop(L, 1);
} else {
/* `return ..., array, ...` */
- luamp_encode(L, cfg, stream, i);
+ luamp_encode(L, cfg, NULL, stream, i);
}
}
return nrets;
@@ -203,7 +204,7 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
* Inspect the first result
*/
struct luaL_field root;
- if (luaL_tofield(L, cfg, 1, &root) < 0)
+ if (luaL_tofield(L, cfg, NULL, 1, &root) < 0)
return luaT_error(L);
struct tuple *tuple;
if (root.type == MP_EXT && (tuple = luaT_istuple(L, 1)) != NULL) {
@@ -217,7 +218,7 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
*/
mpstream_encode_array(stream, 1);
assert(lua_gettop(L) == 1);
- luamp_encode_r(L, cfg, stream, &root, 0);
+ luamp_encode_r(L, cfg, NULL, stream, &root, 0);
return 1;
}
@@ -233,7 +234,7 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
for (uint32_t t = 1; t <= root.size; t++) {
lua_rawgeti(L, 1, t);
struct luaL_field field;
- if (luaL_tofield(L, cfg, -1, &field) < 0)
+ if (luaL_tofield(L, cfg, NULL, -1, &field) < 0)
return luaT_error(L);
if (field.type == MP_EXT && (tuple = luaT_istuple(L, -1))) {
tuple_to_mpstream(tuple, stream);
@@ -249,13 +250,13 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
* Encode the first field of tuple using
* existing information from luaL_tofield
*/
- luamp_encode_r(L, cfg, stream, &field, 0);
+ luamp_encode_r(L, cfg, NULL, stream, &field, 0);
lua_pop(L, 1);
assert(lua_gettop(L) == 1);
/* Encode remaining fields as usual */
for (uint32_t f = 2; f <= root.size; f++) {
lua_rawgeti(L, 1, f);
- luamp_encode(L, cfg, stream, -1);
+ luamp_encode(L, cfg, NULL, stream, -1);
lua_pop(L, 1);
}
return 1;
@@ -265,10 +266,10 @@ luamp_encode_call_16(lua_State *L, struct luaL_serializer *cfg,
* { tuple/array, ..., { scalar }, ... }`
*/
mpstream_encode_array(stream, 1);
- luamp_encode_r(L, cfg, stream, &field, 0);
+ luamp_encode_r(L, cfg, NULL, stream, &field, 0);
} else {
/* `return { tuple/array, ..., tuple/array, ... }` */
- luamp_encode_r(L, cfg, stream, &field, 0);
+ luamp_encode_r(L, cfg, NULL, stream, &field, 0);
}
lua_pop(L, 1);
assert(lua_gettop(L) == 1);
@@ -392,9 +393,11 @@ encode_lua_call(lua_State *L)
* TODO: forbid explicit yield from __serialize or __index here
*/
struct luaL_serializer *cfg = luaL_msgpack_default;
+ const struct serializer_opts *opts =
+ ¤t_session()->meta.serializer_opts;
int size = lua_gettop(ctx->port->L);
for (int i = 1; i <= size; ++i)
- luamp_encode(ctx->port->L, cfg, ctx->stream, i);
+ luamp_encode(ctx->port->L, cfg, opts, ctx->stream, i);
ctx->port->size = size;
mpstream_flush(ctx->stream);
return 0;
diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
index b4843b2..b5db3c9 100644
--- a/src/box/lua/execute.c
+++ b/src/box/lua/execute.c
@@ -328,7 +328,7 @@ lua_sql_bind_decode(struct lua_State *L, struct sql_bind *bind, int idx, int i)
bind->name = NULL;
bind->name_len = 0;
}
- if (luaL_tofield(L, luaL_msgpack_default, -1, &field) < 0)
+ if (luaL_tofield(L, luaL_msgpack_default, NULL, -1, &field) < 0)
return -1;
switch (field.type) {
case MP_UINT:
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 4b0701e..aba906d 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -120,7 +120,7 @@ luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
int argc = lua_gettop(L);
mpstream_encode_array(&stream, argc);
for (int k = 1; k <= argc; ++k) {
- luamp_encode(L, luaL_msgpack_default, &stream, k);
+ luamp_encode(L, luaL_msgpack_default, NULL, &stream, k);
}
} else {
/* Create the tuple from a Lua table. */
@@ -252,18 +252,18 @@ luamp_convert_key(struct lua_State *L, struct luaL_serializer *cfg,
return tuple_to_mpstream(tuple, stream);
struct luaL_field field;
- if (luaL_tofield(L, cfg, index, &field) < 0)
+ if (luaL_tofield(L, cfg, NULL, index, &field) < 0)
luaT_error(L);
if (field.type == MP_ARRAY) {
lua_pushvalue(L, index);
- luamp_encode_r(L, cfg, stream, &field, 0);
+ luamp_encode_r(L, cfg, NULL, stream, &field, 0);
lua_pop(L, 1);
} else if (field.type == MP_NIL) {
mpstream_encode_array(stream, 0);
} else {
mpstream_encode_array(stream, 1);
lua_pushvalue(L, index);
- luamp_encode_r(L, cfg, stream, &field, 0);
+ luamp_encode_r(L, cfg, NULL, stream, &field, 0);
lua_pop(L, 1);
}
}
@@ -275,7 +275,7 @@ luamp_encode_tuple(struct lua_State *L, struct luaL_serializer *cfg,
struct tuple *tuple = luaT_istuple(L, index);
if (tuple != NULL) {
return tuple_to_mpstream(tuple, stream);
- } else if (luamp_encode(L, cfg, stream, index) != MP_ARRAY) {
+ } else if (luamp_encode(L, cfg, NULL, stream, index) != MP_ARRAY) {
diag_set(ClientError, ER_TUPLE_NOT_ARRAY);
luaT_error(L);
}
@@ -435,7 +435,7 @@ lbox_tuple_transform(struct lua_State *L)
mpstream_encode_array(&stream, 3);
mpstream_encode_str(&stream, "!");
mpstream_encode_uint(&stream, offset);
- luamp_encode(L, luaL_msgpack_default, &stream, i);
+ luamp_encode(L, luaL_msgpack_default, NULL, &stream, i);
}
mpstream_flush(&stream);
diff --git a/src/box/session.cc b/src/box/session.cc
index b557eed..08a1092 100644
--- a/src/box/session.cc
+++ b/src/box/session.cc
@@ -275,6 +275,9 @@ session_find(uint64_t sid)
mh_i64ptr_node(session_registry, k)->val;
}
+extern "C" void
+session_settings_init(void);
+
void
session_init()
{
@@ -283,7 +286,7 @@ session_init()
panic("out of memory");
mempool_create(&session_pool, &cord()->slabc, sizeof(struct session));
credentials_create(&admin_credentials, admin_user);
- sql_session_settings_init();
+ session_settings_init();
}
void
diff --git a/src/box/session.h b/src/box/session.h
index 8693799..500a88b 100644
--- a/src/box/session.h
+++ b/src/box/session.h
@@ -36,13 +36,12 @@
#include "fiber.h"
#include "user.h"
#include "authentication.h"
+#include "serializer_opts.h"
#if defined(__cplusplus)
extern "C" {
#endif /* defined(__cplusplus) */
-extern void sql_session_settings_init();
-
struct port;
struct session_vtab;
@@ -90,6 +89,8 @@ struct session_meta {
};
/** Console output format. */
enum output_format output_format;
+ /** Session-specific serialization options. */
+ struct serializer_opts serializer_opts;
};
/**
diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index 79c4b8d..dbbbf24 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -42,6 +42,7 @@ struct session_setting session_settings[SESSION_SETTING_COUNT] = {};
/** Corresponding names of session settings. */
const char *session_setting_strs[SESSION_SETTING_COUNT] = {
+ "error_marshaling_enabled",
"sql_default_engine",
"sql_defer_foreign_keys",
"sql_full_column_names",
@@ -449,3 +450,58 @@ session_setting_find(const char *name) {
else
return -1;
}
+
+/* Module independent session settings. */
+
+static void
+session_setting_error_marshaling_enabled_get(int id, const char **mp_pair,
+ const char **mp_pair_end)
+{
+ assert(id == SESSION_SETTING_ERROR_MARSHALING_ENABLED);
+ struct session *session = current_session();
+ const char *name = session_setting_strs[id];
+ size_t name_len = strlen(name);
+ bool value = session->meta.serializer_opts.error_marshaling_enabled;
+ size_t size = mp_sizeof_array(2) + mp_sizeof_str(name_len) +
+ mp_sizeof_bool(value);
+
+ char *pos = (char*)static_alloc(size);
+ assert(pos != NULL);
+ char *pos_end = mp_encode_array(pos, 2);
+ pos_end = mp_encode_str(pos_end, name, name_len);
+ pos_end = mp_encode_bool(pos_end, value);
+ *mp_pair = pos;
+ *mp_pair_end = pos_end;
+}
+
+static int
+session_setting_error_marshaling_enabled_set(int id, const char *mp_value)
+{
+ assert(id == SESSION_SETTING_ERROR_MARSHALING_ENABLED);
+ enum mp_type mtype = mp_typeof(*mp_value);
+ enum field_type stype = session_settings[id].field_type;
+ if (mtype != MP_BOOL) {
+ diag_set(ClientError, ER_SESSION_SETTING_INVALID_VALUE,
+ session_setting_strs[id], field_type_strs[stype]);
+ return -1;
+ }
+ struct session *session = current_session();
+ session->meta.serializer_opts.error_marshaling_enabled =
+ mp_decode_bool(&mp_value);
+ return 0;
+}
+
+extern void
+sql_session_settings_init();
+
+void
+session_settings_init(void)
+{
+ struct session_setting *s =
+ &session_settings[SESSION_SETTING_ERROR_MARSHALING_ENABLED];
+ s->field_type = FIELD_TYPE_BOOLEAN;
+ s->get = session_setting_error_marshaling_enabled_get;
+ s->set = session_setting_error_marshaling_enabled_set;
+
+ sql_session_settings_init();
+}
diff --git a/src/box/session_settings.h b/src/box/session_settings.h
index e2adc52..6560b8c 100644
--- a/src/box/session_settings.h
+++ b/src/box/session_settings.h
@@ -42,6 +42,7 @@
* space iterator will not be sorted properly.
*/
enum {
+ SESSION_SETTING_ERROR_MARSHALING_ENABLED,
SESSION_SETTING_SQL_BEGIN,
SESSION_SETTING_SQL_DEFAULT_ENGINE = SESSION_SETTING_SQL_BEGIN,
SESSION_SETTING_SQL_DEFER_FOREIGN_KEYS,
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index e3a6e88..3768372 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -257,8 +257,10 @@ port_lua_get_vdbemem(struct port *base, uint32_t *size)
return NULL;
for (int i = 0; i < argc; i++) {
struct luaL_field field;
- if (luaL_tofield(L, luaL_msgpack_default, -1 - i, &field) < 0)
+ if (luaL_tofield(L, luaL_msgpack_default,
+ NULL, -1 - i, &field) < 0) {
goto error;
+ }
switch (field.type) {
case MP_BOOL:
mem_set_bool(&val[i], field.bval);
diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
index e4fb0cf..5c1bf8e 100644
--- a/src/lua/msgpack.c
+++ b/src/lua/msgpack.c
@@ -109,8 +109,8 @@ luamp_set_decode_extension(luamp_decode_extension_f handler)
enum mp_type
luamp_encode_r(struct lua_State *L, struct luaL_serializer *cfg,
- struct mpstream *stream, struct luaL_field *field,
- int level)
+ const struct serializer_opts *opts, struct mpstream *stream,
+ struct luaL_field *field, int level)
{
int top = lua_gettop(L);
enum mp_type type;
@@ -155,13 +155,13 @@ restart: /* used by MP_EXT of unidentified subtype */
lua_pushnil(L); /* first key */
while (lua_next(L, top) != 0) {
lua_pushvalue(L, -2); /* push a copy of key to top */
- if (luaL_tofield(L, cfg, lua_gettop(L), field) < 0)
+ if (luaL_tofield(L, cfg, opts, lua_gettop(L), field) < 0)
return luaT_error(L);
- luamp_encode_r(L, cfg, stream, field, level + 1);
+ luamp_encode_r(L, cfg, opts, stream, field, level + 1);
lua_pop(L, 1); /* pop a copy of key */
- if (luaL_tofield(L, cfg, lua_gettop(L), field) < 0)
+ if (luaL_tofield(L, cfg, opts, lua_gettop(L), field) < 0)
return luaT_error(L);
- luamp_encode_r(L, cfg, stream, field, level + 1);
+ luamp_encode_r(L, cfg, opts, stream, field, level + 1);
lua_pop(L, 1); /* pop value */
}
assert(lua_gettop(L) == top);
@@ -180,9 +180,9 @@ restart: /* used by MP_EXT of unidentified subtype */
mpstream_encode_array(stream, size);
for (uint32_t i = 0; i < size; i++) {
lua_rawgeti(L, top, i + 1);
- if (luaL_tofield(L, cfg, top + 1, field) < 0)
+ if (luaL_tofield(L, cfg, opts, top + 1, field) < 0)
return luaT_error(L);
- luamp_encode_r(L, cfg, stream, field, level + 1);
+ luamp_encode_r(L, cfg, opts, stream, field, level + 1);
lua_pop(L, 1);
}
assert(lua_gettop(L) == top);
@@ -213,7 +213,8 @@ restart: /* used by MP_EXT of unidentified subtype */
enum mp_type
luamp_encode(struct lua_State *L, struct luaL_serializer *cfg,
- struct mpstream *stream, int index)
+ const struct serializer_opts *opts, struct mpstream *stream,
+ int index)
{
int top = lua_gettop(L);
if (index < 0)
@@ -225,9 +226,9 @@ luamp_encode(struct lua_State *L, struct luaL_serializer *cfg,
}
struct luaL_field field;
- if (luaL_tofield(L, cfg, lua_gettop(L), &field) < 0)
+ if (luaL_tofield(L, cfg, opts, lua_gettop(L), &field) < 0)
return luaT_error(L);
- enum mp_type top_type = luamp_encode_r(L, cfg, stream, &field, 0);
+ enum mp_type top_type = luamp_encode_r(L, cfg, opts, stream, &field, 0);
if (!on_top) {
lua_remove(L, top + 1); /* remove a value copy */
@@ -369,7 +370,7 @@ lua_msgpack_encode(lua_State *L)
mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb,
luamp_error, L);
- luamp_encode(L, cfg, &stream, 1);
+ luamp_encode(L, cfg, NULL, &stream, 1);
mpstream_flush(&stream);
if (index > 1) {
diff --git a/src/lua/msgpack.h b/src/lua/msgpack.h
index d0ade30..5a91e28 100644
--- a/src/lua/msgpack.h
+++ b/src/lua/msgpack.h
@@ -43,6 +43,7 @@ extern "C" {
struct luaL_serializer;
struct mpstream;
+struct serializer_opts;
/**
* Default instance of msgpack serializer (msgpack = require('msgpack')).
@@ -63,12 +64,13 @@ enum { LUAMP_ALLOC_FACTOR = 256 };
/* low-level function needed for execute_lua_call() */
enum mp_type
luamp_encode_r(struct lua_State *L, struct luaL_serializer *cfg,
- struct mpstream *stream, struct luaL_field *field,
- int level);
+ const struct serializer_opts *opts, struct mpstream *stream,
+ struct luaL_field *field, int level);
enum mp_type
luamp_encode(struct lua_State *L, struct luaL_serializer *cfg,
- struct mpstream *stream, int index);
+ const struct serializer_opts *opts, struct mpstream *stream,
+ int index);
void
luamp_decode(struct lua_State *L, struct luaL_serializer *cfg,
diff --git a/src/lua/utils.c b/src/lua/utils.c
index 667365f..42a226c 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -452,7 +452,7 @@ lua_field_inspect_ucdata(struct lua_State *L, struct luaL_serializer *cfg,
lua_pcall(L, 1, 1, 0);
/* replace obj with the unpacked value */
lua_replace(L, idx);
- if (luaL_tofield(L, cfg, idx, field) < 0)
+ if (luaL_tofield(L, cfg, NULL, idx, field) < 0)
luaT_error(L);
} /* else ignore lua_gettable exceptions */
lua_settop(L, top); /* remove temporary objects */
@@ -515,7 +515,7 @@ lua_field_try_serialize(struct lua_State *L)
/* copy object itself */
lua_pushvalue(L, 1);
lua_call(L, 1, 1);
- s->is_error = (luaL_tofield(L, cfg, -1, field) != 0);
+ s->is_error = (luaL_tofield(L, cfg, NULL, -1, field) != 0);
s->is_value_returned = true;
return 1;
}
@@ -641,14 +641,17 @@ lua_field_tostring(struct lua_State *L, struct luaL_serializer *cfg, int idx,
lua_call(L, 1, 1);
lua_replace(L, idx);
lua_settop(L, top);
- if (luaL_tofield(L, cfg, idx, field) < 0)
+ if (luaL_tofield(L, cfg, NULL, idx, field) < 0)
luaT_error(L);
}
int
-luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
+luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg,
+ const struct serializer_opts *opts, int index,
struct luaL_field *field)
{
+ /* opts will be used for encode MP_ERROR in the future */
+ (void)opts;
if (index < 0)
index = lua_gettop(L) + index + 1;
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 4bc0417..b10754e 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -59,6 +59,7 @@ extern "C" {
#include "lib/core/mp_extension_types.h"
#include "lib/core/decimal.h" /* decimal_t */
+struct serializer_opts;
struct lua_State;
struct ibuf;
struct tt_uuid;
@@ -366,6 +367,7 @@ struct luaL_field {
*
* @param L stack
* @param cfg configuration
+ * @param opts the Lua serializer additional options.
* @param index stack index
* @param field conversion result
*
@@ -373,7 +375,8 @@ struct luaL_field {
* @retval -1 Error.
*/
int
-luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
+luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg,
+ const struct serializer_opts *opts, int index,
struct luaL_field *field);
/**
@@ -412,7 +415,7 @@ static inline void
luaL_checkfield(struct lua_State *L, struct luaL_serializer *cfg, int idx,
struct luaL_field *field)
{
- if (luaL_tofield(L, cfg, idx, field) < 0)
+ if (luaL_tofield(L, cfg, NULL, idx, field) < 0)
luaT_error(L);
if (field->type != MP_EXT || field->ext_type != MP_UNKNOWN_EXTENSION)
return;
diff --git a/src/serializer_opts.h b/src/serializer_opts.h
new file mode 100644
index 0000000..9e2c15e
--- /dev/null
+++ b/src/serializer_opts.h
@@ -0,0 +1,44 @@
+#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.
+ */
+
+/**
+ * Serializer options which can regulate how to serialize
+ * something in scope of one session.
+ */
+struct serializer_opts {
+ /**
+ * When enabled, error objects get their own MP_EXT
+ * MessagePack type and therefore can be type-safely
+ * transmitted over the network.
+ */
+ bool error_marshaling_enabled;
+};
diff --git a/test/box/session_settings.result b/test/box/session_settings.result
index ea6302d..149cc4b 100644
--- a/test/box/session_settings.result
+++ b/test/box/session_settings.result
@@ -52,7 +52,8 @@ s:replace({'sql_defer_foreign_keys', true})
--
s:select()
| ---
- | - - ['sql_default_engine', 'memtx']
+ | - - ['error_marshaling_enabled', false]
+ | - ['sql_default_engine', 'memtx']
| - ['sql_defer_foreign_keys', false]
| - ['sql_full_column_names', false]
| - ['sql_full_metadata', false]
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH V6 04/10] error: update constructors of some errors
2020-04-19 22:25 [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Leonid Vasiliev
` (2 preceding siblings ...)
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 03/10] error: add session setting for error type marshaling Leonid Vasiliev
@ 2020-04-19 22:25 ` Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 05/10] box: move Lua MP_EXT decoder from tuple.c Leonid Vasiliev
` (7 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Leonid Vasiliev @ 2020-04-19 22:25 UTC (permalink / raw)
To: v.shpilevoy, alexander.turenko; +Cc: tarantool-patches
We want to have a transparent marshalling through net.box
for errors. To do this, we need to recreate the error
on the client side with the same parameters as on the server.
For convenience, we update AccessDeniedError constructor
which has pointers to static strings and add the XlogGapError
constructor that does not require vclock.
Needed for #4398
---
src/box/error.cc | 24 +++++++++++++++---------
src/box/error.h | 11 ++++++++---
2 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/src/box/error.cc b/src/box/error.cc
index 277727d..c3c2af3 100644
--- a/src/box/error.cc
+++ b/src/box/error.cc
@@ -255,6 +255,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)
@@ -283,23 +290,22 @@ AccessDeniedError::AccessDeniedError(const char *file, unsigned int line,
const char *access_type,
const char *object_type,
const char *object_name,
- const char *user_name)
+ const char *user_name,
+ bool run_trigers)
:ClientError(&type_AccessDeniedError, file, line, ER_ACCESS_DENIED)
{
error_format_msg(this, tnt_errcode_desc(m_errcode),
access_type, object_type, object_name, user_name);
struct on_access_denied_ctx ctx = {access_type, object_type, object_name};
- trigger_run(&on_access_denied, (void *) &ctx);
/*
- * We want to use ctx parameters as error parameters
- * later, so we have to alloc space for it.
- * As m_access_type and m_object_type are constant
- * literals they are statically allocated. We must copy
- * only m_object_name.
+ * Don't run the triggers when create after marshaling
+ * through network.
*/
- m_object_type = object_type;
- m_access_type = access_type;
+ if (run_trigers)
+ trigger_run(&on_access_denied, (void *) &ctx);
+ m_object_type = strdup(object_type);
+ m_access_type = strdup(access_type);
m_object_name = strdup(object_name);
}
diff --git a/src/box/error.h b/src/box/error.h
index 461ca0f..988b982 100644
--- a/src/box/error.h
+++ b/src/box/error.h
@@ -241,11 +241,14 @@ class AccessDeniedError: public ClientError
public:
AccessDeniedError(const char *file, unsigned int line,
const char *access_type, const char *object_type,
- const char *object_name, const char *user_name);
+ const char *object_name, const char *user_name,
+ bool run_trigers = true);
~AccessDeniedError()
{
free(m_object_name);
+ free(m_object_type);
+ free(m_access_type);
}
const char *
@@ -268,11 +271,11 @@ public:
private:
/** Type of object the required access was denied to */
- const char *m_object_type;
+ char *m_object_type;
/** Name of object the required access was denied to */
char *m_object_name;
/** Type of declined access */
- const char *m_access_type;
+ char *m_access_type;
};
/**
@@ -302,6 +305,8 @@ struct XlogGapError: public XlogError
{
XlogGapError(const char *file, unsigned line,
const struct vclock *from, const struct vclock *to);
+ XlogGapError(const char *file, unsigned line,
+ const char *msg);
virtual void raise() { throw this; }
};
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH V6 05/10] box: move Lua MP_EXT decoder from tuple.c
2020-04-19 22:25 [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Leonid Vasiliev
` (3 preceding siblings ...)
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 04/10] error: update constructors of some errors Leonid Vasiliev
@ 2020-04-19 22:25 ` Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 06/10] error: add error MsgPack encoding Leonid Vasiliev
` (6 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Leonid Vasiliev @ 2020-04-19 22:25 UTC (permalink / raw)
To: v.shpilevoy, alexander.turenko; +Cc: tarantool-patches
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Lua C module 'msgpack' supports registration of custom extension
decoders for MP_EXT values. That is needed to make 'msgpack' not
depending on any modules which use it.
So far the only box-related extension were tuples - struct tuple
cdata needed to be encoded as an array.
That is going to change in next commits, where struct error cdata
appears, also depending on box. So the decoder can't be located
in src/box/lua/tuple.c. It is moved to a more common place -
src/box/lua/init.c.
Needed for #4398
---
src/box/lua/init.c | 18 ++++++++++++++++++
src/box/lua/tuple.c | 16 ----------------
2 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index 0a65c3b..7899c16 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -36,6 +36,7 @@
#include "lua/utils.h" /* luaT_error() */
#include "lua/trigger.h"
+#include "lua/msgpack.h"
#include "box/box.h"
#include "box/txn.h"
@@ -395,6 +396,21 @@ static const struct luaL_Reg boxlib_backup[] = {
{NULL, NULL}
};
+/**
+ * A MsgPack extensions handler, for types defined in box.
+ */
+static enum mp_type
+luamp_encode_extension_box(struct lua_State *L, int idx,
+ struct mpstream *stream)
+{
+ struct tuple *tuple = luaT_istuple(L, idx);
+ if (tuple != NULL) {
+ tuple_to_mpstream(tuple, stream);
+ return MP_ARRAY;
+ }
+ return MP_EXT;
+}
+
#include "say.h"
void
@@ -435,6 +451,8 @@ box_lua_init(struct lua_State *L)
luaopen_merger(L);
lua_pop(L, 1);
+ luamp_set_encode_extension(luamp_encode_extension_box);
+
/* Load Lua extension */
for (const char **s = lua_sources; *s; s += 2) {
const char *modname = *s;
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index aba906d..03b4b8a 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -290,20 +290,6 @@ tuple_to_mpstream(struct tuple *tuple, struct mpstream *stream)
mpstream_advance(stream, bsize);
}
-/* A MsgPack extensions handler that supports tuples */
-static enum mp_type
-luamp_encode_extension_box(struct lua_State *L, int idx,
- struct mpstream *stream)
-{
- struct tuple *tuple = luaT_istuple(L, idx);
- if (tuple != NULL) {
- tuple_to_mpstream(tuple, stream);
- return MP_ARRAY;
- }
-
- return MP_EXT;
-}
-
/**
* Convert a tuple into lua table. Named fields are stored as
* {name = value} pairs. Not named fields are stored as
@@ -582,8 +568,6 @@ box_lua_tuple_init(struct lua_State *L)
luaL_register_module(L, tuplelib_name, lbox_tuplelib);
lua_pop(L, 1);
- luamp_set_encode_extension(luamp_encode_extension_box);
-
tuple_serializer_update_options();
trigger_create(&tuple_serializer.update_trigger,
on_msgpack_serializer_update, NULL, NULL);
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH V6 06/10] error: add error MsgPack encoding
2020-04-19 22:25 [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Leonid Vasiliev
` (4 preceding siblings ...)
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 05/10] box: move Lua MP_EXT decoder from tuple.c Leonid Vasiliev
@ 2020-04-19 22:25 ` Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 07/10] error: export error_unref() function Leonid Vasiliev
` (5 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Leonid Vasiliev @ 2020-04-19 22:25 UTC (permalink / raw)
To: v.shpilevoy, alexander.turenko; +Cc: tarantool-patches
Co-authored-by: Vladislav Shpilevoy<v.shpilevoy@tarantool.org>
Closes #4398
@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 | 6 +-
src/box/lua/init.c | 36 +++
src/box/mp_error.cc | 533 +++++++++++++++++++++++++++++++++++
src/box/mp_error.h | 60 ++++
src/lib/core/mp_extension_types.h | 1 +
src/lua/error.c | 2 +-
src/lua/error.h | 1 +
src/lua/msgpack.c | 2 +
src/lua/utils.c | 8 +-
test/box-tap/extended_error.test.lua | 124 ++++++++
test/unit/CMakeLists.txt | 2 +
test/unit/mp_error.cc | 473 +++++++++++++++++++++++++++++++
test/unit/mp_error.result | 44 +++
13 files changed, 1286 insertions(+), 6 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
create mode 100644 test/unit/mp_error.cc
create mode 100644 test/unit/mp_error.result
diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 5ed7eae..6ca540d 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -34,15 +34,15 @@ 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)
-target_link_libraries(box_error core stat)
+add_library(box_error STATIC error.cc errcode.c vclock.c mp_error.cc)
+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})
add_library(tuple STATIC
tuple.c
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index 7899c16..cac62b9 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -42,6 +42,8 @@
#include "box/txn.h"
#include "box/func.h"
#include "box/vclock.h"
+#include "box/session.h"
+#include "box/mp_error.h"
#include "box/lua/error.h"
#include "box/lua/tuple.h"
@@ -64,6 +66,8 @@
#include "box/lua/key_def.h"
#include "box/lua/merger.h"
+#include "mpstream/mpstream.h"
+
static uint32_t CTID_STRUCT_TXN_SAVEPOINT_PTR = 0;
extern char session_lua[],
@@ -408,9 +412,40 @@ luamp_encode_extension_box(struct lua_State *L, int idx,
tuple_to_mpstream(tuple, stream);
return MP_ARRAY;
}
+ struct error *err = luaL_iserror(L, idx);
+ struct serializer_opts *opts = ¤t_session()->meta.serializer_opts;
+ if (err != NULL && opts->error_marshaling_enabled)
+ error_to_mpstream(err, stream);
+
return MP_EXT;
}
+/**
+ * A MsgPack extensions handler that supports errors decode.
+ */
+static void
+luamp_decode_extension_box(struct lua_State *L, const char **data)
+{
+ assert(mp_typeof(**data) == MP_EXT);
+ int8_t ext_type;
+ uint32_t len = mp_decode_extl(data, &ext_type);
+
+ if (ext_type != MP_ERROR) {
+ luaL_error(L, "Unsuported MsgPack extension type: %u",
+ ext_type);
+ return;
+ }
+
+ struct error *err = error_unpack(data, len);
+ if (err == NULL) {
+ luaL_error(L, "Can not parse an error from MsgPack");
+ return;
+ }
+
+ luaT_pusherror(L, err);
+ return;
+}
+
#include "say.h"
void
@@ -452,6 +487,7 @@ box_lua_init(struct lua_State *L)
lua_pop(L, 1);
luamp_set_encode_extension(luamp_encode_extension_box);
+ luamp_set_decode_extension(luamp_decode_extension_box);
/* Load Lua extension */
for (const char **s = lua_sources; *s; s += 2) {
diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
new file mode 100644
index 0000000..e7d2808
--- /dev/null
+++ b/src/box/mp_error.cc
@@ -0,0 +1,533 @@
+/*
+ * 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/mp_error.h"
+#include "box/error.h"
+#include "mpstream/mpstream.h"
+#include "msgpuck.h"
+#include "mp_extension_types.h"
+#include "fiber.h"
+
+/**
+ * MP_ERROR format:
+ *
+ * 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>: ...,
+ * ...
+ * },
+ * ...
+ * },
+ * ...
+ * ]
+ * }
+ */
+
+/**
+ * MP_ERROR keys
+ */
+enum {
+ MP_ERROR_STACK = 0x00
+};
+
+/**
+ * Keys of individual error in the stack.
+ */
+enum {
+ /** Error type. */
+ MP_ERROR_TYPE = 0x00,
+ /** File name from trace. */
+ MP_ERROR_FILE = 0x01,
+ /** Line from trace. */
+ MP_ERROR_LINE = 0x02,
+ /** Error message. */
+ MP_ERROR_MESSAGE = 0x03,
+ /** Errno at the moment of error creation. */
+ MP_ERROR_ERRNO = 0x04,
+ /** Error code. */
+ MP_ERROR_CODE = 0x05,
+ /*
+ * Type-specific fields stored as a map
+ * {string key = value}.
+ */
+ MP_ERROR_FIELDS = 0x06
+};
+
+/**
+ * The structure is used for storing parameters
+ * during decoding MP_ERROR.
+ */
+struct mp_error {
+ uint32_t code;
+ uint32_t line;
+ uint32_t saved_errno;
+ const char *type;
+ const char *file;
+ const char *message;
+ const char *custom_type;
+ const char *ad_object_type;
+ const char *ad_object_name;
+ const char *ad_access_type;
+};
+
+static void
+mp_error_create(struct mp_error *mp_error)
+{
+ memset(mp_error, 0, sizeof(*mp_error));
+}
+
+static uint32_t
+mp_sizeof_error(const struct error *error)
+{
+ uint32_t errcode = box_error_code(error);
+
+ bool is_custom = false;
+ bool is_access_denied = false;
+
+ 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 = 6;
+ uint32_t data_size = 0;
+
+ data_size += mp_sizeof_uint(MP_ERROR_TYPE);
+ data_size += mp_sizeof_str(strlen(error->type->name));
+ data_size += mp_sizeof_uint(MP_ERROR_LINE);
+ 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_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_access_denied) {
+ ++details_num;
+ data_size += mp_sizeof_uint(MP_ERROR_FIELDS);
+ data_size += mp_sizeof_map(3);
+ AccessDeniedError *ad_err = type_cast(AccessDeniedError, error);
+ data_size += mp_sizeof_str(strlen("object_type"));
+ data_size += mp_sizeof_str(strlen(ad_err->object_type()));
+ data_size += mp_sizeof_str(strlen("object_name"));
+ data_size += mp_sizeof_str(strlen(ad_err->object_name()));
+ 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);
+ 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)));
+ }
+
+ data_size += mp_sizeof_map(details_num);
+
+ return data_size;
+}
+
+static inline char *
+mp_encode_str0(char *data, const char *str)
+{
+ return mp_encode_str(data, str, strlen(str));
+}
+
+static void
+mp_encode_error_one(const struct error *error, char **data)
+{
+ uint32_t errcode = box_error_code(error);
+
+ bool is_custom = false;
+ bool is_access_denied = false;
+
+ 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 = 6;
+ if (is_access_denied || is_custom)
+ ++details_num;
+
+ *data = mp_encode_map(*data, details_num);
+ *data = mp_encode_uint(*data, MP_ERROR_TYPE);
+ *data = mp_encode_str0(*data, error->type->name);
+ *data = mp_encode_uint(*data, MP_ERROR_LINE);
+ *data = mp_encode_uint(*data, error->line);
+ *data = mp_encode_uint(*data, MP_ERROR_FILE);
+ *data = mp_encode_str0(*data, error->file);
+ *data = mp_encode_uint(*data, MP_ERROR_MESSAGE);
+ *data = mp_encode_str0(*data, 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_access_denied) {
+ *data = mp_encode_uint(*data, MP_ERROR_FIELDS);
+ *data = mp_encode_map(*data, 3);
+ AccessDeniedError *ad_err = type_cast(AccessDeniedError, error);
+ *data = mp_encode_str0(*data, "object_type");
+ *data = mp_encode_str0(*data, ad_err->object_type());
+ *data = mp_encode_str0(*data, "object_name");
+ *data = mp_encode_str0(*data, ad_err->object_name());
+ *data = mp_encode_str0(*data, "access_type");
+ *data = mp_encode_str0(*data, ad_err->access_type());
+ } else if (is_custom) {
+ *data = mp_encode_uint(*data, MP_ERROR_FIELDS);
+ *data = mp_encode_map(*data, 1);
+ *data = mp_encode_str0(*data, "custom_type");
+ *data = mp_encode_str0(*data, box_error_custom_type(error));
+ }
+}
+
+static struct error *
+error_build_xc(struct mp_error *mp_error)
+{
+ /*
+ * 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 (mp_error->type == NULL || mp_error->message == NULL ||
+ mp_error->file == NULL) {
+missing_fields:
+ diag_set(ClientError, ER_INVALID_MSGPACK,
+ "Missing mandatory error fields");
+ return NULL;
+ }
+
+ if (strcmp(mp_error->type, "ClientError") == 0) {
+ ClientError *e = new ClientError(mp_error->file, mp_error->line,
+ ER_UNKNOWN);
+ e->m_errcode = mp_error->code;
+ err = e;
+ } else if (strcmp(mp_error->type, "CustomError") == 0) {
+ if (mp_error->custom_type == NULL)
+ goto missing_fields;
+ err = new CustomError(mp_error->file, mp_error->line,
+ mp_error->custom_type, mp_error->code);
+ } else if (strcmp(mp_error->type, "AccessDeniedError") == 0) {
+ if (mp_error->ad_access_type == NULL ||
+ mp_error->ad_object_type == NULL ||
+ mp_error->ad_object_name == NULL)
+ goto missing_fields;
+ err = new AccessDeniedError(mp_error->file, mp_error->line,
+ mp_error->ad_access_type,
+ 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);
+ } else if (strcmp(mp_error->type, "XlogGapError") == 0) {
+ err = new XlogGapError(mp_error->file, mp_error->line,
+ mp_error->message);
+ } else if (strcmp(mp_error->type, "SystemError") == 0) {
+ err = new SystemError(mp_error->file, mp_error->line,
+ "%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->message);
+ } else if (strcmp(mp_error->type, "OutOfMemory") == 0) {
+ err = new OutOfMemory(mp_error->file, mp_error->line,
+ 0, "", "");
+ } else if (strcmp(mp_error->type, "TimedOut") == 0) {
+ err = new TimedOut(mp_error->file, mp_error->line);
+ } else if (strcmp(mp_error->type, "ChannelIsClosed") == 0) {
+ err = new ChannelIsClosed(mp_error->file, mp_error->line);
+ } else if (strcmp(mp_error->type, "FiberIsCancelled") == 0) {
+ err = new FiberIsCancelled(mp_error->file, mp_error->line);
+ } else if (strcmp(mp_error->type, "LuajitError") == 0) {
+ err = new LuajitError(mp_error->file, mp_error->line,
+ mp_error->message);
+ } else if (strcmp(mp_error->type, "IllegalParams") == 0) {
+ err = new IllegalParams(mp_error->file, mp_error->line,
+ "%s", mp_error->message);
+ } else if (strcmp(mp_error->type, "CollationError") == 0) {
+ err = new CollationError(mp_error->file, mp_error->line,
+ "%s", mp_error->message);
+ } else if (strcmp(mp_error->type, "SwimError") == 0) {
+ err = new SwimError(mp_error->file, mp_error->line,
+ "%s", mp_error->message);
+ } else if (strcmp(mp_error->type, "CryptoError") == 0) {
+ err = new CryptoError(mp_error->file, mp_error->line,
+ "%s", mp_error->message);
+ } else {
+ err = new ClientError(mp_error->file, mp_error->line,
+ ER_UNKNOWN);
+ }
+ err->saved_errno = mp_error->saved_errno;
+ error_format_msg(err, "%s", mp_error->message);
+ 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;
+}
+
+static inline const char *
+mp_decode_and_copy_str(const char **data, struct region *region)
+{
+ 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
+mp_decode_error_fields(const char **data, struct mp_error *mp_err,
+ struct region *region)
+{
+ if (mp_typeof(**data) != MP_MAP)
+ return -1;
+ uint32_t map_sz = mp_decode_map(data);
+ const char *key;
+ uint32_t key_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 (str_nonterm_is_eq("object_type", key, key_len)) {
+ mp_err->ad_object_type =
+ mp_decode_and_copy_str(data, region);
+ if (mp_err->ad_object_type == NULL)
+ return -1;
+ } else if (str_nonterm_is_eq("object_name", key, key_len)) {
+ mp_err->ad_object_name =
+ mp_decode_and_copy_str(data, region);
+ if (mp_err->ad_object_name == NULL)
+ return -1;
+ } else if (str_nonterm_is_eq("access_type", key, key_len)) {
+ mp_err->ad_access_type =
+ mp_decode_and_copy_str(data, region);
+ if (mp_err->ad_access_type == NULL)
+ return -1;
+ } else if (str_nonterm_is_eq("custom_type", key, key_len)) {
+ mp_err->custom_type =
+ mp_decode_and_copy_str(data, region);
+ if (mp_err->custom_type == NULL)
+ return -1;
+ } else {
+ mp_next(data);
+ }
+ }
+ return 0;
+}
+
+static struct error *
+mp_decode_error_one(const char **data)
+{
+ struct mp_error mp_err;
+ mp_error_create(&mp_err);
+ struct region *region = &fiber()->gc;
+ uint32_t region_svp = region_used(region);
+ 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)
+ goto error;
+
+ uint64_t key = mp_decode_uint(data);
+ switch(key) {
+ case MP_ERROR_TYPE:
+ mp_err.type = mp_decode_and_copy_str(data, region);
+ if (mp_err.type == NULL)
+ goto finish;
+ break;
+ case MP_ERROR_FILE:
+ mp_err.file = mp_decode_and_copy_str(data, region);
+ if (mp_err.file == NULL)
+ goto finish;
+ break;
+ case MP_ERROR_LINE:
+ if (mp_typeof(**data) != MP_UINT)
+ goto error;
+ mp_err.line = mp_decode_uint(data);
+ break;
+ case MP_ERROR_MESSAGE:
+ mp_err.message = mp_decode_and_copy_str(data, region);
+ if (mp_err.message == NULL)
+ goto finish;
+ break;
+ case MP_ERROR_ERRNO:
+ if (mp_typeof(**data) != MP_UINT)
+ goto error;
+ mp_err.saved_errno = mp_decode_uint(data);
+ break;
+ 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 (mp_decode_error_fields(data, &mp_err, region) != 0)
+ goto finish;
+ break;
+ default:
+ mp_next(data);
+ }
+ }
+
+ try {
+ err = error_build_xc(&mp_err);
+ } catch (OutOfMemory *e) {
+ assert(err == NULL && !diag_is_empty(diag_get()));
+ }
+finish:
+ region_truncate(region, region_svp);
+ return err;
+
+error:
+ diag_set(ClientError, ER_INVALID_MSGPACK,
+ "Invalid MP_ERROR MsgPack format");
+ goto finish;
+}
+
+void
+error_to_mpstream(const struct error *error, struct mpstream *stream)
+{
+ uint32_t err_cnt = 0;
+ uint32_t data_size = mp_sizeof_map(1);
+ data_size += mp_sizeof_uint(MP_ERROR_STACK);
+ for (const struct error *it = error; it != NULL; it = it->cause) {
+ err_cnt++;
+ data_size += mp_sizeof_error(it);
+ }
+
+ data_size += mp_sizeof_array(err_cnt);
+ uint32_t data_size_ext = mp_sizeof_ext(data_size);
+ char *ptr = mpstream_reserve(stream, data_size_ext);
+ char *data = ptr;
+ data = mp_encode_extl(data, MP_ERROR, data_size);
+ data = mp_encode_map(data, 1);
+ data = mp_encode_uint(data, MP_ERROR_STACK);
+ data = mp_encode_array(data, err_cnt);
+ for (const struct error *it = error; it != NULL; it = it->cause) {
+ mp_encode_error_one(it, &data);
+ }
+
+ assert(data == ptr + data_size_ext);
+ mpstream_advance(stream, data_size_ext);
+}
+
+struct error *
+error_unpack(const char **data, uint32_t len)
+{
+ const char *end = *data + len;
+ struct error *err = NULL;
+
+ if (mp_typeof(**data) != MP_MAP) {
+ diag_set(ClientError, ER_INVALID_MSGPACK,
+ "Invalid MP_ERROR MsgPack format");
+ return NULL;
+ }
+ uint32_t 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");
+ return NULL;
+ }
+ uint64_t key = mp_decode_uint(data);
+ switch(key) {
+ case MP_ERROR_STACK: {
+ if (mp_typeof(**data) != MP_ARRAY) {
+ diag_set(ClientError, ER_INVALID_MSGPACK,
+ "Invalid MP_ERROR MsgPack format");
+ return NULL;
+ }
+ uint32_t stack_sz = mp_decode_array(data);
+ struct error *effect = NULL;
+ for (uint32_t i = 0; i < stack_sz; i++) {
+ struct error *cur = mp_decode_error_one(data);
+ if (cur == NULL)
+ return NULL;
+ if (err == NULL) {
+ err = cur;
+ effect = cur;
+ continue;
+ }
+
+ error_set_prev(effect, cur);
+ effect = cur;
+ }
+ break;
+ }
+ default:
+ mp_next(data);
+ }
+ }
+
+ (void)end;
+ assert(*data == end);
+ return err;
+}
diff --git a/src/box/mp_error.h b/src/box/mp_error.h
new file mode 100644
index 0000000..5c746d9
--- /dev/null
+++ b/src/box/mp_error.h
@@ -0,0 +1,60 @@
+#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>
+
+struct mpstream;
+
+/**
+ * @brief Encode error to mpstream as MP_ERROR.
+ * @param error pointer to struct error for encoding.
+ * @param stream pointer to output stream.
+ */
+void
+error_to_mpstream(const struct error *error, struct mpstream *stream);
+
+/**
+ * @brief Unpack MP_ERROR to error.
+ * @param data pointer to MP_ERROR.
+ * @param len data size.
+ * @return struct error * or NULL if failed.
+ */
+struct error *
+error_unpack(const char **data, uint32_t len);
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
diff --git a/src/lib/core/mp_extension_types.h b/src/lib/core/mp_extension_types.h
index 7d42f21..e3ff9f5 100644
--- a/src/lib/core/mp_extension_types.h
+++ b/src/lib/core/mp_extension_types.h
@@ -43,6 +43,7 @@ enum mp_extension_type {
MP_UNKNOWN_EXTENSION = 0,
MP_DECIMAL = 1,
MP_UUID = 2,
+ MP_ERROR = 3,
mp_extension_type_MAX,
};
diff --git a/src/lua/error.c b/src/lua/error.c
index 18a990a..616aa7f 100644
--- a/src/lua/error.c
+++ b/src/lua/error.c
@@ -34,7 +34,7 @@
#include <fiber.h>
#include "utils.h"
-static int CTID_CONST_STRUCT_ERROR_REF = 0;
+uint32_t 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..54635bd 100644
--- a/src/lua/error.h
+++ b/src/lua/error.h
@@ -37,6 +37,7 @@
extern "C" {
#endif /* defined(__cplusplus) */
+extern uint32_t CTID_CONST_STRUCT_ERROR_REF;
/** \cond public */
struct error;
diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
index 5c1bf8e..d482c9b 100644
--- a/src/lua/msgpack.c
+++ b/src/lua/msgpack.c
@@ -195,6 +195,8 @@ restart: /* used by MP_EXT of unidentified subtype */
case MP_UUID:
mpstream_encode_uuid(stream, field->uuidval);
break;
+ case MP_ERROR:
+ return luamp_encode_extension(L, top, stream);
default:
/* Run trigger if type can't be encoded */
type = luamp_encode_extension(L, top, stream);
diff --git a/src/lua/utils.c b/src/lua/utils.c
index 42a226c..d410a3d 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -37,6 +37,8 @@
#include <diag.h>
#include <fiber.h>
+#include "serializer_opts.h"
+
int luaL_nil_ref = LUA_REFNIL;
int luaL_map_metatable_ref = LUA_REFNIL;
int luaL_array_metatable_ref = LUA_REFNIL;
@@ -650,8 +652,6 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg,
const struct serializer_opts *opts, int index,
struct luaL_field *field)
{
- /* opts will be used for encode MP_ERROR in the future */
- (void)opts;
if (index < 0)
index = lua_gettop(L) + index + 1;
@@ -759,6 +759,10 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg,
} else if (cd->ctypeid == CTID_UUID) {
field->ext_type = MP_UUID;
field->uuidval = (struct tt_uuid *) cdata;
+ } else if (cd->ctypeid == CTID_CONST_STRUCT_ERROR_REF &&
+ opts != NULL &&
+ opts->error_marshaling_enabled) {
+ field->ext_type = MP_ERROR;
} else {
field->ext_type = MP_UNKNOWN_EXTENSION;
}
diff --git a/test/box-tap/extended_error.test.lua b/test/box-tap/extended_error.test.lua
new file mode 100755
index 0000000..1681419
--- /dev/null
+++ b/test/box-tap/extended_error.test.lua
@@ -0,0 +1,124 @@
+#! /usr/bin/env tarantool
+
+local netbox = require('net.box')
+local os = require('os')
+local tap = require('tap')
+
+box.cfg{
+ listen = os.getenv('LISTEN')
+}
+
+--
+-- gh-4398: error objects after transmission through network
+-- should not loose their fields and type.
+--
+
+-- Create AccessDeniedError. It is going to be used in the tests
+-- below.
+function forbidden_function()
+ return nil
+end
+local user = box.session.user()
+box.session.su('admin')
+box.schema.func.create('forbidden_function')
+box.session.su('guest')
+local tmp = box.func.forbidden_function
+local access_denied_error
+tmp, access_denied_error = pcall(tmp.call, tmp)
+box.session.su('admin')
+box.schema.func.drop('forbidden_function')
+box.session.su(user)
+
+local test = tap.test('Error marshaling')
+test:plan(6)
+
+function error_new(...)
+ return box.error.new(...)
+end
+
+function error_new_stacked(args1, args2)
+ local e1 = box.error.new(args1)
+ local e2 = box.error.new(args2)
+ e1:set_prev(e2)
+ return e1
+end
+
+function error_access_denied()
+ return access_denied_error
+end
+
+local function check_error(err, check_list)
+ assert(type(check_list) == 'table')
+ if type(err.trace) ~= 'table' or err.trace[1] == nil or
+ err.trace[1].file == nil or err.trace[1].line == nil then
+ return false
+ end
+ for k, v in pairs(check_list) do
+ if err[k] ~= v then
+ return false
+ end
+ end
+ return true
+end
+
+box.schema.user.grant('guest', 'super')
+local c = netbox.connect(box.cfg.listen)
+c:eval('box.session.settings.error_marshaling_enabled = true')
+local err = c:call('error_new', {{code = 1000, reason = 'Reason'}})
+local checks = {
+ code = 1000,
+ message = 'Reason',
+ base_type = 'ClientError',
+ type = 'ClientError',
+}
+test:ok(check_error(err, checks), "ClientError marshaling")
+
+err = c:call('error_new', {{
+ code = 1001, reason = 'Reason2', type = 'MyError'
+}})
+checks = {
+ code = 1001,
+ message = 'Reason2',
+ base_type = 'CustomError',
+ type = 'MyError',
+}
+test:ok(check_error(err, checks), "CustomError marshaling")
+
+err = c:call('error_access_denied')
+checks = {
+ code = 42,
+ type = 'AccessDeniedError',
+ base_type = 'AccessDeniedError',
+ message = "Execute access to function 'forbidden_function' is denied for user 'guest'",
+ object_type = 'function',
+ object_name = 'forbidden_function',
+ access_type = 'Execute',
+}
+test:ok(check_error(err, checks), "AccessDeniedError marshaling")
+
+err = c:call('error_new_stacked', {
+ {code = 1003, reason = 'Reason3', type = 'MyError2'},
+ {code = 1004, reason = 'Reason4'}
+})
+local err1 = err
+local err2 = err.prev
+test:isnt(err2, nil, 'Stack is received')
+checks = {
+ code = 1003,
+ message = 'Reason3',
+ base_type = 'CustomError',
+ type = 'MyError2'
+}
+test:ok(check_error(err1, checks), "First error in the stack")
+checks = {
+ code = 1004,
+ message = 'Reason4',
+ base_type = 'ClientError',
+ type = 'ClientError'
+}
+test:ok(check_error(err2, checks), "Second error in the stack")
+
+c:close()
+box.schema.user.revoke('guest', 'super')
+
+os.exit(test:check() and 0 or 1)
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index e1d506f..24586c2 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -69,6 +69,8 @@ add_executable(xrow.test xrow.cc)
target_link_libraries(xrow.test xrow unit)
add_executable(decimal.test decimal.c)
target_link_libraries(decimal.test core unit)
+add_executable(mp_error.test mp_error.cc)
+target_link_libraries(mp_error.test box_error core unit)
add_executable(fiber.test fiber.cc)
set_source_files_properties(fiber.cc PROPERTIES COMPILE_FLAGS -O0)
diff --git a/test/unit/mp_error.cc b/test/unit/mp_error.cc
new file mode 100644
index 0000000..e02deac
--- /dev/null
+++ b/test/unit/mp_error.cc
@@ -0,0 +1,473 @@
+/*
+ * Copyright 2010-2020, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and iproto 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 iproto 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 "unit.h"
+#include "exception.h"
+#include "fiber.h"
+#include "memory.h"
+#include "msgpuck.h"
+#include "tt_static.h"
+
+#include "box/error.h"
+#include "box/mp_error.h"
+
+enum {
+ MP_ERROR_STACK = 0x00
+};
+
+enum {
+ MP_ERROR_TYPE = 0x00,
+ MP_ERROR_FILE = 0x01,
+ MP_ERROR_LINE = 0x02,
+ MP_ERROR_MESSAGE = 0x03,
+ MP_ERROR_ERRNO = 0x04,
+ MP_ERROR_CODE = 0x05,
+ MP_ERROR_FIELDS = 0x06
+};
+
+struct mp_error {
+ uint32_t code;
+ uint32_t line;
+ int32_t saved_errno;
+ uint32_t unknown_uint_field;
+ const char *type;
+ const char *file;
+ const char *message;
+ const char *custom_type;
+ const char *ad_object_type;
+ const char *ad_object_name;
+ const char *ad_access_type;
+ const char *unknown_str_field;
+};
+
+const char *standard_errors[] = {
+ "XlogError",
+ "XlogGapError",
+ "SystemError",
+ "SocketError",
+ "OutOfMemory",
+ "TimedOut",
+ "ChannelIsClosed",
+ "FiberIsCancelled",
+ "LuajitError",
+ "IllegalParams",
+ "CollationError",
+ "SwimError",
+ "CryptoError",
+};
+
+enum {
+ TEST_STANDARD_ERRORS_NUM =
+ sizeof(standard_errors) / sizeof(standard_errors[0]),
+};
+
+static inline char *
+mp_encode_str0(char *data, const char *str)
+{
+ return mp_encode_str(data, str, strlen(str));
+}
+
+/** Note, not the same as mp_encode_error(). */
+static char *
+mp_encode_mp_error(const struct mp_error *e, char *data)
+{
+ uint32_t fields_num = 6;
+ int field_count = (e->custom_type != NULL) +
+ (e->ad_object_type != NULL) +
+ (e->ad_object_name != NULL) +
+ (e->ad_access_type != NULL) +
+ (e->unknown_str_field != NULL);
+ fields_num += (field_count != 0);
+ fields_num += (e->unknown_uint_field != 0);
+
+ data = mp_encode_map(data, fields_num);
+ data = mp_encode_uint(data, MP_ERROR_TYPE);
+ data = mp_encode_str0(data, e->type);
+ data = mp_encode_uint(data, MP_ERROR_FILE);
+ data = mp_encode_str0(data, e->file);
+ data = mp_encode_uint(data, MP_ERROR_LINE);
+ data = mp_encode_uint(data, e->line);
+ data = mp_encode_uint(data, MP_ERROR_MESSAGE);
+ data = mp_encode_str0(data, e->message);
+ data = mp_encode_uint(data, MP_ERROR_ERRNO);
+ data = mp_encode_uint(data, e->saved_errno);
+ data = mp_encode_uint(data, MP_ERROR_CODE);
+ data = mp_encode_uint(data, e->code);
+ if (e->unknown_uint_field != 0) {
+ data = mp_encode_uint(data, UINT64_MAX);
+ data = mp_encode_uint(data, e->unknown_uint_field);
+ }
+ if (field_count != 0) {
+ data = mp_encode_uint(data, MP_ERROR_FIELDS);
+ data = mp_encode_map(data, field_count);
+ if (e->custom_type != NULL) {
+ data = mp_encode_str0(data, "custom_type");
+ data = mp_encode_str0(data, e->custom_type);
+ }
+ if (e->ad_object_type != NULL) {
+ data = mp_encode_str0(data, "object_type");
+ data = mp_encode_str0(data, e->ad_object_type);
+ }
+ if (e->ad_object_name != NULL) {
+ data = mp_encode_str0(data, "object_name");
+ data = mp_encode_str0(data, e->ad_object_name);
+ }
+ if (e->ad_access_type != NULL) {
+ data = mp_encode_str0(data, "access_type");
+ data = mp_encode_str0(data, e->ad_access_type);
+ }
+ if (e->unknown_str_field != NULL) {
+ data = mp_encode_str0(data, "unknown_field");
+ data = mp_encode_str0(data, e->unknown_str_field);
+ }
+ }
+ return data;
+}
+
+static char *
+mp_encode_error_header(char *data, int stack_size)
+{
+ data = mp_encode_map(data, 1);
+ data = mp_encode_uint(data, MP_ERROR_STACK);
+ data = mp_encode_array(data, stack_size);
+ return data;
+}
+
+static char *
+mp_encode_test_error_stack(char *data)
+{
+ data = mp_encode_error_header(data, TEST_STANDARD_ERRORS_NUM + 3);
+ /*
+ * CustomError
+ */
+ struct mp_error err;
+ memset(&err, 0, sizeof(err));
+ err.code = 123;
+ err.line = 1;
+ err.saved_errno = 2;
+ err.type = "CustomError";
+ err.file = "File1";
+ err.message = "Message1";
+ err.custom_type = "MyType";
+ data = mp_encode_mp_error(&err, data);
+ /*
+ * AccessDeniedError
+ */
+ memset(&err, 0, sizeof(err));
+ err.code = 42;
+ err.line = 3;
+ err.saved_errno = 4;
+ err.type = "AccessDeniedError";
+ err.file = "File2";
+ err.message = "Message2";
+ err.ad_object_type = "ObjectType";
+ err.ad_object_name = "ObjectName";
+ err.ad_access_type = "AccessType";
+ data = mp_encode_mp_error(&err, data);
+ /*
+ * ClientError
+ */
+ memset(&err, 0, sizeof(err));
+ err.code = 123;
+ err.line = 5;
+ err.saved_errno = 6;
+ err.type = "ClientError";
+ err.file = "File3";
+ err.message = "Message4";
+ data = mp_encode_mp_error(&err, data);
+
+ /*
+ * All errors with standard fields only.
+ */
+ for (uint8_t i = 0; i < TEST_STANDARD_ERRORS_NUM; ++i) {
+ memset(&err, 0, sizeof(err));
+ err.code = i;
+ err.line = i;
+ err.saved_errno = i;
+ err.type = standard_errors[i];
+ err.file = tt_sprintf("File%d", i);
+ err.message = tt_sprintf("Message%d", i);
+ data = mp_encode_mp_error(&err, data);
+ }
+
+ return data;
+}
+
+static bool
+error_is_eq_mp_error(struct error *err, struct mp_error *check)
+{
+ if (err->saved_errno != check->saved_errno)
+ return false;
+ if (strcmp(err->type->name, check->type) != 0)
+ return false;
+ if (strcmp(err->file, check->file) != 0)
+ return false;
+ if (err->line != check->line)
+ return false;
+ if (strcmp(err->errmsg, check->message) != 0)
+ return false;
+
+ if (strcmp(check->type, "ClientError") == 0) {
+ if (box_error_code(err) != check->code)
+ return false;
+ } else if (strcmp(check->type, "CustomError") == 0) {
+ CustomError *cust_err = type_cast(CustomError, err);
+ if (box_error_code(err) != check->code ||
+ strcmp(cust_err->custom_type(), check->custom_type) != 0)
+ return false;
+ } else if (strcmp(check->type, "AccessDeniedError") == 0) {
+ AccessDeniedError *ad_err = type_cast(AccessDeniedError, err);
+ if (box_error_code(err) != check->code ||
+ strcmp(ad_err->access_type(), check->ad_access_type) != 0 ||
+ strcmp(ad_err->object_name(), check->ad_object_name) != 0 ||
+ strcmp(ad_err->object_type(), check->ad_object_type) != 0)
+ return false;
+ }
+ return true;
+}
+
+void
+test_stack_error_decode()
+{
+ header();
+ plan(TEST_STANDARD_ERRORS_NUM + 4);
+
+ char buffer[2048];
+ memset(buffer, 0, sizeof(buffer));
+ char *end = mp_encode_test_error_stack(buffer);
+
+ uint32_t len = end - buffer;
+ const char *pos = buffer;
+ struct error *err1 = error_unpack(&pos, len);
+ error_ref(err1);
+ struct error *err2 = err1->cause;
+ struct error *err3 = err2->cause;
+
+ struct mp_error check;
+ memset(&check, 0, sizeof(check));
+ check.code = 123;
+ check.line = 1;
+ check.saved_errno = 2;
+ check.type = "CustomError";
+ check.file = "File1";
+ check.message = "Message1";
+ check.custom_type = "MyType";
+ ok(error_is_eq_mp_error(err1, &check), "check CustomError");
+
+ memset(&check, 0, sizeof(check));
+ check.code = 42;
+ check.line = 3;
+ check.saved_errno = 4;
+ check.type = "AccessDeniedError";
+ check.file = "File2";
+ check.message = "Message2";
+ check.ad_object_type = "ObjectType";
+ check.ad_object_name = "ObjectName";
+ check.ad_access_type = "AccessType";
+ ok(error_is_eq_mp_error(err2, &check), "check AccessDeniedError");
+
+ memset(&check, 0, sizeof(check));
+ check.code = 123;
+ check.line = 5;
+ check.saved_errno = 6;
+ check.type = "ClientError";
+ check.file = "File3";
+ check.message = "Message4";
+ ok(error_is_eq_mp_error(err3, &check), "check ClientError");
+
+ struct error *cur_err = err3;
+ int i = 0;
+ while(cur_err->cause) {
+ cur_err = cur_err->cause;
+ memset(&check, 0, sizeof(check));
+ check.code = i;
+ check.line = i;
+ check.saved_errno = i;
+ check.type = standard_errors[i];
+ check.file = tt_sprintf("File%d", i);
+ check.message = tt_sprintf("Message%d", i);
+ ok(error_is_eq_mp_error(cur_err, &check), "check %s",
+ standard_errors[i]);
+ ++i;
+ }
+ is(i, TEST_STANDARD_ERRORS_NUM, "stack size");
+ error_unref(err1);
+ check_plan();
+ footer();
+}
+
+void
+test_decode_unknown_type()
+{
+ header();
+ plan(1);
+ char buffer[2048];
+ memset(buffer, 0, sizeof(buffer));
+
+ char *data = mp_encode_error_header(buffer, 1);
+ struct mp_error err;
+ memset(&err, 0, sizeof(err));
+ err.code = 1;
+ err.line = 2;
+ err.saved_errno = 3;
+ err.type = "SomeNewError";
+ err.file = "File1";
+ err.message = "Message1";
+ data = mp_encode_mp_error(&err, data);
+
+ uint32_t len = data - buffer;
+ const char *pos = buffer;
+ struct error *unpacked = error_unpack(&pos, len);
+ error_ref(unpacked);
+ err.code = 0;
+ err.type = "ClientError";
+ ok(error_is_eq_mp_error(unpacked, &err), "check SomeNewError");
+ error_unref(unpacked);
+
+ check_plan();
+ footer();
+}
+
+void
+test_fail_not_enough_fields()
+{
+ header();
+ plan(2);
+ char buffer[2048];
+ memset(buffer, 0, sizeof(buffer));
+
+ char *data = mp_encode_error_header(buffer, 1);
+ struct mp_error err;
+ memset(&err, 0, sizeof(err));
+ err.code = 42;
+ err.line = 3;
+ err.saved_errno = 4;
+ err.type = "AccessDeniedError";
+ err.file = "File1";
+ err.message = "Message1";
+ err.ad_object_type = "ObjectType";
+ err.ad_access_type = "AccessType";
+ data = mp_encode_mp_error(&err, data);
+
+ uint32_t len = data - buffer;
+ const char *pos = buffer;
+ struct error *unpacked = error_unpack(&pos, len);
+
+ is(unpacked, NULL, "check not enough additional fields");
+ ok(!diag_is_empty(diag_get()), "error about parsing problem is set");
+ check_plan();
+ footer();
+}
+
+void
+test_unknown_fields()
+{
+ header();
+ plan(1);
+ char buffer[2048];
+ memset(buffer, 0, sizeof(buffer));
+
+ char *data = mp_encode_error_header(buffer, 1);
+ struct mp_error err;
+ memset(&err, 0, sizeof(err));
+ err.code = 0;
+ err.line = 1;
+ err.saved_errno = 0;
+ err.type = "SystemError";
+ err.file = "File";
+ err.message = "Message";
+ err.unknown_uint_field = 55;
+ data = mp_encode_mp_error(&err, data);
+
+ uint32_t len = data - buffer;
+ const char *pos = buffer;
+ struct error *unpacked = error_unpack(&pos, len);
+ error_ref(unpacked);
+ data = mp_encode_mp_error(&err, data);
+
+ ok(error_is_eq_mp_error(unpacked, &err), "check unknown fields");
+ error_unref(unpacked);
+ check_plan();
+}
+
+void
+test_unknown_additional_fields()
+{
+ header();
+ plan(1);
+ char buffer[2048];
+ memset(buffer, 0, sizeof(buffer));
+
+ char *data = mp_encode_error_header(buffer, 1);
+ struct mp_error err;
+ memset(&err, 0, sizeof(err));
+ err.code = 42;
+ err.line = 3;
+ err.saved_errno = 4;
+ err.type = "AccessDeniedError";
+ err.file = "File";
+ err.message = "Message";
+ err.ad_object_type = "ObjectType";
+ err.ad_object_name = "ObjectName";
+ err.ad_access_type = "AccessType";
+ err.unknown_str_field = "unknown_field";
+ data = mp_encode_mp_error(&err, data);
+
+ uint32_t len = data - buffer;
+ const char *pos = buffer;
+ struct error *unpacked = error_unpack(&pos, len);
+ error_ref(unpacked);
+ ok(error_is_eq_mp_error(unpacked, &err),
+ "check unknown additional field");
+ error_unref(unpacked);
+
+ check_plan();
+ footer();
+}
+
+int
+main(void)
+{
+ header();
+ plan(5);
+ memory_init();
+ fiber_init(fiber_c_invoke);
+
+ test_stack_error_decode();
+ test_decode_unknown_type();
+ test_fail_not_enough_fields();
+ test_unknown_fields();
+ test_unknown_additional_fields();
+
+ fiber_free();
+ memory_free();
+ footer();
+ return check_plan();
+}
diff --git a/test/unit/mp_error.result b/test/unit/mp_error.result
new file mode 100644
index 0000000..6ef37b4
--- /dev/null
+++ b/test/unit/mp_error.result
@@ -0,0 +1,44 @@
+ *** main ***
+1..5
+ *** test_stack_error_decode ***
+ 1..17
+ ok 1 - check CustomError
+ ok 2 - check AccessDeniedError
+ ok 3 - check ClientError
+ ok 4 - check XlogError
+ ok 5 - check XlogGapError
+ ok 6 - check SystemError
+ ok 7 - check SocketError
+ ok 8 - check OutOfMemory
+ ok 9 - check TimedOut
+ ok 10 - check ChannelIsClosed
+ ok 11 - check FiberIsCancelled
+ ok 12 - check LuajitError
+ ok 13 - check IllegalParams
+ ok 14 - check CollationError
+ ok 15 - check SwimError
+ ok 16 - check CryptoError
+ ok 17 - stack size
+ok 1 - subtests
+ *** test_stack_error_decode: done ***
+ *** test_decode_unknown_type ***
+ 1..1
+ ok 1 - check SomeNewError
+ok 2 - subtests
+ *** test_decode_unknown_type: done ***
+ *** test_fail_not_enough_fields ***
+ 1..2
+ ok 1 - check not enough additional fields
+ ok 2 - error about parsing problem is set
+ok 3 - subtests
+ *** test_fail_not_enough_fields: done ***
+ *** test_unknown_fields ***
+ 1..1
+ ok 1 - check unknown fields
+ok 4 - subtests
+ *** test_unknown_additional_fields ***
+ 1..1
+ ok 1 - check unknown additional field
+ok 5 - subtests
+ *** test_unknown_additional_fields: done ***
+ *** main: done ***
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH V6 07/10] error: export error_unref() function
2020-04-19 22:25 [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Leonid Vasiliev
` (5 preceding siblings ...)
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 06/10] error: add error MsgPack encoding Leonid Vasiliev
@ 2020-04-19 22:25 ` Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 08/10] error: make iproto errors reuse mp_error module Leonid Vasiliev
` (4 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Leonid Vasiliev @ 2020-04-19 22:25 UTC (permalink / raw)
To: v.shpilevoy, alexander.turenko; +Cc: tarantool-patches
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
C struct error objects can be created directly only in C.
C-side increments their reference counter when pushes to the Lua
stack.
It is not going to be so convenient soon. error_unpack() function
will be used in netbox to decode error object via Lua FFI.
Such error object will have 0 refs and no Lua GC callback
established. Because it won't be pushed on Lua stack natually,
from Lua C. To make such errors alive their reference counter
will be incremented and error_unref() will be set as GC callback.
Follow up for #4398
---
| 1 +
src/lib/core/diag.c | 20 ++++++++++++++++++++
src/lib/core/diag.h | 21 ++-------------------
3 files changed, 23 insertions(+), 19 deletions(-)
--git a/extra/exports b/extra/exports
index 9467398..3317ddb 100644
--- a/extra/exports
+++ b/extra/exports
@@ -242,6 +242,7 @@ box_error_last
box_error_clear
box_error_set
error_set_prev
+error_unref
box_latch_new
box_latch_delete
box_latch_lock
diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
index e143db1..787f0f8 100644
--- a/src/lib/core/diag.c
+++ b/src/lib/core/diag.c
@@ -31,6 +31,26 @@
#include "diag.h"
#include "fiber.h"
+void
+error_unref(struct error *e)
+{
+ assert(e->refs > 0);
+ struct error *to_delete = e;
+ while (--to_delete->refs == 0) {
+ /* Unlink error from lists completely.*/
+ struct error *cause = to_delete->cause;
+ assert(to_delete->effect == NULL);
+ if (to_delete->cause != NULL) {
+ to_delete->cause->effect = NULL;
+ to_delete->cause = NULL;
+ }
+ to_delete->destroy(to_delete);
+ if (cause == NULL)
+ return;
+ to_delete = cause;
+ }
+}
+
int
error_set_prev(struct error *e, struct error *prev)
{
diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
index 7a5454d..beb6a75 100644
--- a/src/lib/core/diag.h
+++ b/src/lib/core/diag.h
@@ -118,25 +118,8 @@ error_ref(struct error *e)
e->refs++;
}
-static inline void
-error_unref(struct error *e)
-{
- assert(e->refs > 0);
- struct error *to_delete = e;
- while (--to_delete->refs == 0) {
- /* Unlink error from lists completely.*/
- struct error *cause = to_delete->cause;
- assert(to_delete->effect == NULL);
- if (to_delete->cause != NULL) {
- to_delete->cause->effect = NULL;
- to_delete->cause = NULL;
- }
- to_delete->destroy(to_delete);
- if (cause == NULL)
- return;
- to_delete = cause;
- }
-}
+void
+error_unref(struct error *e);
/**
* Unlink error from its effect. For instance:
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH V6 08/10] error: make iproto errors reuse mp_error module
2020-04-19 22:25 [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Leonid Vasiliev
` (6 preceding siblings ...)
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 07/10] error: export error_unref() function Leonid Vasiliev
@ 2020-04-19 22:25 ` Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 09/10] iproto: rename IPROTO_ERROR and IPROTO_ERROR_STACK Leonid Vasiliev
` (3 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Leonid Vasiliev @ 2020-04-19 22:25 UTC (permalink / raw)
To: v.shpilevoy, alexander.turenko; +Cc: tarantool-patches
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
After error objects marshaling was implemented in #4398, there
were essentially 2 versions of the marshaling - when an error is
sent inside response body, and when it is thrown and is encoded
in iproto fields IPROTO_ERROR and IPROTO_ERROR_STACK. That is not
really useful to have 2 implementation of the same feature. This
commit drops the old iproto error encoding (its IPROTO_ERROR_STACK
part), and makes it reuse the common error encoder.
Note, the encoder skips MP_EXT header. This is because
* The header is not needed - error is encoded as a value of
IPROTO_ERROR_STACK key, so it is known this is an error. MP_EXT
is needed only when type is unknown on decoding side in advance;
* Old clients may not expect MP_EXT in iproto fields. That is the
case of netbox connector, at least.
Follow up #4398
@TarantoolBot document
Title: Stacked diagnostics binary protocol
Stacked diagnostics is described in details in
https://github.com/tarantool/doc/issues/1224. This commit
changes nothing except binary protocol. The old protocol should
not be documented anywhere.
`IPROTO_ERROR_STACK` is still 0x52, but format of its value is
different now. It looks exactly like `MP_ERROR` object, without
`MP_EXT` header.
```
IPROTO_ERROR_STACK: <MP_MAP> {
MP_ERROR_STACK: <MP_ARRAY> [
<MP_MAP> {
... <all the other fields of MP_ERROR> ...
},
...
]
}
```
It is easy to see, that key `IPROTO_ERROR_STACK` is called
'stack', and `MP_ERROR_STACK` is also 'stack'. So it may be good
to rename the former key in the documentation. For example, the
old `IPROTO_ERROR` can be renamed to `IPROTO_ERROR_24` and
`IPROTO_ERROR_STACK` can be renamed to just `IPROTO_ERROR`.
---
| 1 +
src/box/iproto_constants.h | 5 -
src/box/lua/net_box.lua | 86 +++++++++++-----
src/box/mp_error.cc | 31 +++++-
src/box/mp_error.h | 18 +++-
src/box/xrow.c | 63 +-----------
test/box-tap/extended_error.test.lua | 48 ++++++---
test/box/iproto.result | 37 -------
test/box/iproto.test.lua | 11 ---
test/unit/xrow.cc | 183 +----------------------------------
test/unit/xrow.result | 27 +-----
11 files changed, 151 insertions(+), 359 deletions(-)
--git a/extra/exports b/extra/exports
index 3317ddb..d6b02e7 100644
--- a/extra/exports
+++ b/extra/exports
@@ -242,6 +242,7 @@ box_error_last
box_error_clear
box_error_set
error_set_prev
+error_unpack_unsafe
error_unref
box_latch_new
box_latch_delete
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index 7ed8296..b57d565 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -151,11 +151,6 @@ enum iproto_ballot_key {
IPROTO_BALLOT_IS_LOADING = 0x04,
};
-enum iproto_error_key {
- IPROTO_ERROR_CODE = 0x01,
- IPROTO_ERROR_MESSAGE = 0x02,
-};
-
#define bit(c) (1ULL<<IPROTO_##c)
#define IPROTO_HEAD_BMAP (bit(REQUEST_TYPE) | bit(SYNC) | bit(REPLICA_ID) |\
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 07fa54c..1e55eaa 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -16,6 +16,7 @@ local fiber_clock = fiber.clock
local fiber_self = fiber.self
local decode = msgpack.decode_unchecked
local decode_map_header = msgpack.decode_map_header
+local buffer_reg = buffer.reg1
local table_new = require('table.new')
local check_iterator_type = box.internal.check_iterator_type
@@ -45,8 +46,6 @@ local IPROTO_FIELD_NAME_KEY = 0
local IPROTO_DATA_KEY = 0x30
local IPROTO_ERROR_KEY = 0x31
local IPROTO_ERROR_STACK = 0x52
-local IPROTO_ERROR_CODE = 0x01
-local IPROTO_ERROR_MESSAGE = 0x02
local IPROTO_GREETING_SIZE = 128
local IPROTO_CHUNK_KEY = 128
local IPROTO_OK_KEY = 0
@@ -57,6 +56,14 @@ local E_NO_CONNECTION = box.error.NO_CONNECTION
local E_TIMEOUT = box.error.TIMEOUT
local E_PROC_LUA = box.error.PROC_LUA
+ffi.cdef[[
+struct error *
+error_unpack_unsafe(const char **data);
+
+void
+error_unref(struct error *e);
+]]
+
-- utility tables
local is_final_state = {closed = 1, error = 1}
@@ -143,6 +150,30 @@ local method_decoder = {
push = decode_push,
}
+local function decode_error_stack(raw_data)
+ local ptr = buffer_reg.acucp
+ ptr[0] = raw_data
+ local err = ffi.C.error_unpack_unsafe(ptr)
+ if err ~= nil then
+ err._refs = err._refs + 1
+ err = ffi.gc(err, ffi.C.error_unref)
+ -- From FFI it is returned as 'struct error *', which is
+ -- not considered equal to 'const struct error &', and is
+ -- is not accepted by functions like box.error(). Need to
+ -- cast explicitly.
+ err = ffi.cast('const struct error &', err)
+ else
+ -- Error unpacker installs fail reason into diag.
+ box.error()
+ end
+ return err, ptr[0]
+end
+
+local response_decoder = {
+ [IPROTO_ERROR_KEY] = decode,
+ [IPROTO_ERROR_STACK] = decode_error_stack,
+}
+
local function next_id(id) return band(id + 1, 0x7FFFFFFF) end
--
@@ -280,22 +311,14 @@ local function create_transport(host, port, user, password, callback,
--
function request_index:result()
if self.errno then
- if type(self.response) == 'table' then
- -- Decode and fill in error stack.
- local prev = nil
- for i = #self.response, 1, -1 do
- local error = self.response[i]
- local code = error[IPROTO_ERROR_CODE]
- local msg = error[IPROTO_ERROR_MESSAGE]
- local new_err = box.error.new({code = code, reason = msg})
- new_err:set_prev(prev)
- prev = new_err
- end
- return nil, prev
- else
- return nil, box.error.new({code = self.errno,
- reason = self.response})
+ if type(self.response) ~= 'cdata' then
+ -- Error could be set by the connection state
+ -- machine, and it is usually a string explaining
+ -- a reason.
+ self.response = box.error.new({code = self.errno,
+ reason = self.response})
end
+ return nil, self.response
elseif not self.id then
return self.response
elseif not worker_fiber then
@@ -567,22 +590,40 @@ local function create_transport(host, port, user, password, callback,
return
end
local status = hdr[IPROTO_STATUS_KEY]
- local body, body_end_check
+ local body
+ local body_len = body_end - body_rpos
if status > IPROTO_CHUNK_KEY then
-- Handle errors
requests[id] = nil
request.id = nil
- body, body_end_check = decode(body_rpos)
- assert(body_end == body_end_check, "invalid xrow length")
+ local map_len, key, skip
+ map_len, body_rpos = decode_map_header(body_rpos, body_len)
+ -- Reserve for 2 keys and 2 array indexes. Because no
+ -- any guarantees how Lua will decide to save the
+ -- sparse table.
+ body = table_new(2, 2)
+ for _ = 1, map_len do
+ key, body_rpos = decode(body_rpos)
+ local rdec = response_decoder[key]
+ if rdec then
+ body[key], body_rpos = rdec(body_rpos)
+ else
+ skip, body_rpos = decode(body_rpos)
+ end
+ end
+ assert(body_end == body_rpos, "invalid xrow length")
request.errno = band(status, IPROTO_ERRNO_MASK)
-- IPROTO_ERROR_STACK comprises error encoded with
-- IPROTO_ERROR_KEY, so we may ignore content of that key.
if body[IPROTO_ERROR_STACK] ~= nil then
request.response = body[IPROTO_ERROR_STACK]
- assert(type(request.response) == 'table')
+ assert(type(request.response) == 'cdata')
else
- request.response = body[IPROTO_ERROR_KEY]
+ request.response = box.error.new({
+ code = request.errno,
+ reason = body[IPROTO_ERROR_KEY]
+ })
end
request.cond:broadcast()
return
@@ -591,7 +632,6 @@ local function create_transport(host, port, user, password, callback,
local buffer = request.buffer
if buffer ~= nil then
-- Copy xrow.body to user-provided buffer
- local body_len = body_end - body_rpos
if request.skip_header then
-- Skip {[IPROTO_DATA_KEY] = ...} wrapper.
local map_len, key
diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
index e7d2808..0491a7a 100644
--- a/src/box/mp_error.cc
+++ b/src/box/mp_error.cc
@@ -453,6 +453,31 @@ error:
}
void
+error_to_mpstream_noext(const struct error *error, struct mpstream *stream)
+{
+ uint32_t err_cnt = 0;
+ uint32_t data_size = mp_sizeof_map(1);
+ data_size += mp_sizeof_uint(MP_ERROR_STACK);
+ for (const struct error *it = error; it != NULL; it = it->cause) {
+ err_cnt++;
+ data_size += mp_sizeof_error(it);
+ }
+
+ data_size += mp_sizeof_array(err_cnt);
+ char *ptr = mpstream_reserve(stream, data_size);
+ char *data = ptr;
+ data = mp_encode_map(data, 1);
+ data = mp_encode_uint(data, MP_ERROR_STACK);
+ data = mp_encode_array(data, err_cnt);
+ for (const struct error *it = error; it != NULL; it = it->cause) {
+ mp_encode_error_one(it, &data);
+ }
+
+ assert(data == ptr + data_size);
+ mpstream_advance(stream, data_size);
+}
+
+void
error_to_mpstream(const struct error *error, struct mpstream *stream)
{
uint32_t err_cnt = 0;
@@ -480,9 +505,8 @@ error_to_mpstream(const struct error *error, struct mpstream *stream)
}
struct error *
-error_unpack(const char **data, uint32_t len)
+error_unpack_unsafe(const char **data)
{
- const char *end = *data + len;
struct error *err = NULL;
if (mp_typeof(**data) != MP_MAP) {
@@ -526,8 +550,5 @@ error_unpack(const char **data, uint32_t len)
mp_next(data);
}
}
-
- (void)end;
- assert(*data == end);
return err;
}
diff --git a/src/box/mp_error.h b/src/box/mp_error.h
index 5c746d9..f040e0f 100644
--- a/src/box/mp_error.h
+++ b/src/box/mp_error.h
@@ -35,6 +35,7 @@ extern "C" {
#endif /* defined(__cplusplus) */
#include <stdint.h>
+#include <assert.h>
struct mpstream;
@@ -46,14 +47,27 @@ struct mpstream;
void
error_to_mpstream(const struct error *error, struct mpstream *stream);
+void
+error_to_mpstream_noext(const struct error *error, struct mpstream *stream);
+
+struct error *
+error_unpack_unsafe(const char **data);
+
/**
* @brief Unpack MP_ERROR to error.
* @param data pointer to MP_ERROR.
* @param len data size.
* @return struct error * or NULL if failed.
*/
-struct error *
-error_unpack(const char **data, uint32_t len);
+static inline struct error *
+error_unpack(const char **data, uint32_t len)
+{
+ const char *end = *data + len;
+ struct error *e = error_unpack_unsafe(data);
+ (void)end;
+ assert(*data == end);
+ return e;
+}
#if defined(__cplusplus)
} /* extern "C" */
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 97a604a..06c6afb 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -39,6 +39,7 @@
#include "version.h"
#include "tt_static.h"
#include "error.h"
+#include "mp_error.h"
#include "vclock.h"
#include "scramble.h"
#include "iproto_constants.h"
@@ -497,19 +498,8 @@ mpstream_iproto_encode_error(struct mpstream *stream, const struct error *error)
mpstream_encode_map(stream, 2);
mpstream_encode_uint(stream, IPROTO_ERROR);
mpstream_encode_str(stream, error->errmsg);
-
- uint32_t err_cnt = 0;
- for (const struct error *it = error; it != NULL; it = it->cause)
- err_cnt++;
mpstream_encode_uint(stream, IPROTO_ERROR_STACK);
- mpstream_encode_array(stream, err_cnt);
- for (const struct error *it = error; it != NULL; it = it->cause) {
- mpstream_encode_map(stream, 2);
- mpstream_encode_uint(stream, IPROTO_ERROR_CODE);
- mpstream_encode_uint(stream, box_error_code(it));
- mpstream_encode_uint(stream, IPROTO_ERROR_MESSAGE);
- mpstream_encode_str(stream, it->errmsg);
- }
+ error_to_mpstream_noext(error, stream);
}
int
@@ -1093,51 +1083,6 @@ xrow_encode_auth(struct xrow_header *packet, const char *salt, size_t salt_len,
return 0;
}
-static int
-iproto_decode_error_stack(const char **pos)
-{
- const char *reason = NULL;
- static_assert(TT_STATIC_BUF_LEN >= DIAG_ERRMSG_MAX, "static buffer is "\
- "expected to be larger than error message max length");
- /*
- * Erase previously set diag errors. It is required since
- * box_error_add() does not replace previous errors.
- */
- box_error_clear();
- if (mp_typeof(**pos) != MP_ARRAY)
- return -1;
- uint32_t stack_sz = mp_decode_array(pos);
- for (uint32_t i = 0; i < stack_sz; i++) {
- uint64_t code = 0;
- if (mp_typeof(**pos) != MP_MAP)
- return -1;
- uint32_t map_sz = mp_decode_map(pos);
- for (uint32_t key_idx = 0; key_idx < map_sz; key_idx++) {
- if (mp_typeof(**pos) != MP_UINT)
- return -1;
- uint64_t key = mp_decode_uint(pos);
- if (key == IPROTO_ERROR_CODE) {
- if (mp_typeof(**pos) != MP_UINT)
- return -1;
- code = mp_decode_uint(pos);
- if (code > UINT32_MAX)
- return -1;
- } else if (key == IPROTO_ERROR_MESSAGE) {
- if (mp_typeof(**pos) != MP_STR)
- return -1;
- uint32_t len;
- const char *str = mp_decode_str(pos, &len);
- reason = tt_cstr(str, len);
- } else {
- mp_next(pos);
- continue;
- }
- }
- box_error_add(__FILE__, __LINE__, code, NULL, reason);
- }
- return 0;
-}
-
void
xrow_decode_error(struct xrow_header *row)
{
@@ -1175,8 +1120,10 @@ xrow_decode_error(struct xrow_header *row)
snprintf(error, sizeof(error), "%.*s", len, str);
box_error_set(__FILE__, __LINE__, code, error);
} else if (key == IPROTO_ERROR_STACK) {
- if (iproto_decode_error_stack(&pos) != 0)
+ struct error *e = error_unpack_unsafe(&pos);
+ if (e == NULL)
goto error;
+ diag_set_error(diag_get(), e);
} else {
mp_next(&pos);
continue;
diff --git a/test/box-tap/extended_error.test.lua b/test/box-tap/extended_error.test.lua
index 1681419..884387b 100755
--- a/test/box-tap/extended_error.test.lua
+++ b/test/box-tap/extended_error.test.lua
@@ -30,12 +30,16 @@ box.schema.func.drop('forbidden_function')
box.session.su(user)
local test = tap.test('Error marshaling')
-test:plan(6)
+test:plan(12)
function error_new(...)
return box.error.new(...)
end
+function error_throw(...)
+ box.error(error_new(...))
+end
+
function error_new_stacked(args1, args2)
local e1 = box.error.new(args1)
local e2 = box.error.new(args2)
@@ -43,10 +47,18 @@ function error_new_stacked(args1, args2)
return e1
end
+function error_throw_stacked(...)
+ box.error(error_new_stacked(...))
+end
+
function error_access_denied()
return access_denied_error
end
+function error_throw_access_denied()
+ box.error(access_denied_error)
+end
+
local function check_error(err, check_list)
assert(type(check_list) == 'table')
if type(err.trace) ~= 'table' or err.trace[1] == nil or
@@ -64,7 +76,8 @@ end
box.schema.user.grant('guest', 'super')
local c = netbox.connect(box.cfg.listen)
c:eval('box.session.settings.error_marshaling_enabled = true')
-local err = c:call('error_new', {{code = 1000, reason = 'Reason'}})
+local args = {{code = 1000, reason = 'Reason'}}
+local err = c:call('error_new', args)
local checks = {
code = 1000,
message = 'Reason',
@@ -72,10 +85,11 @@ local checks = {
type = 'ClientError',
}
test:ok(check_error(err, checks), "ClientError marshaling")
+tmp, err = pcall(c.call, c, 'error_throw', args)
+test:ok(check_error(err, checks), "ClientError marshaling in iproto fields")
-err = c:call('error_new', {{
- code = 1001, reason = 'Reason2', type = 'MyError'
-}})
+args = {{code = 1001, reason = 'Reason2', type = 'MyError'}}
+err = c:call('error_new', args)
checks = {
code = 1001,
message = 'Reason2',
@@ -83,6 +97,8 @@ checks = {
type = 'MyError',
}
test:ok(check_error(err, checks), "CustomError marshaling")
+tmp, err = pcall(c.call, c, 'error_throw', args)
+test:ok(check_error(err, checks), "CustomError marshaling in iproto fields")
err = c:call('error_access_denied')
checks = {
@@ -95,28 +111,38 @@ checks = {
access_type = 'Execute',
}
test:ok(check_error(err, checks), "AccessDeniedError marshaling")
+tmp, err = pcall(c.call, c, 'error_throw_access_denied')
+test:ok(check_error(err, checks), "AccessDeniedError marshaling in iproto fields")
-err = c:call('error_new_stacked', {
+args = {
{code = 1003, reason = 'Reason3', type = 'MyError2'},
{code = 1004, reason = 'Reason4'}
-})
+}
+err = c:call('error_new_stacked', args)
local err1 = err
local err2 = err.prev
test:isnt(err2, nil, 'Stack is received')
-checks = {
+local checks1 = {
code = 1003,
message = 'Reason3',
base_type = 'CustomError',
type = 'MyError2'
}
-test:ok(check_error(err1, checks), "First error in the stack")
-checks = {
+test:ok(check_error(err1, checks1), "First error in the stack")
+local checks2 = {
code = 1004,
message = 'Reason4',
base_type = 'ClientError',
type = 'ClientError'
}
-test:ok(check_error(err2, checks), "Second error in the stack")
+test:ok(check_error(err2, checks2), "Second error in the stack")
+
+tmp, err = pcall(c.call, c, 'error_throw_stacked', args)
+err1 = err
+err2 = err.prev
+test:isnt(err2, nil, 'Stack is received via iproto fields')
+test:ok(check_error(err1, checks1), "First error in the stack in iproto fields")
+test:ok(check_error(err2, checks2), "Second error in the stack in iproto fields")
c:close()
box.schema.user.revoke('guest', 'super')
diff --git a/test/box/iproto.result b/test/box/iproto.result
index b6dc7ed..e4fe684 100644
--- a/test/box/iproto.result
+++ b/test/box/iproto.result
@@ -100,43 +100,6 @@ assert(response.body[IPROTO_ERROR] ~= nil)
---
- true
...
-err = response.body[IPROTO_ERROR_STACK][1]
----
-...
-assert(err[IPROTO_ERROR_MESSAGE] == response.body[IPROTO_ERROR])
----
-- true
-...
-assert(err[IPROTO_ERROR_MESSAGE] == 'e3')
----
-- true
-...
-assert(err[IPROTO_ERROR_CODE] == 111)
----
-- true
-...
-err = response.body[IPROTO_ERROR_STACK][2]
----
-...
-assert(err[IPROTO_ERROR_MESSAGE] == 'e2')
----
-- true
-...
-assert(err[IPROTO_ERROR_CODE] == 111)
----
-- true
-...
-err = response.body[IPROTO_ERROR_STACK][3]
----
-...
-assert(err[IPROTO_ERROR_MESSAGE] == 'e1')
----
-- true
-...
-assert(err[IPROTO_ERROR_CODE] == 111)
----
-- true
-...
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
---
...
diff --git a/test/box/iproto.test.lua b/test/box/iproto.test.lua
index 6402a22..ec715e9 100644
--- a/test/box/iproto.test.lua
+++ b/test/box/iproto.test.lua
@@ -52,15 +52,4 @@ sock:close()
assert(response.body[IPROTO_ERROR_STACK] ~= nil)
assert(response.body[IPROTO_ERROR] ~= nil)
-err = response.body[IPROTO_ERROR_STACK][1]
-assert(err[IPROTO_ERROR_MESSAGE] == response.body[IPROTO_ERROR])
-assert(err[IPROTO_ERROR_MESSAGE] == 'e3')
-assert(err[IPROTO_ERROR_CODE] == 111)
-err = response.body[IPROTO_ERROR_STACK][2]
-assert(err[IPROTO_ERROR_MESSAGE] == 'e2')
-assert(err[IPROTO_ERROR_CODE] == 111)
-err = response.body[IPROTO_ERROR_STACK][3]
-assert(err[IPROTO_ERROR_MESSAGE] == 'e1')
-assert(err[IPROTO_ERROR_CODE] == 111)
-
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
diff --git a/test/unit/xrow.cc b/test/unit/xrow.cc
index dd1a85d..9fd1547 100644
--- a/test/unit/xrow.cc
+++ b/test/unit/xrow.cc
@@ -242,186 +242,6 @@ test_xrow_header_encode_decode()
check_plan();
}
-static char *
-error_stack_entry_encode(char *pos, const char *err_str)
-{
- pos = mp_encode_map(pos, 2);
- pos = mp_encode_uint(pos, IPROTO_ERROR_CODE);
- pos = mp_encode_uint(pos, 159);
- pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE);
- pos = mp_encode_str(pos, err_str, strlen(err_str));
- return pos;
-}
-
-void
-test_xrow_error_stack_decode()
-{
- plan(21);
- char buffer[2048];
- /*
- * To start with, let's test the simplest and obsolete
- * way of setting errors.
- */
- char *pos = mp_encode_map(buffer, 1);
- pos = mp_encode_uint(pos, IPROTO_ERROR);
- pos = mp_encode_str(pos, "e1", strlen("e1"));
-
- struct xrow_header header;
- header.bodycnt = 666;
- header.type = 159 | IPROTO_TYPE_ERROR;
- header.body[0].iov_base = buffer;
- header.body[0].iov_len = pos - buffer;
-
- xrow_decode_error(&header);
- struct error *last = diag_last_error(diag_get());
- isnt(last, NULL, "xrow_decode succeed: diag has been set");
- is(strcmp(last->errmsg, "e1"), 0,
- "xrow_decode succeed: error is parsed");
-
- pos = mp_encode_map(buffer, 1);
- pos = mp_encode_uint(pos, IPROTO_ERROR_STACK);
- pos = mp_encode_array(pos, 2);
- pos = error_stack_entry_encode(pos, "e1");
- pos = error_stack_entry_encode(pos, "e2");
- header.body[0].iov_base = buffer;
- header.body[0].iov_len = pos - buffer;
-
- diag_clear(diag_get());
- xrow_decode_error(&header);
- last = diag_last_error(diag_get());
- isnt(last, NULL, "xrow_decode succeed: diag has been set");
- is(strcmp(last->errmsg, "e2"), 0, "xrow_decode error stack suceed: "
- "e2 at the top of stack");
- last = last->cause;
- isnt(last, NULL, "xrow_decode succeed: 'cause' is present in stack")
- is(strcmp(last->errmsg, "e1"), 0, "xrow_decode succeed: "
- "stack has been parsed");
- last = last->cause;
- is(last, NULL, "xrow_decode succeed: only two errors in the stack")
- /*
- * Let's try decode broken stack. Variations:
- * 1. Stack is not encoded as an array;
- * 2. Stack doesn't contain maps;
- * 3. Stack's map keys are not uints;
- * 4. Stack's map values have wrong types;
- * 5. Stack's map key is broken (doesn't fit into u8);
- * 6. Stack's map contains overflowed (> 2^32) error code;
- * In all these cases diag_last should contain empty err.
- */
- /* Stack is encoded as map. */
- pos = mp_encode_map(buffer, 1);
- pos = mp_encode_uint(pos, IPROTO_ERROR_STACK);
- pos = mp_encode_map(pos, 1);
- pos = error_stack_entry_encode(pos, "e1");
- header.body[0].iov_base = buffer;
- header.body[0].iov_len = pos - buffer;
-
- diag_clear(diag_get());
- xrow_decode_error(&header);
- last = diag_last_error(diag_get());
- isnt(last, NULL, "xrow_decode succeed: diag has been set");
- is(strcmp(last->errmsg, ""), 0, "xrow_decode corrupted stack: "
- "stack contains map instead of array");
-
- /* Stack doesn't containt map(s) - array instead. */
- pos = mp_encode_map(buffer, 1);
- pos = mp_encode_uint(pos, IPROTO_ERROR_STACK);
- pos = mp_encode_array(pos, 2);
- pos = mp_encode_uint(pos, IPROTO_ERROR_CODE);
- pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE);
- header.body[0].iov_base = buffer;
- header.body[0].iov_len = pos - buffer;
-
- diag_clear(diag_get());
- xrow_decode_error(&header);
- last = diag_last_error(diag_get());
- isnt(last, NULL, "xrow_decode succeed: diag has been set");
- is(strcmp(last->errmsg, ""), 0, "xrow_decode corrupted stack: "
- "stack contains array values instead of maps");
-
- /* Stack's map keys are not uints. */
- pos = mp_encode_map(buffer, 1);
- pos = mp_encode_uint(pos, IPROTO_ERROR_STACK);
- pos = mp_encode_array(pos, 1);
- pos = mp_encode_map(pos, 2);
- pos = mp_encode_str(pos, "string instead of uint",
- strlen("string instead of uint"));
- pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE);
- pos = mp_encode_uint(pos, IPROTO_ERROR_CODE);
- pos = mp_encode_uint(pos, 159);
- header.body[0].iov_base = buffer;
- header.body[0].iov_len = pos - buffer;
-
- diag_clear(diag_get());
- xrow_decode_error(&header);
- last = diag_last_error(diag_get());
- isnt(last, NULL, "xrow_decode succeed: diag has been set");
- is(strcmp(last->errmsg, ""), 0, "xrow_decode corrupted stack: "
- "stack's map keys are not uints");
-
- /* Stack's map values have wrong types. */
- pos = mp_encode_map(buffer, 1);
- pos = mp_encode_uint(pos, IPROTO_ERROR_STACK);
- pos = mp_encode_array(pos, 1);
- pos = mp_encode_map(pos, 2);
- pos = mp_encode_uint(pos, IPROTO_ERROR_CODE);
- pos = mp_encode_uint(pos, 159);
- pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE);
- pos = mp_encode_uint(pos, 666);
- header.body[0].iov_base = buffer;
- header.body[0].iov_len = pos - buffer;
-
- diag_clear(diag_get());
- xrow_decode_error(&header);
- last = diag_last_error(diag_get());
- isnt(last, NULL, "xrow_decode succeed: diag has been set");
- is(strcmp(last->errmsg, ""), 0, "xrow_decode corrupted stack: "
- "stack's map wrong value type");
-
- /* Bad key in the packet. */
- pos = mp_encode_map(buffer, 1);
- pos = mp_encode_uint(pos, IPROTO_ERROR_STACK);
- pos = mp_encode_array(pos, 1);
- pos = mp_encode_map(pos, 2);
- pos = mp_encode_uint(pos, 0xff00000000 | IPROTO_ERROR_CODE);
- pos = mp_encode_uint(pos, 159);
- pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE);
- pos = mp_encode_str(pos, "test msg", strlen("test msg"));
- header.body[0].iov_base = buffer;
- header.body[0].iov_len = pos - buffer;
-
- diag_clear(diag_get());
- xrow_decode_error(&header);
- last = diag_last_error(diag_get());
- isnt(last, NULL, "xrow_decode succeed: diag has been set");
- is(box_error_code(last), 0, "xrow_decode last error code is default 0");
- is(strcmp(last->errmsg, "test msg"), 0, "xrow_decode corrupted stack: "
- "stack's map wrong key");
-
- /* Overflow error code. */
- pos = mp_encode_map(buffer, 1);
- pos = mp_encode_uint(pos, IPROTO_ERROR_STACK);
- pos = mp_encode_array(pos, 1);
- pos = mp_encode_map(pos, 2);
- pos = mp_encode_uint(pos, IPROTO_ERROR_CODE);
- pos = mp_encode_uint(pos, (uint64_t)1 << 40);
- pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE);
- pos = mp_encode_str(pos, "test msg", strlen("test msg"));
- header.body[0].iov_base = buffer;
- header.body[0].iov_len = pos - buffer;
-
- diag_clear(diag_get());
- xrow_decode_error(&header);
- last = diag_last_error(diag_get());
- isnt(last, NULL, "xrow_decode succeed: diag has been set");
- is(box_error_code(last), 159, "xrow_decode failed, took code from "
- "header");
- is(strcmp(last->errmsg, ""), 0, "xrow_decode failed, message is not "
- "decoded");
-
- check_plan();
-}
-
void
test_request_str()
{
@@ -460,14 +280,13 @@ main(void)
{
memory_init();
fiber_init(fiber_c_invoke);
- plan(4);
+ plan(3);
random_init();
test_iproto_constants();
test_greeting();
test_xrow_header_encode_decode();
- test_xrow_error_stack_decode();
test_request_str();
random_free();
diff --git a/test/unit/xrow.result b/test/unit/xrow.result
index c78109a..5ee92ad 100644
--- a/test/unit/xrow.result
+++ b/test/unit/xrow.result
@@ -1,4 +1,4 @@
-1..4
+1..3
1..40
ok 1 - round trip
ok 2 - roundtrip.version_id
@@ -53,29 +53,6 @@ ok 1 - subtests
ok 9 - decoded sync
ok 10 - decoded bodycnt
ok 2 - subtests
- 1..21
- ok 1 - xrow_decode succeed: diag has been set
- ok 2 - xrow_decode succeed: error is parsed
- ok 3 - xrow_decode succeed: diag has been set
- ok 4 - xrow_decode error stack suceed: e2 at the top of stack
- ok 5 - xrow_decode succeed: 'cause' is present in stack
- ok 6 - xrow_decode succeed: stack has been parsed
- ok 7 - xrow_decode succeed: only two errors in the stack
- ok 8 - xrow_decode succeed: diag has been set
- ok 9 - xrow_decode corrupted stack: stack contains map instead of array
- ok 10 - xrow_decode succeed: diag has been set
- ok 11 - xrow_decode corrupted stack: stack contains array values instead of maps
- ok 12 - xrow_decode succeed: diag has been set
- ok 13 - xrow_decode corrupted stack: stack's map keys are not uints
- ok 14 - xrow_decode succeed: diag has been set
- ok 15 - xrow_decode corrupted stack: stack's map wrong value type
- ok 16 - xrow_decode succeed: diag has been set
- ok 17 - xrow_decode last error code is default 0
- ok 18 - xrow_decode corrupted stack: stack's map wrong key
- ok 19 - xrow_decode succeed: diag has been set
- ok 20 - xrow_decode failed, took code from header
- ok 21 - xrow_decode failed, message is not decoded
-ok 3 - subtests
1..1
ok 1 - request_str
-ok 4 - subtests
+ok 3 - subtests
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH V6 09/10] iproto: rename IPROTO_ERROR and IPROTO_ERROR_STACK
2020-04-19 22:25 [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Leonid Vasiliev
` (7 preceding siblings ...)
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 08/10] error: make iproto errors reuse mp_error module Leonid Vasiliev
@ 2020-04-19 22:25 ` Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 10/10] error: fix iproto error stack overlapped by old error Leonid Vasiliev
` (2 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Leonid Vasiliev @ 2020-04-19 22:25 UTC (permalink / raw)
To: v.shpilevoy, alexander.turenko; +Cc: tarantool-patches
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
IPROTO_ERROR in fact is not an error. It is an error message.
Secondly, this key is deprecated in favor of IPROTO_ERROR_STACK,
which contains all attributes of the whole error stack. It uses
MP_ERROR MessagePack extenstion for that.
So IPROTO_ERROR is renamed to IPROTO_ERROR_24 (similar to how old
call was renamed to IPROTO_CALL_16). IPROTO_ERROR_STACK becomes
new IPROTO_ERROR.
Follow up #4398
---
src/box/iproto_constants.h | 4 ++--
src/box/lua/net_box.lua | 25 +++++++++++++------------
src/box/xrow.c | 10 +++++-----
test/box/iproto.result | 8 ++++----
test/box/iproto.test.lua | 6 +++---
5 files changed, 27 insertions(+), 26 deletions(-)
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index b57d565..f8eee0f 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -101,7 +101,7 @@ enum iproto_key {
/* Leave a gap between request keys and response keys */
IPROTO_DATA = 0x30,
- IPROTO_ERROR = 0x31,
+ IPROTO_ERROR_24 = 0x31,
/**
* IPROTO_METADATA: [
* { IPROTO_FIELD_NAME: name },
@@ -126,7 +126,7 @@ enum iproto_key {
/* Leave a gap between SQL keys and additional request keys */
IPROTO_REPLICA_ANON = 0x50,
IPROTO_ID_FILTER = 0x51,
- IPROTO_ERROR_STACK = 0x52,
+ IPROTO_ERROR = 0x52,
IPROTO_KEY_MAX
};
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 1e55eaa..1b2b6ac 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -44,8 +44,8 @@ local IPROTO_SQL_INFO_KEY = 0x42
local SQL_INFO_ROW_COUNT_KEY = 0
local IPROTO_FIELD_NAME_KEY = 0
local IPROTO_DATA_KEY = 0x30
-local IPROTO_ERROR_KEY = 0x31
-local IPROTO_ERROR_STACK = 0x52
+local IPROTO_ERROR_24 = 0x31
+local IPROTO_ERROR = 0x52
local IPROTO_GREETING_SIZE = 128
local IPROTO_CHUNK_KEY = 128
local IPROTO_OK_KEY = 0
@@ -150,7 +150,7 @@ local method_decoder = {
push = decode_push,
}
-local function decode_error_stack(raw_data)
+local function decode_error(raw_data)
local ptr = buffer_reg.acucp
ptr[0] = raw_data
local err = ffi.C.error_unpack_unsafe(ptr)
@@ -170,8 +170,8 @@ local function decode_error_stack(raw_data)
end
local response_decoder = {
- [IPROTO_ERROR_KEY] = decode,
- [IPROTO_ERROR_STACK] = decode_error_stack,
+ [IPROTO_ERROR_24] = decode,
+ [IPROTO_ERROR] = decode_error,
}
local function next_id(id) return band(id + 1, 0x7FFFFFFF) end
@@ -614,15 +614,16 @@ local function create_transport(host, port, user, password, callback,
end
assert(body_end == body_rpos, "invalid xrow length")
request.errno = band(status, IPROTO_ERRNO_MASK)
- -- IPROTO_ERROR_STACK comprises error encoded with
- -- IPROTO_ERROR_KEY, so we may ignore content of that key.
- if body[IPROTO_ERROR_STACK] ~= nil then
- request.response = body[IPROTO_ERROR_STACK]
+ -- IPROTO_ERROR comprises error encoded with
+ -- IPROTO_ERROR_24, so we may ignore content of that
+ -- key.
+ if body[IPROTO_ERROR] ~= nil then
+ request.response = body[IPROTO_ERROR]
assert(type(request.response) == 'cdata')
else
request.response = box.error.new({
code = request.errno,
- reason = body[IPROTO_ERROR_KEY]
+ reason = body[IPROTO_ERROR_24]
})
end
request.cond:broadcast()
@@ -800,7 +801,7 @@ local function create_transport(host, port, user, password, callback,
if hdr[IPROTO_STATUS_KEY] ~= 0 then
local body
body, body_end = decode(body_rpos)
- return error_sm(E_NO_CONNECTION, body[IPROTO_ERROR_KEY])
+ return error_sm(E_NO_CONNECTION, body[IPROTO_ERROR_24])
end
set_state('fetch_schema')
return iproto_schema_sm(hdr[IPROTO_SCHEMA_VERSION_KEY])
@@ -845,7 +846,7 @@ local function create_transport(host, port, user, password, callback,
if status ~= 0 then
local body
body, body_end = decode(body_rpos)
- return error_sm(E_NO_CONNECTION, body[IPROTO_ERROR_KEY])
+ return error_sm(E_NO_CONNECTION, body[IPROTO_ERROR_24])
end
if schema_version == nil then
schema_version = response_schema_version
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 06c6afb..1b3f3f9 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -379,7 +379,7 @@ iproto_header_encode(char *out, uint32_t type, uint64_t sync,
struct PACKED iproto_body_bin {
uint8_t m_body; /* MP_MAP */
- uint8_t k_data; /* IPROTO_DATA or IPROTO_ERROR */
+ uint8_t k_data; /* IPROTO_DATA or errors */
uint8_t m_data; /* MP_STR or MP_ARRAY */
uint32_t v_data_len; /* string length of array size */
};
@@ -496,9 +496,9 @@ static void
mpstream_iproto_encode_error(struct mpstream *stream, const struct error *error)
{
mpstream_encode_map(stream, 2);
- mpstream_encode_uint(stream, IPROTO_ERROR);
+ mpstream_encode_uint(stream, IPROTO_ERROR_24);
mpstream_encode_str(stream, error->errmsg);
- mpstream_encode_uint(stream, IPROTO_ERROR_STACK);
+ mpstream_encode_uint(stream, IPROTO_ERROR);
error_to_mpstream_noext(error, stream);
}
@@ -1109,7 +1109,7 @@ xrow_decode_error(struct xrow_header *row)
continue;
}
uint8_t key = mp_decode_uint(&pos);
- if (key == IPROTO_ERROR && mp_typeof(*pos) == MP_STR) {
+ if (key == IPROTO_ERROR_24 && mp_typeof(*pos) == MP_STR) {
/*
* Obsolete way of sending error responses.
* To be deprecated but still should be supported
@@ -1119,7 +1119,7 @@ xrow_decode_error(struct xrow_header *row)
const char *str = mp_decode_str(&pos, &len);
snprintf(error, sizeof(error), "%.*s", len, str);
box_error_set(__FILE__, __LINE__, code, error);
- } else if (key == IPROTO_ERROR_STACK) {
+ } else if (key == IPROTO_ERROR) {
struct error *e = error_unpack_unsafe(&pos);
if (e == NULL)
goto error;
diff --git a/test/box/iproto.result b/test/box/iproto.result
index e4fe684..4b648d3 100644
--- a/test/box/iproto.result
+++ b/test/box/iproto.result
@@ -25,10 +25,10 @@ IPROTO_FUNCTION_NAME = 0x22
IPROTO_TUPLE = 0x21
---
...
-IPROTO_ERROR = 0x31
+IPROTO_ERROR_24 = 0x31
---
...
-IPROTO_ERROR_STACK = 0x52
+IPROTO_ERROR = 0x52
---
...
IPROTO_ERROR_CODE = 0x01
@@ -92,11 +92,11 @@ sock:close()
...
-- Both keys (obsolete and stack ones) are present in response.
--
-assert(response.body[IPROTO_ERROR_STACK] ~= nil)
+assert(response.body[IPROTO_ERROR] ~= nil)
---
- true
...
-assert(response.body[IPROTO_ERROR] ~= nil)
+assert(response.body[IPROTO_ERROR_24] ~= nil)
---
- true
...
diff --git a/test/box/iproto.test.lua b/test/box/iproto.test.lua
index ec715e9..4af39cc 100644
--- a/test/box/iproto.test.lua
+++ b/test/box/iproto.test.lua
@@ -9,8 +9,8 @@ IPROTO_SYNC = 0x01
IPROTO_CALL = 0x0A
IPROTO_FUNCTION_NAME = 0x22
IPROTO_TUPLE = 0x21
-IPROTO_ERROR = 0x31
-IPROTO_ERROR_STACK = 0x52
+IPROTO_ERROR_24 = 0x31
+IPROTO_ERROR = 0x52
IPROTO_ERROR_CODE = 0x01
IPROTO_ERROR_MESSAGE = 0x02
@@ -49,7 +49,7 @@ sock:close()
-- Both keys (obsolete and stack ones) are present in response.
--
-assert(response.body[IPROTO_ERROR_STACK] ~= nil)
assert(response.body[IPROTO_ERROR] ~= nil)
+assert(response.body[IPROTO_ERROR_24] ~= nil)
box.schema.user.revoke('guest', 'read,write,execute', 'universe')
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH V6 10/10] error: fix iproto error stack overlapped by old error
2020-04-19 22:25 [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Leonid Vasiliev
` (8 preceding siblings ...)
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 09/10] iproto: rename IPROTO_ERROR and IPROTO_ERROR_STACK Leonid Vasiliev
@ 2020-04-19 22:25 ` Leonid Vasiliev
2020-04-20 0:26 ` [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Vladislav Shpilevoy
2020-04-20 8:30 ` Kirill Yukhin
11 siblings, 0 replies; 18+ messages in thread
From: Leonid Vasiliev @ 2020-04-19 22:25 UTC (permalink / raw)
To: v.shpilevoy, alexander.turenko; +Cc: tarantool-patches
Fix possible overlap of IPROTO_ERROR by IPROTO_ERROR_24.
This was possible because messages are transmitted in a map and
an order is not defined. IPROTO_ERROR_24 could be parsed after
the IPROTO_ERROR, and could throw it away.
---
src/box/xrow.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/box/xrow.c b/src/box/xrow.c
index 1b3f3f9..06473a8 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -1102,6 +1102,7 @@ xrow_decode_error(struct xrow_header *row)
if (mp_typeof(*pos) != MP_MAP)
goto error;
map_size = mp_decode_map(&pos);
+ bool is_stack_parsed = false;
for (uint32_t i = 0; i < map_size; i++) {
if (mp_typeof(*pos) != MP_UINT) {
mp_next(&pos); /* key */
@@ -1117,12 +1118,15 @@ xrow_decode_error(struct xrow_header *row)
*/
uint32_t len;
const char *str = mp_decode_str(&pos, &len);
- snprintf(error, sizeof(error), "%.*s", len, str);
- box_error_set(__FILE__, __LINE__, code, error);
+ if (!is_stack_parsed) {
+ snprintf(error, sizeof(error), "%.*s", len, str);
+ box_error_set(__FILE__, __LINE__, code, error);
+ }
} else if (key == IPROTO_ERROR) {
struct error *e = error_unpack_unsafe(&pos);
if (e == NULL)
goto error;
+ is_stack_parsed = true;
diag_set_error(diag_get(), e);
} else {
mp_next(&pos);
--
2.7.4
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH V6 00/10] Extending error functionality
2020-04-19 22:25 [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Leonid Vasiliev
` (9 preceding siblings ...)
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 10/10] error: fix iproto error stack overlapped by old error Leonid Vasiliev
@ 2020-04-20 0:26 ` Vladislav Shpilevoy
2020-04-20 8:05 ` lvasiliev
2020-04-20 8:05 ` Kirill Yukhin
2020-04-20 8:30 ` Kirill Yukhin
11 siblings, 2 replies; 18+ messages in thread
From: Vladislav Shpilevoy @ 2020-04-20 0:26 UTC (permalink / raw)
To: Kirill Yukhin; +Cc: tarantool-patches
Hi!
In short: formally LGTM.
Long version:
I don't like doing and reviewing patches in such a hurry.
This feature clearly lacked planning, design, and discussion
with community, RFC for the final version before its
implementation.
It is still unfinished because of underdesigned traceback
feature, because of payload absence. IMO, MP_EXT is also an
overkill. Tuples live fine as MP_ARRAY, and they are the most
used type. We should have gone for simple MP_MAP, without
MP_EXT. Just a map.
It is worth mentioning separately, how hard it is to use the
error marshaling now, because of this session setting. And
there still is no way to enable the feature without touching
the session, even if all my connectors support it. As I
mentioned, enabling it for every session manually is a
non-trivial task for a user.
But we have releases coming, so lets push all in whatever
state it is, of course.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH V6 00/10] Extending error functionality
2020-04-20 0:26 ` [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Vladislav Shpilevoy
@ 2020-04-20 8:05 ` lvasiliev
2020-04-20 8:05 ` Kirill Yukhin
1 sibling, 0 replies; 18+ messages in thread
From: lvasiliev @ 2020-04-20 8:05 UTC (permalink / raw)
To: Vladislav Shpilevoy, Kirill Yukhin; +Cc: tarantool-patches
On 20.04.2020 3:26, Vladislav Shpilevoy wrote:
> Hi!
>
> In short: formally LGTM.
>
> Long version:
>
> I don't like doing and reviewing patches in such a hurry.
> This feature clearly lacked planning, design, and discussion
> with community, RFC for the final version before its
> implementation.
>
> It is still unfinished because of underdesigned traceback
> feature, because of payload absence. IMO, MP_EXT is also an
> overkill. Tuples live fine as MP_ARRAY, and they are the most
> used type. We should have gone for simple MP_MAP, without
> MP_EXT. Just a map.
>
> It is worth mentioning separately, how hard it is to use the
> error marshaling now, because of this session setting. And
> there still is no way to enable the feature without touching
> the session, even if all my connectors support it. As I
> mentioned, enabling it for every session manually is a
> non-trivial task for a user.
>
> But we have releases coming, so lets push all in whatever
> state it is, of course.
Hi! Thanks you for the help in implementation.
I tried to write the RFC but my attempt was ignored by everyone.
The first implementation used a map and it was said that MP_EXT
should be used. I will try to take into account the experience
gained and not proceed with the implementation of non-trivial
tasks without LGTM on RFC.
Thanks again for your help.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH V6 00/10] Extending error functionality
2020-04-20 0:26 ` [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Vladislav Shpilevoy
2020-04-20 8:05 ` lvasiliev
@ 2020-04-20 8:05 ` Kirill Yukhin
2020-04-21 19:03 ` Konstantin Osipov
1 sibling, 1 reply; 18+ messages in thread
From: Kirill Yukhin @ 2020-04-20 8:05 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Hello,
On 20 апр 02:26, Vladislav Shpilevoy wrote:
> Hi!
>
> In short: formally LGTM.
>
> Long version:
>
> I don't like doing and reviewing patches in such a hurry.
> This feature clearly lacked planning, design, and discussion
> with community, RFC for the final version before its
> implementation.
That is true. Size of the feature was underestimated.
> It is still unfinished because of underdesigned traceback
> feature, because of payload absence. IMO, MP_EXT is also an
> overkill. Tuples live fine as MP_ARRAY, and they are the most
> used type. We should have gone for simple MP_MAP, without
> MP_EXT. Just a map.
We already broken connectors by introducing UUID (w/ MP_EXT).
> It is worth mentioning separately, how hard it is to use the
> error marshaling now, because of this session setting. And
> there still is no way to enable the feature without touching
> the session, even if all my connectors support it. As I
> mentioned, enabling it for every session manually is a
> non-trivial task for a user.
I guess we can switch it on by default in future releases.
> But we have releases coming, so lets push all in whatever
> state it is, of course.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH V6 00/10] Extending error functionality
2020-04-19 22:25 [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Leonid Vasiliev
` (10 preceding siblings ...)
2020-04-20 0:26 ` [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Vladislav Shpilevoy
@ 2020-04-20 8:30 ` Kirill Yukhin
11 siblings, 0 replies; 18+ messages in thread
From: Kirill Yukhin @ 2020-04-20 8:30 UTC (permalink / raw)
To: Leonid Vasiliev; +Cc: tarantool-patches, v.shpilevoy
Hello,
On 20 апр 01:25, Leonid Vasiliev wrote:
> https://github.com/tarantool/tarantool/issues/4398
> https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4398-expose-error-module-5
LGTM.
I've checked the patchset into master.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH V6 00/10] Extending error functionality
2020-04-20 8:05 ` Kirill Yukhin
@ 2020-04-21 19:03 ` Konstantin Osipov
2020-04-22 16:17 ` lvasiliev
0 siblings, 1 reply; 18+ messages in thread
From: Konstantin Osipov @ 2020-04-21 19:03 UTC (permalink / raw)
To: Kirill Yukhin; +Cc: tarantool-patches, Vladislav Shpilevoy
* Kirill Yukhin <kyukhin@tarantool.org> [20/04/20 11:08]:
> > It is still unfinished because of underdesigned traceback
> > feature, because of payload absence. IMO, MP_EXT is also an
> > overkill. Tuples live fine as MP_ARRAY, and they are the most
> > used type. We should have gone for simple MP_MAP, without
> > MP_EXT. Just a map.
>
> We already broken connectors by introducing UUID (w/ MP_EXT).
UUID change was planned. This was not. UUID is a fundamental data
type. struct error is just an object.
This was a rushed up decision. Better revert it and go back to
MP_MAP.
> > It is worth mentioning separately, how hard it is to use the
> > error marshaling now, because of this session setting. And
> > there still is no way to enable the feature without touching
> > the session, even if all my connectors support it. As I
> > mentioned, enabling it for every session manually is a
> > non-trivial task for a user.
>
> I guess we can switch it on by default in future releases.
>
> > But we have releases coming, so lets push all in whatever
> > state it is, of course.
>
> --
> Regards, Kirill Yukhin
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH V6 00/10] Extending error functionality
2020-04-21 19:03 ` Konstantin Osipov
@ 2020-04-22 16:17 ` lvasiliev
2020-04-22 17:23 ` Konstantin Osipov
0 siblings, 1 reply; 18+ messages in thread
From: lvasiliev @ 2020-04-22 16:17 UTC (permalink / raw)
To: Konstantin Osipov, Kirill Yukhin, Vladislav Shpilevoy, tarantool-patches
On 21.04.2020 22:03, Konstantin Osipov wrote:
> * Kirill Yukhin <kyukhin@tarantool.org> [20/04/20 11:08]:
>>> It is still unfinished because of underdesigned traceback
>>> feature, because of payload absence. IMO, MP_EXT is also an
>>> overkill. Tuples live fine as MP_ARRAY, and they are the most
>>> used type. We should have gone for simple MP_MAP, without
>>> MP_EXT. Just a map.
>>
>> We already broken connectors by introducing UUID (w/ MP_EXT).
I’ll add for clarity: "MP_ERROR doesn't break connectors because
IPROTO_ERROR doesn't use MP_EXT, MP_ERROR used at IPROTO_BODY after
client do session_settings update."
>
> UUID change was planned. This was not. UUID is a fundamental data
> type. struct error is just an object.
>
> This was a rushed up decision. Better revert it and go back to
> MP_MAP.
>>> It is worth mentioning separately, how hard it is to use the
>>> error marshaling now, because of this session setting. And
>>> there still is no way to enable the feature without touching
>>> the session, even if all my connectors support it. As I
>>> mentioned, enabling it for every session manually is a
>>> non-trivial task for a user.
>>
>> I guess we can switch it on by default in future releases.
>>
>>> But we have releases coming, so lets push all in whatever
>>> state it is, of course.
>>
>> --
>> Regards, Kirill Yukhin
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH V6 00/10] Extending error functionality
2020-04-22 16:17 ` lvasiliev
@ 2020-04-22 17:23 ` Konstantin Osipov
0 siblings, 0 replies; 18+ messages in thread
From: Konstantin Osipov @ 2020-04-22 17:23 UTC (permalink / raw)
To: lvasiliev; +Cc: Vladislav Shpilevoy, tarantool-patches
* lvasiliev <lvasiliev@tarantool.org> [20/04/22 19:20]:
> I’ll add for clarity: "MP_ERROR doesn't break connectors because
> IPROTO_ERROR doesn't use MP_EXT, MP_ERROR used at IPROTO_BODY after
> client do session_settings update."
There aren't that many things in the world which can break when
turned off, it's of course good that MP_ERROR is not one of them.
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-04-22 17:23 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-19 22:25 [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 01/10] error: add custom error type Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 02/10] session: add offset to SQL session settings array Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 03/10] error: add session setting for error type marshaling Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 04/10] error: update constructors of some errors Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 05/10] box: move Lua MP_EXT decoder from tuple.c Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 06/10] error: add error MsgPack encoding Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 07/10] error: export error_unref() function Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 08/10] error: make iproto errors reuse mp_error module Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 09/10] iproto: rename IPROTO_ERROR and IPROTO_ERROR_STACK Leonid Vasiliev
2020-04-19 22:25 ` [Tarantool-patches] [PATCH V6 10/10] error: fix iproto error stack overlapped by old error Leonid Vasiliev
2020-04-20 0:26 ` [Tarantool-patches] [PATCH V6 00/10] Extending error functionality Vladislav Shpilevoy
2020-04-20 8:05 ` lvasiliev
2020-04-20 8:05 ` Kirill Yukhin
2020-04-21 19:03 ` Konstantin Osipov
2020-04-22 16:17 ` lvasiliev
2020-04-22 17:23 ` Konstantin Osipov
2020-04-20 8:30 ` Kirill Yukhin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox