From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D6DAC469719 for ; Thu, 20 Feb 2020 00:10:48 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: Date: Wed, 19 Feb 2020 22:10:46 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik , tarantool-patches@dev.tarantool.org 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 > #include > #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.