From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 E47944696C3 for ; Wed, 1 Apr 2020 19:09:15 +0300 (MSK) Date: Wed, 1 Apr 2020 16:09:14 +0000 From: Nikita Pettik Message-ID: <20200401160914.GC26447@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v2 06/10] box: introduce stacked diagnostic area List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 28 Mar 19:40, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > See 7 comments below. > > On 25/03/2020 02:43, 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 (cause and effort correspondingly) have been added to > > 1. effort -> effect. Fixed. > > 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. > > > > diff --git a/src/box/key_list.c b/src/box/key_list.c > > index 3d736b55f..81eb501a5 100644 > > --- a/src/box/key_list.c > > +++ b/src/box/key_list.c > > @@ -64,7 +64,7 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple, > > /* 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) : "", > > + space != NULL ? space_name(space) : "", > > diag_last_error(diag_get())->errmsg); > > return -1; > > } > > @@ -75,7 +75,7 @@ key_list_iterator_create(struct key_list_iterator *it, struct tuple *tuple, > > 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) : "", > > + space != NULL ? space_name(space) : "", > > diag_last_error(diag_get())->errmsg); > > return -1; > > } > > diff --git a/src/box/lua/call.c b/src/box/lua/call.c > > index f1bbde7f0..92575374d 100644 > > --- a/src/box/lua/call.c > > +++ b/src/box/lua/call.c > > @@ -688,8 +688,8 @@ func_persistent_lua_load(struct func_lua *func) > > 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); > > + func->base.def->name, > > + diag_last_error(diag_get())->errmsg); > > goto end; > > 2. Do you really need the 3 hunks above? Seems like unnecessary > refactoring. Fixed (extra diff occured due to splitting into two patches). > > diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c > > index c350abb4a..57da5da44 100644 > > --- a/src/lib/core/diag.c > > +++ b/src/lib/core/diag.c > > @@ -34,6 +34,43 @@ > > /* Must be set by the library user */ > > struct error_factory *error_factory = NULL; > > > > +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->cause; > > + } > > 3. Or we could remove 'prev' from its current stack gracefully, > since the list is doubly linked. Then cycles become impossible. > Is there a reason not to do that? I mean, we remove 'prev' > from its old stack by changing its 'effect' already anyway. > > Can do the same for the other pointer, and no need to check > for cycles anymore. > I can only think of a problem when someone makes > > e1 = box.error.new() > e2 = box.error.new() > e2:set_prev(e1) > > and then > > e4 = box.error.new() > e4:set_prev(e1) > > assuming that e1 will be copied. But it will steal e1 from > its old stack. Even with your current solution. Maybe to make > it more protected against dummies we should forbid moving error > from one stack to another? So an error can't change its 'cause' > and 'effect' once they become not NULL. In that way we eliminate > any uncertainties and in future can decide whether we want to copy > in set_prev() when necessary, or move from previous stack. Or we > will keep this forbidden because probably no one does moving > between stacks, nor the example I showed above. This also > significantly simplifies tests and reduces probability of having > a bug there. What are your thoughts? I'm okay with current approach. It is all about documentation - the better feature is documented, the easier it turns out to be in terms of usage. There's always way to misuse any feature however well designed. If you insist on some changes - let me know. > > + /* > > + * At once error can feature only one reason. > > + * So unlink previous 'cause' node. > > + */ > > + if (e->cause != NULL) { > > + e->cause->effect = NULL; > > + error_unref(e->cause); > > + } > > + /* Set new 'prev' node. */ > > + e->cause = prev; > > + /* > > + * Unlink new 'effect' node from its old list of 'cause' > > + * errors. nil can be also passed as an argument. > > + */ > > + if (prev != NULL) { > > + error_unlink_effect(prev); > > + prev->effect = e; > > + error_ref(prev); > > 4. Better call error_ref() before error_unlink_effect(). To > make it impossible for prev to be deleted inside unlink() and > be used after free. I can't find how can be it done now, but by > moving ref() a bit above we will make it impossible in future > as well. Agree, fixed. > > diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h > > index 7e1e1a174..675b9c6f1 100644 > > --- a/src/lib/core/diag.h > > +++ b/src/lib/core/diag.h> @@ -96,11 +113,56 @@ static inline void > > > > +/** > > + * Set previous error: cut @a prev from its previous 'tail' of > > + * causes and link to the one @a e belongs to. Note that all > > + * previous errors starting from @a prev->cause are transferred > > + * with it as well (i.e. causes for given error are not erased). > > + * For instance: > > + * e1 -> e2 -> NULL; e3 -> e4 -> NULL; > > + * e2:set_effect(e3): e1 -> e2 -> e3 -> e4 -> NULL > > + * > > + * @a effect can be NULL. To be used as ffi method in lua/error.lua. > > 5. Out of 66 and double whitespace before 'NULL'. Fixed. > > diff --git a/test/box/error.result b/test/box/error.result > > index a19a7044f..ff2b8b270 100644 > > --- a/test/box/error.result > > +++ b/test/box/error.result > > @@ -502,6 +502,290 @@ box.error() > > +-- In case error is destroyed, it unrefs reference counter > > +-- of its previous error. In turn, box.error.clear() refs/unrefs > > +-- only head and doesn't touch other errors. > > +-- > > +e2:set_prev(nil) > > + | --- > > + | ... > > +box.error.set(e1) > > + | --- > > + | ... > > +assert(box.error.last() == e1) > > + | --- > > + | - true > > + | ... > > +assert(box.error.last().prev == e2) > > + | --- > > + | - true > > + | ... > > +box.error.clear() > > + | --- > > + | ... > > +assert(box.error.last() == nil) > > + | --- > > + | - true > > + | ... > > +assert(e1.prev == e2) > > + | --- > > + | - true > > + | ... > > +assert(e2.code == 111) > > 6. Double whitespace. Macbook keyboard, huh? Fixed. > > diff --git a/test/engine/func_index.result b/test/engine/func_index.result > > index 84cb83022..159158f1c 100644 > > --- a/test/engine/func_index.result > > +++ b/test/engine/func_index.result > > @@ -261,12 +261,6 @@ box.schema.func.create('runtimeerror', {body = lua_code, is_deterministic = true > > idx = s:create_index('idx', {func = box.func.runtimeerror.id, parts = {{1, 'string'}}}) > > | --- > > | ... > > -s:insert({1}) > > - | --- > > - | - error: 'Failed to build a key for functional index ''idx'' of space ''withdata'': > > - | [string "return function(tuple) local ..."]:1: attempt to call > > - | global ''require'' (a nil value)' > > - | ... > > idx:drop() > > 7. Why do you drop it here? The commit does not change any > existing places setting errors. So it should not lead to > any test changes. Sorry, removed (accidental diff due to splitting into two commits).