[Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Feb 20 00:10:46 MSK 2020
On 19/02/2020 15:16, Nikita Pettik wrote:
> In terms of implementation, now struct error objects can be organized
> into double-linked lists. To achieve this pointers to the next and
> previous elements have been added to struct error. It is worth
> mentioning that already existing rlist and stailq list implementations
> are not suitable: rlist is cycled list, as a result it is impossible to
> start iteration over the list from random list entry and finish it at
> the logical end of the list; stailq is single-linked list leaving no
> possibility to remove elements from the middle of the list.
>
> As a part of C interface, box_error_add() has been introduced. In
> contrast to box_error_set() it does not replace last raised error, but
> instead it adds error to the list of diagnostic errors having already
> been set. If error is to be deleted (its reference counter hits 0 value)
> it is unlinked from the list it belongs to and destroyed.
>
> To organize errors into lists in Lua, table representing error object in
> Lua now has .prev field (corresponding to 'previous' error) and method
> :set_prev(e). The latter accepts error object (i.e. created via
> box.error.new() or box.error.last()) and nil value. Both field .prev and
> :set_prev() method are implemented as ffi functions. Also note that
> cycles are now allowed while organizing errors into lists:
> e1 -> e2 -> e3; e3:set_prev(e1) -- would lead to error.
>
> Part of #1148
> ---
> extra/exports | 2 +
> src/box/key_list.c | 16 +--
> src/box/lua/call.c | 6 +-
> src/lib/core/diag.c | 51 +++++++++
> src/lib/core/diag.h | 95 +++++++++++++++-
> src/lua/error.lua | 40 +++++++
> test/box/misc.result | 233 ++++++++++++++++++++++++++++++++++++++++
> test/box/misc.test.lua | 89 +++++++++++++++
> test/engine/func_index.result | 50 +++++++--
> test/engine/func_index.test.lua | 7 ++
> 10 files changed, 568 insertions(+), 21 deletions(-)
>
> diff --git a/extra/exports b/extra/exports
> index 7b84a1452..94cbdd210 100644
> --- a/extra/exports
> +++ b/extra/exports
> @@ -246,6 +246,8 @@ clock_monotonic64
> clock_process64
> clock_thread64
> string_strip_helper
> +error_prev
> +error_set_prev
>
> # Lua / LuaJIT
>
> diff --git a/src/box/key_list.c b/src/box/key_list.c
> index 3d736b55f..a766ce0ec 100644
> --- a/src/box/key_list.c
> +++ b/src/box/key_list.c
> @@ -63,9 +63,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
> if (rc != 0) {
> /* Can't evaluate function. */
> struct space *space = space_by_id(index_def->space_id);
> - diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> - space ? space_name(space) : "",
> - diag_last_error(diag_get())->errmsg);
> + diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> + space != NULL ? space_name(space) : "",
> + "can't evaluate function");
> return -1;
> }
> uint32_t key_data_sz;
> @@ -74,9 +74,9 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple,
> if (key_data == NULL) {
> struct space *space = space_by_id(index_def->space_id);
> /* Can't get a result returned by function . */
> - diag_set(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> - space ? space_name(space) : "",
> - diag_last_error(diag_get())->errmsg);
> + diag_add(ClientError, ER_FUNC_INDEX_FUNC, index_def->name,
> + space != NULL ? space_name(space) : "",
> + "can't get a value returned by function");
> return -1;
> }
>
> @@ -170,9 +170,9 @@ key_list_iterator_next(struct key_list_iterator *it, const char **value)
> * The key doesn't follow functional index key
> * definition.
> */
> - diag_set(ClientError, ER_FUNC_INDEX_FORMAT, it->index_def->name,
> + diag_add(ClientError, ER_FUNC_INDEX_FORMAT, it->index_def->name,
> space ? space_name(space) : "",
> - diag_last_error(diag_get())->errmsg);
> + "key does not follow functional index definition");
> return -1;
> }
>
> diff --git a/src/box/lua/call.c b/src/box/lua/call.c
> index f1bbde7f0..5d3579eff 100644
> --- a/src/box/lua/call.c
> +++ b/src/box/lua/call.c
> @@ -687,9 +687,9 @@ func_persistent_lua_load(struct func_lua *func)
> if (func->base.def->is_sandboxed) {
> if (prepare_lua_sandbox(tarantool_L, default_sandbox_exports,
> nelem(default_sandbox_exports)) != 0) {
> - diag_set(ClientError, ER_LOAD_FUNCTION,
> - func->base.def->name,
> - diag_last_error(diag_get())->errmsg);
> + diag_add(ClientError, ER_LOAD_FUNCTION,
> + func->base.def->name,
> + "can't prepare a Lua sandbox");
> goto end;
> }
> } else {
> diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
> index c350abb4a..1776dc8cf 100644
> --- a/src/lib/core/diag.c
> +++ b/src/lib/core/diag.c
> @@ -34,6 +34,55 @@
> /* Must be set by the library user */
> struct error_factory *error_factory = NULL;
>
> +struct error *
> +error_prev(struct error *e)
> +{
> + assert(e != NULL);
> + return e->next;
> +}
> +
> +int
> +error_set_prev(struct error *e, struct error *prev)
> +{
> + /*
> + * Make sure that adding error won't result in cycles.
> + * Don't bother with sophisticated cycle-detection
> + * algorithms, simple iteration is OK since as a rule
> + * list contains a dozen errors at maximum.
> + */
> + struct error *tmp = prev;
> + while (tmp != NULL) {
> + if (tmp == e)
> + return -1;
> + tmp = tmp->next;
> + }
> + /*
> + * At once error can be reason for only one error.
> + * So unlink previous 'prev' node.
> + *
> + * +--------+ NEXT +--------+
> + * | e | ---> |old prev|
> + * +--------+ +--------+
> + * ^ |
> + * | PREV |
> + * +-------X-------+
> + *
> + */
> + if (e->next != NULL)
> + e->next->prev = NULL;
> + /* Set new 'prev' node. */
> + e->next = prev;
> + /*
> + * Unlink new 'prev' node from its old stack.
> + * nil can be also passed as an argument.
> + */
> + if (prev != NULL) {
> + error_unlink_tail(prev);
> + prev->prev = e;
> + }
> + return 0;
> +}
> +
> void
> error_create(struct error *e,
> error_f destroy, error_f raise, error_f log,
> @@ -53,6 +102,8 @@ error_create(struct error *e,
> e->line = 0;
> }
> e->errmsg[0] = '\0';
> + e->prev = NULL;
> + e->next = NULL;
> }
>
> struct diag *
> diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
> index 7e1e1a174..5271733cb 100644
> --- a/src/lib/core/diag.h
> +++ b/src/lib/core/diag.h
> @@ -37,6 +37,7 @@
> #include <stdbool.h>
> #include <assert.h>
> #include "say.h"
> +#include "small/rlist.h"
>
> #if defined(__cplusplus)
> extern "C" {
> @@ -84,6 +85,17 @@ struct error {
> char file[DIAG_FILENAME_MAX];
> /* Error description. */
> char errmsg[DIAG_ERRMSG_MAX];
> + /**
> + * Link to the next and previous errors.
> + * RLIST implementation is not really suitable here
> + * since it is organized as circular list. In such
> + * a case it is impossible to start an iteration
> + * from any node and finish at the logical end of the
> + * list. Double-linked list is required to allow deletion
> + * from the middle of the list.
> + */
> + struct error *next;
> + struct error *prev;
> };
>
> static inline void
> @@ -92,15 +104,61 @@ error_ref(struct error *e)
> e->refs++;
> }
>
> +/**
> + * Unlink error from any error which point to it. For instance:
> + * e1 -> e2 -> e3 -> e4 (e1:set_prev(e2); e2:set_prev(33) ...)
> + * unlink(e3): e1 -> e2 -> NULL; e3 -> e4 -> NULL
> + */
> +static inline void
> +error_unlink_tail(struct error *e)
> +{
> + if (e->prev != NULL)
> + e->prev->next = NULL;
> + e->prev = NULL;
> +}
> +
> +
> static inline void
> error_unref(struct error *e)
> {
> assert(e->refs > 0);
> --e->refs;
> - if (e->refs == 0)
> + if (e->refs == 0) {
> + /* Unlink error from the list completely. */
> + if (e->prev != NULL)
> + e->prev->next = e->next;
> + if (e->next != NULL)
> + e->next->prev = e->prev;
> + e->next = NULL;
> + e->prev = NULL;
> e->destroy(e);
> + }
> }
>
> +/**
> + * Return previous (for given error) error. Result can be NULL
> + * which means that there's no previous error. Simple getter
> + * to be used as ffi method in lua/error.lua.
> + */
> +struct error *
> +error_prev(struct error *e);
> +
> +/**
> + * Set previous error: remove @a prev from its current stack and
> + * link to the one @a e belongs to. Note that all previous errors
> + * starting from @a prev->next are transferred with it as well
> + * (i.e. reasons for given error are not erased). For instance:
> + * e1 -> e2 -> NULL; e3 -> e4 -> NULL;
> + * e2:set_prev(e3): e1 -> e2 -> e3 -> e4 -> NULL
> + *
> + * @a prev can be NULL. To be used as ffi method in lua/error.lua.
> + *
> + * @retval -1 in case adding @a prev results in list cycles;
> + * 0 otherwise.
> + */
> +int
> +error_set_prev(struct error *e, struct error *prev);
> +
> NORETURN static inline void
> error_raise(struct error *e)
> {
> @@ -163,7 +221,12 @@ diag_clear(struct diag *diag)
> {
> if (diag->last == NULL)
> return;
> - error_unref(diag->last);
> + struct error *last = diag->last;
> + while (last != NULL) {
> + struct error *tmp = last->next;
> + error_unref(last);
> + last = tmp;
> + }
> diag->last = NULL;
Hi! Please, read what I wrote in the ticket about box.error.new(). You
should not clear all the errors. The diag owns only the head ref. Head
destruction should unref a next error. Destruction of a next error
should unref next next error and so on.
More information about the Tarantool-patches
mailing list