[Tarantool-patches] [PATCH v2 06/10] box: introduce stacked diagnostic area
Nikita Pettik
korablev at tarantool.org
Thu Apr 2 20:42:32 MSK 2020
On 02 Apr 02:29, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
>
> See 2 comments below.
>
> > 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;
> > if (to_delete->effect != NULL)
> > to_delete->effect->cause = to_delete->cause;
>
> 1. Looks like 'effect' should be always NULL here. Because
> if it is not NULL, then effect->cause == e, and it would keep a
> reference, making e->refs == 0 impossible. I applied the diff
> below and all the tests passed.
Agree, applied.
> ====================
> diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
> index 3a817a659..276bf2362 100644
> --- a/src/lib/core/diag.h
> +++ b/src/lib/core/diag.h
> @@ -117,12 +117,11 @@ error_unref(struct error *e)
> while (--to_delete->refs == 0) {
> /* Unlink error from lists completely.*/
> struct error *cause = to_delete->cause;
> - if (to_delete->effect != NULL)
> - to_delete->effect->cause = to_delete->cause;
> - if (to_delete->cause != NULL)
> - to_delete->cause->effect = to_delete->effect;
> - to_delete->cause = NULL;
> - to_delete->effect = NULL;
> + 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;
> ====================
> > }>>> 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.
>
> 2. I don't insist. Can only recommend. And I would strongly recommend
> to cut the ability to move errors between stacks. I see only pros in
> cutting that. Because it is never needed (I can't imagine a sane case),
> can be surprising for someone, and complicates the code. The server
> code. Makes it easier to leave a bug here. Another reason is that it is
> easier to add something later if someone will need it, than to forbid
> something after introduction.
>
> Besides, it seems we could even make the error list unidirectional then.
> Instead of 'effect' pointer we could keep a flag saying whether this error
> belongs to a stack. No more complex handling of when to do ref/unref/cycles
> and so on. Even bigger simplification.
>
> I decided to try that and got a patch. See it attached to the email as
> 'unidirect.txt', and below. I didn't push it on the branch so as not to
> change next commits.
>
> On the summary, I made the list unidirectional, removed the cycle
> detection and forbidden any chance of creating it by banning change of
> error object when it is not on top of the stack. So a stack should be
> built always in one order - from the cause of all causes to top. The
> only thing you can do is to cut a tail of a stack. All more complex
> actions which may be needed in very rare cases still can be implemented
> by users using the provided basic API.
>
> That made the patch 110 lines shorter.
Size of the patch does not always define its readability.
> Also a small comment about cycle detection in your patch. Here I
> would strongly-strongly recommend to avoid it when it is clearly
> not necessary.
To be honest, I don't understand why do you care about this opimization so
much. Linking errors into list is unlikely to be hot execution path, so nobody
should bother with such minor improvement here. Instead, keeping code clear,
simple and straightforward (your diff contains more branches and looks a bit
more complicated to me) must be the main goal. Applied diff below only due to
your recommendation.
> You can't create a cycle, if e->effect and prev->effect are NULL.
> Which is the case in 99% of cases. Even if prev->cause is not NULL,
> you still can't create a cycle when both effects are NULL. See diff
> below and attached as 'opt-cycle.txt'. That skips 'prev' iteration
> even if it is a whole another stack, but 'prev' is its top.
>
> Optimized cycle detection:
> ====================
> diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
> index ed8beb58d..64ff5e4d6 100644
> --- a/src/lib/core/diag.c
> +++ b/src/lib/core/diag.c
> @@ -43,11 +43,28 @@ error_set_prev(struct error *e, struct error *prev)
> * 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)
> + if (prev != NULL) {
> + if (e == prev)
> return -1;
> - tmp = tmp->cause;
> + if (prev->effect != NULL || e->effect != NULL) {
> + /*
> + * e and prev already compared, so start
> + * from prev->cause.
> + */
> + struct error *tmp = prev->cause;
> + while (tmp != NULL) {
> + if (tmp == e)
> + return -1;
> + tmp = tmp->cause;
> + }
> + /*
> + * Unlink new 'effect' node from its old
> + * list of 'cause' errors.
> + */
> + error_unlink_effect(prev);
> + }
> + error_ref(prev);
> + prev->effect = e;
> }
> /*
> * At once error can feature only one reason.
>
>
> Simplified error list patch:
Still, I haven't changed my mind. I didn't apply diff below, sorry.
I'll leave it to the second reviewer.
> ====================
> diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
> index ed8beb58d..69a19d91f 100644
> --- a/src/lib/core/diag.c
> +++ b/src/lib/core/diag.c
> @@ -37,37 +37,32 @@ 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)
> + if (e == prev)
> + return -1;
> + if (prev != NULL) {
> + /*
> + * It is not allowed to change middle of the error
> + * stack. Except when the tail is cut. Not
> + * replaced. Reason is to make the code simpler,
> + * and avoid any necessity to detect cycles. In
> + * that implementation cycles are not allowed, and
> + * this guarantee costs nothing.
> + */
> + if (prev->has_effect || e->has_effect)
> return -1;
> - tmp = tmp->cause;
> + prev->has_effect = true;
> + error_ref(prev);
> }
> /*
> * At once error can feature only one reason.
> * So unlink previous 'cause' node.
> */
> if (e->cause != NULL) {
> - e->cause->effect = NULL;
> + e->cause->has_effect = false;
> 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_ref(prev);
> - error_unlink_effect(prev);
> - prev->effect = e;
> - }
> return 0;
> }
>
> @@ -91,7 +86,7 @@ error_create(struct error *e,
> }
> e->errmsg[0] = '\0';
> e->cause = NULL;
> - e->effect = NULL;
> + e->has_effect = false;
> }
>
> struct diag *
> diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
> index 3a817a659..52987befa 100644
> --- a/src/lib/core/diag.h
> +++ b/src/lib/core/diag.h
> @@ -85,22 +85,20 @@ struct error {
> /* Error description. */
> char errmsg[DIAG_ERRMSG_MAX];
> /**
> - * Link to the cause and effect of given error. The cause
> - * creates the effect:
> + * Cause of the given error..
> * e1 = box.error.new({code = 0, reason = 'e1'})
> * e2 = box.error.new({code = 0, reason = 'e2'})
> - * e1:set_prev(e2) -- Now e2 is the cause of e1 and e1 is
> - * the effect of e2.
> - * Only cause keeps reference to avoid cyclic dependence.
> - * 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.
> + * e1:set_prev(e2) -- Now e2 is the cause of e1.
> */
> struct error *cause;
> - struct error *effect;
> + /**
> + * Flag whether this error is not top of the error stack.
> + * I.e. it has an 'effect'. Effect is e1 in the example
> + * above. The flag is used to prevent changing middle of
> + * the stack (except when it is cut off - this easy to
> + * support, and can't create a cycle).
> + */
> + bool has_effect;
> };
>
> static inline void
> @@ -115,51 +113,24 @@ 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;
> - if (to_delete->effect != NULL)
> - to_delete->effect->cause = to_delete->cause;
> - if (to_delete->cause != NULL)
> - to_delete->cause->effect = to_delete->effect;
> to_delete->cause = NULL;
> - to_delete->effect = NULL;
> to_delete->destroy(to_delete);
> if (cause == NULL)
> return;
> + cause->has_effect = false;
> to_delete = cause;
> }
> }
>
> /**
> - * Unlink error from its effect. For instance:
> - * e1 -> e2 -> e3 -> e4 (e1:set_prev(e2); e2:set_prev(e3) ...)
> - * unlink(e3): e1 -> e2 -> NULL; e3 -> e4 -> NULL
> - */
> -static inline void
> -error_unlink_effect(struct error *e)
> -{
> - if (e->effect != NULL) {
> - assert(e->refs > 1);
> - error_unref(e);
> - e->effect->cause = NULL;
> - }
> - e->effect = NULL;
> -}
> -
> -/**
> - * 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.
> + * Set previous error. It can be NULL to make @a not having any
> + * previous error. In case @a prev is not NULL, it should not be
> + * already belong to another stack, and @a e should be top of the
> + * stack.
> *
> - * @retval -1 in case adding @a effect results in list cycles;
> - * 0 otherwise.
> + * @retval -1 In case adding @a prev is not possible.
> + * @retval 0 Success.
> */
> int
> error_set_prev(struct error *e, struct error *prev);
> @@ -241,7 +212,6 @@ diag_set_error(struct diag *diag, struct error *e)
> assert(e != NULL);
> error_ref(e);
> diag_clear(diag);
> - error_unlink_effect(e);
> diag->last = e;
> }
>
> @@ -255,11 +225,11 @@ static inline void
> diag_add_error(struct diag *diag, struct error *e)
> {
> assert(e != NULL);
> + assert(e->cause == NULL);
> error_ref(e);
> - error_unlink_effect(e);
> e->cause = diag->last;
> if (diag->last != NULL)
> - diag->last->effect = e;
> + diag->last->has_effect = true;
> diag->last = e;
> }
>
> diff --git a/src/lua/error.lua b/src/lua/error.lua
> index bdc9c714d..3d0b75689 100644
> --- a/src/lua/error.lua
> +++ b/src/lua/error.lua
> @@ -25,7 +25,7 @@ struct error {
> /* Error description. */
> char _errmsg[DIAG_ERRMSG_MAX];
> struct error *_cause;
> - struct error *_effect;
> + bool has_effect;
> };
>
> char *
> diff --git a/test/box/error.result b/test/box/error.result
> index 4f0f30491..300def647 100644
> --- a/test/box/error.result
> +++ b/test/box/error.result
> @@ -502,6 +502,10 @@ box.error()
> | ---
> | ...
>
> +space:drop()
> + | ---
> + | ...
> +
> -- gh-1148: errors can be arranged into list (so called
> -- stacked diagnostics).
> --
> @@ -540,14 +544,17 @@ assert(e2.prev == nil)
> | ...
> -- At this point stack is following: e1 -> e2
> -- Let's test following cases:
> --- 1. e3 -> e2, e1 -> NULL (e3:set_prev(e2))
> +-- 1. e3 -> e2, e1 -> NULL (e1:set_prev(nil), e3:set_prev(e2))
> -- 2. e1 -> e3, e2 -> NULL (e1:set_prev(e3))
> -- 3. e3 -> e1 -> e2 (e3:set_prev(e1))
> --- 4. e1 -> e2 -> e3 (e2:set_prev(e3))
> +-- 4. e1 -> e2 -> e3 (e1:set_prev(nil) e2:set_prev(e3) e1:set_prev(e2))
> --
> e3 = box.error.new({code = 111, reason = "another cause"})
> | ---
> | ...
> +e1:set_prev(nil)
> + | ---
> + | ...
> e3:set_prev(e2)
> | ---
> | ...
> @@ -566,6 +573,9 @@ assert(e1.prev == nil)
>
> -- Reset stack to e1 -> e2 and test case 2.
> --
> +e3:set_prev(nil)
> + | ---
> + | ...
> e1:set_prev(e2)
> | ---
> | ...
> @@ -628,19 +638,19 @@ assert(e3.prev == e1)
>
> -- Unlink errors and test case 4.
> --
> -e1:set_prev(nil)
> +e3:set_prev(nil)
> | ---
> | ...
> e2:set_prev(nil)
> | ---
> | ...
> -e3:set_prev(nil)
> +e1:set_prev(nil)
> | ---
> | ...
> -e1:set_prev(e2)
> +e2:set_prev(e3)
> | ---
> | ...
> -e2:set_prev(e3)
> +e1:set_prev(e2)
> | ---
> | ...
> assert(e1.prev == e2)
> @@ -676,72 +686,6 @@ assert(e3.prev == nil)
> | - true
> | ...
>
> --- Test splitting list into two ones.
> --- After that we will get two lists: e1->e2->e5 and e3->e4
> ---
> -e4 = box.error.new({code = 111, reason = "yet another cause"})
> - | ---
> - | ...
> -e5 = box.error.new({code = 111, reason = "and another one"})
> - | ---
> - | ...
> -e3:set_prev(e4)
> - | ---
> - | ...
> -e2:set_prev(e5)
> - | ---
> - | ...
> -assert(e1.prev == e2)
> - | ---
> - | - true
> - | ...
> -assert(e2.prev == e5)
> - | ---
> - | - true
> - | ...
> -assert(e3.prev == e4)
> - | ---
> - | - true
> - | ...
> -assert(e5.prev == nil)
> - | ---
> - | - true
> - | ...
> -assert(e4.prev == nil)
> - | ---
> - | - true
> - | ...
> -
> --- Another splitting option: e1->e2 and e5->e3->e4
> --- But firstly restore to one single list e1->e2->e3->e4
> ---
> -e2:set_prev(e3)
> - | ---
> - | ...
> -e5:set_prev(e3)
> - | ---
> - | ...
> -assert(e1.prev == e2)
> - | ---
> - | - true
> - | ...
> -assert(e2.prev == nil)
> - | ---
> - | - true
> - | ...
> -assert(e5.prev == e3)
> - | ---
> - | - true
> - | ...
> -assert(e3.prev == e4)
> - | ---
> - | - true
> - | ...
> -assert(e4.prev == nil)
> - | ---
> - | - true
> - | ...
> -
> -- 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.
> @@ -785,7 +729,3 @@ assert(e1.prev == e2)
> | ---
> | - true
> | ...
> -
> -space:drop()
> - | ---
> - | ...
> diff --git a/test/box/error.test.lua b/test/box/error.test.lua
> index 66a22db90..04e00168e 100644
> --- a/test/box/error.test.lua
> +++ b/test/box/error.test.lua
> @@ -108,6 +108,8 @@ box.error.new(err)
> box.error.clear()
> box.error()
>
> +space:drop()
> +
> -- gh-1148: errors can be arranged into list (so called
> -- stacked diagnostics).
> --
> @@ -122,12 +124,13 @@ e2:set_prev(e1)
> assert(e2.prev == nil)
> -- At this point stack is following: e1 -> e2
> -- Let's test following cases:
> --- 1. e3 -> e2, e1 -> NULL (e3:set_prev(e2))
> +-- 1. e3 -> e2, e1 -> NULL (e1:set_prev(nil), e3:set_prev(e2))
> -- 2. e1 -> e3, e2 -> NULL (e1:set_prev(e3))
> -- 3. e3 -> e1 -> e2 (e3:set_prev(e1))
> --- 4. e1 -> e2 -> e3 (e2:set_prev(e3))
> +-- 4. e1 -> e2 -> e3 (e1:set_prev(nil) e2:set_prev(e3) e1:set_prev(e2))
> --
> e3 = box.error.new({code = 111, reason = "another cause"})
> +e1:set_prev(nil)
> e3:set_prev(e2)
> assert(e3.prev == e2)
> assert(e2.prev == nil)
> @@ -135,6 +138,7 @@ assert(e1.prev == nil)
>
> -- Reset stack to e1 -> e2 and test case 2.
> --
> +e3:set_prev(nil)
> e1:set_prev(e2)
> assert(e2.prev == nil)
> assert(e3.prev == nil)
> @@ -156,11 +160,11 @@ assert(e3.prev == e1)
>
> -- Unlink errors and test case 4.
> --
> -e1:set_prev(nil)
> -e2:set_prev(nil)
> e3:set_prev(nil)
> -e1:set_prev(e2)
> +e2:set_prev(nil)
> +e1:set_prev(nil)
> e2:set_prev(e3)
> +e1:set_prev(e2)
> assert(e1.prev == e2)
> assert(e2.prev == e3)
> assert(e3.prev == nil)
> @@ -173,30 +177,6 @@ assert(e3.prev == nil)
> e3:set_prev(e2)
> assert(e3.prev == nil)
>
> --- Test splitting list into two ones.
> --- After that we will get two lists: e1->e2->e5 and e3->e4
> ---
> -e4 = box.error.new({code = 111, reason = "yet another cause"})
> -e5 = box.error.new({code = 111, reason = "and another one"})
> -e3:set_prev(e4)
> -e2:set_prev(e5)
> -assert(e1.prev == e2)
> -assert(e2.prev == e5)
> -assert(e3.prev == e4)
> -assert(e5.prev == nil)
> -assert(e4.prev == nil)
> -
> --- Another splitting option: e1->e2 and e5->e3->e4
> --- But firstly restore to one single list e1->e2->e3->e4
> ---
> -e2:set_prev(e3)
> -e5:set_prev(e3)
> -assert(e1.prev == e2)
> -assert(e2.prev == nil)
> -assert(e5.prev == e3)
> -assert(e3.prev == e4)
> -assert(e4.prev == nil)
> -
> -- 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.
> @@ -212,5 +192,3 @@ assert(e2.code == 111)
> box.error.set(e1)
> box.error.clear()
> assert(e1.prev == e2)
> -
> -space:drop()
> diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
> index 3a817a659..276bf2362 100644
> --- a/src/lib/core/diag.h
> +++ b/src/lib/core/diag.h
> @@ -117,12 +117,11 @@ error_unref(struct error *e)
> while (--to_delete->refs == 0) {
> /* Unlink error from lists completely.*/
> struct error *cause = to_delete->cause;
> - if (to_delete->effect != NULL)
> - to_delete->effect->cause = to_delete->cause;
> - if (to_delete->cause != NULL)
> - to_delete->cause->effect = to_delete->effect;
> - to_delete->cause = NULL;
> - to_delete->effect = NULL;
> + 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;
> commit eaada025e478aeb66d0b8f8f31c91045b0f0089f
> Author: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> Date: Thu Apr 2 00:52:54 2020 +0200
>
> Diag stack simplification
>
> diff --git a/src/lib/core/diag.c b/src/lib/core/diag.c
> index ed8beb58d..69a19d91f 100644
> --- a/src/lib/core/diag.c
> +++ b/src/lib/core/diag.c
> @@ -37,37 +37,32 @@ 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)
> + if (e == prev)
> + return -1;
> + if (prev != NULL) {
> + /*
> + * It is not allowed to change middle of the error
> + * stack. Except when the tail is cut. Not
> + * replaced. Reason is to make the code simpler,
> + * and avoid any necessity to detect cycles. In
> + * that implementation cycles are not allowed, and
> + * this guarantee costs nothing.
> + */
> + if (prev->has_effect || e->has_effect)
> return -1;
> - tmp = tmp->cause;
> + prev->has_effect = true;
> + error_ref(prev);
> }
> /*
> * At once error can feature only one reason.
> * So unlink previous 'cause' node.
> */
> if (e->cause != NULL) {
> - e->cause->effect = NULL;
> + e->cause->has_effect = false;
> 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_ref(prev);
> - error_unlink_effect(prev);
> - prev->effect = e;
> - }
> return 0;
> }
>
> @@ -91,7 +86,7 @@ error_create(struct error *e,
> }
> e->errmsg[0] = '\0';
> e->cause = NULL;
> - e->effect = NULL;
> + e->has_effect = false;
> }
>
> struct diag *
> diff --git a/src/lib/core/diag.h b/src/lib/core/diag.h
> index 3a817a659..52987befa 100644
> --- a/src/lib/core/diag.h
> +++ b/src/lib/core/diag.h
> @@ -85,22 +85,20 @@ struct error {
> /* Error description. */
> char errmsg[DIAG_ERRMSG_MAX];
> /**
> - * Link to the cause and effect of given error. The cause
> - * creates the effect:
> + * Cause of the given error..
> * e1 = box.error.new({code = 0, reason = 'e1'})
> * e2 = box.error.new({code = 0, reason = 'e2'})
> - * e1:set_prev(e2) -- Now e2 is the cause of e1 and e1 is
> - * the effect of e2.
> - * Only cause keeps reference to avoid cyclic dependence.
> - * 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.
> + * e1:set_prev(e2) -- Now e2 is the cause of e1.
> */
> struct error *cause;
> - struct error *effect;
> + /**
> + * Flag whether this error is not top of the error stack.
> + * I.e. it has an 'effect'. Effect is e1 in the example
> + * above. The flag is used to prevent changing middle of
> + * the stack (except when it is cut off - this easy to
> + * support, and can't create a cycle).
> + */
> + bool has_effect;
> };
>
> static inline void
> @@ -115,51 +113,24 @@ 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;
> - if (to_delete->effect != NULL)
> - to_delete->effect->cause = to_delete->cause;
> - if (to_delete->cause != NULL)
> - to_delete->cause->effect = to_delete->effect;
> to_delete->cause = NULL;
> - to_delete->effect = NULL;
> to_delete->destroy(to_delete);
> if (cause == NULL)
> return;
> + cause->has_effect = false;
> to_delete = cause;
> }
> }
>
> /**
> - * Unlink error from its effect. For instance:
> - * e1 -> e2 -> e3 -> e4 (e1:set_prev(e2); e2:set_prev(e3) ...)
> - * unlink(e3): e1 -> e2 -> NULL; e3 -> e4 -> NULL
> - */
> -static inline void
> -error_unlink_effect(struct error *e)
> -{
> - if (e->effect != NULL) {
> - assert(e->refs > 1);
> - error_unref(e);
> - e->effect->cause = NULL;
> - }
> - e->effect = NULL;
> -}
> -
> -/**
> - * 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.
> + * Set previous error. It can be NULL to make @a not having any
> + * previous error. In case @a prev is not NULL, it should not be
> + * already belong to another stack, and @a e should be top of the
> + * stack.
> *
> - * @retval -1 in case adding @a effect results in list cycles;
> - * 0 otherwise.
> + * @retval -1 In case adding @a prev is not possible.
> + * @retval 0 Success.
> */
> int
> error_set_prev(struct error *e, struct error *prev);
> @@ -241,7 +212,6 @@ diag_set_error(struct diag *diag, struct error *e)
> assert(e != NULL);
> error_ref(e);
> diag_clear(diag);
> - error_unlink_effect(e);
> diag->last = e;
> }
>
> @@ -255,11 +225,11 @@ static inline void
> diag_add_error(struct diag *diag, struct error *e)
> {
> assert(e != NULL);
> + assert(e->cause == NULL);
> error_ref(e);
> - error_unlink_effect(e);
> e->cause = diag->last;
> if (diag->last != NULL)
> - diag->last->effect = e;
> + diag->last->has_effect = true;
> diag->last = e;
> }
>
> diff --git a/src/lua/error.lua b/src/lua/error.lua
> index bdc9c714d..3d0b75689 100644
> --- a/src/lua/error.lua
> +++ b/src/lua/error.lua
> @@ -25,7 +25,7 @@ struct error {
> /* Error description. */
> char _errmsg[DIAG_ERRMSG_MAX];
> struct error *_cause;
> - struct error *_effect;
> + bool has_effect;
> };
>
> char *
> diff --git a/test/box/error.result b/test/box/error.result
> index 4f0f30491..300def647 100644
> --- a/test/box/error.result
> +++ b/test/box/error.result
> @@ -502,6 +502,10 @@ box.error()
> | ---
> | ...
>
> +space:drop()
> + | ---
> + | ...
> +
> -- gh-1148: errors can be arranged into list (so called
> -- stacked diagnostics).
> --
> @@ -540,14 +544,17 @@ assert(e2.prev == nil)
> | ...
> -- At this point stack is following: e1 -> e2
> -- Let's test following cases:
> --- 1. e3 -> e2, e1 -> NULL (e3:set_prev(e2))
> +-- 1. e3 -> e2, e1 -> NULL (e1:set_prev(nil), e3:set_prev(e2))
> -- 2. e1 -> e3, e2 -> NULL (e1:set_prev(e3))
> -- 3. e3 -> e1 -> e2 (e3:set_prev(e1))
> --- 4. e1 -> e2 -> e3 (e2:set_prev(e3))
> +-- 4. e1 -> e2 -> e3 (e1:set_prev(nil) e2:set_prev(e3) e1:set_prev(e2))
> --
> e3 = box.error.new({code = 111, reason = "another cause"})
> | ---
> | ...
> +e1:set_prev(nil)
> + | ---
> + | ...
> e3:set_prev(e2)
> | ---
> | ...
> @@ -566,6 +573,9 @@ assert(e1.prev == nil)
>
> -- Reset stack to e1 -> e2 and test case 2.
> --
> +e3:set_prev(nil)
> + | ---
> + | ...
> e1:set_prev(e2)
> | ---
> | ...
> @@ -628,19 +638,19 @@ assert(e3.prev == e1)
>
> -- Unlink errors and test case 4.
> --
> -e1:set_prev(nil)
> +e3:set_prev(nil)
> | ---
> | ...
> e2:set_prev(nil)
> | ---
> | ...
> -e3:set_prev(nil)
> +e1:set_prev(nil)
> | ---
> | ...
> -e1:set_prev(e2)
> +e2:set_prev(e3)
> | ---
> | ...
> -e2:set_prev(e3)
> +e1:set_prev(e2)
> | ---
> | ...
> assert(e1.prev == e2)
> @@ -676,72 +686,6 @@ assert(e3.prev == nil)
> | - true
> | ...
>
> --- Test splitting list into two ones.
> --- After that we will get two lists: e1->e2->e5 and e3->e4
> ---
> -e4 = box.error.new({code = 111, reason = "yet another cause"})
> - | ---
> - | ...
> -e5 = box.error.new({code = 111, reason = "and another one"})
> - | ---
> - | ...
> -e3:set_prev(e4)
> - | ---
> - | ...
> -e2:set_prev(e5)
> - | ---
> - | ...
> -assert(e1.prev == e2)
> - | ---
> - | - true
> - | ...
> -assert(e2.prev == e5)
> - | ---
> - | - true
> - | ...
> -assert(e3.prev == e4)
> - | ---
> - | - true
> - | ...
> -assert(e5.prev == nil)
> - | ---
> - | - true
> - | ...
> -assert(e4.prev == nil)
> - | ---
> - | - true
> - | ...
> -
> --- Another splitting option: e1->e2 and e5->e3->e4
> --- But firstly restore to one single list e1->e2->e3->e4
> ---
> -e2:set_prev(e3)
> - | ---
> - | ...
> -e5:set_prev(e3)
> - | ---
> - | ...
> -assert(e1.prev == e2)
> - | ---
> - | - true
> - | ...
> -assert(e2.prev == nil)
> - | ---
> - | - true
> - | ...
> -assert(e5.prev == e3)
> - | ---
> - | - true
> - | ...
> -assert(e3.prev == e4)
> - | ---
> - | - true
> - | ...
> -assert(e4.prev == nil)
> - | ---
> - | - true
> - | ...
> -
> -- 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.
> @@ -785,7 +729,3 @@ assert(e1.prev == e2)
> | ---
> | - true
> | ...
> -
> -space:drop()
> - | ---
> - | ...
> diff --git a/test/box/error.test.lua b/test/box/error.test.lua
> index 66a22db90..04e00168e 100644
> --- a/test/box/error.test.lua
> +++ b/test/box/error.test.lua
> @@ -108,6 +108,8 @@ box.error.new(err)
> box.error.clear()
> box.error()
>
> +space:drop()
> +
> -- gh-1148: errors can be arranged into list (so called
> -- stacked diagnostics).
> --
> @@ -122,12 +124,13 @@ e2:set_prev(e1)
> assert(e2.prev == nil)
> -- At this point stack is following: e1 -> e2
> -- Let's test following cases:
> --- 1. e3 -> e2, e1 -> NULL (e3:set_prev(e2))
> +-- 1. e3 -> e2, e1 -> NULL (e1:set_prev(nil), e3:set_prev(e2))
> -- 2. e1 -> e3, e2 -> NULL (e1:set_prev(e3))
> -- 3. e3 -> e1 -> e2 (e3:set_prev(e1))
> --- 4. e1 -> e2 -> e3 (e2:set_prev(e3))
> +-- 4. e1 -> e2 -> e3 (e1:set_prev(nil) e2:set_prev(e3) e1:set_prev(e2))
> --
> e3 = box.error.new({code = 111, reason = "another cause"})
> +e1:set_prev(nil)
> e3:set_prev(e2)
> assert(e3.prev == e2)
> assert(e2.prev == nil)
> @@ -135,6 +138,7 @@ assert(e1.prev == nil)
>
> -- Reset stack to e1 -> e2 and test case 2.
> --
> +e3:set_prev(nil)
> e1:set_prev(e2)
> assert(e2.prev == nil)
> assert(e3.prev == nil)
> @@ -156,11 +160,11 @@ assert(e3.prev == e1)
>
> -- Unlink errors and test case 4.
> --
> -e1:set_prev(nil)
> -e2:set_prev(nil)
> e3:set_prev(nil)
> -e1:set_prev(e2)
> +e2:set_prev(nil)
> +e1:set_prev(nil)
> e2:set_prev(e3)
> +e1:set_prev(e2)
> assert(e1.prev == e2)
> assert(e2.prev == e3)
> assert(e3.prev == nil)
> @@ -173,30 +177,6 @@ assert(e3.prev == nil)
> e3:set_prev(e2)
> assert(e3.prev == nil)
>
> --- Test splitting list into two ones.
> --- After that we will get two lists: e1->e2->e5 and e3->e4
> ---
> -e4 = box.error.new({code = 111, reason = "yet another cause"})
> -e5 = box.error.new({code = 111, reason = "and another one"})
> -e3:set_prev(e4)
> -e2:set_prev(e5)
> -assert(e1.prev == e2)
> -assert(e2.prev == e5)
> -assert(e3.prev == e4)
> -assert(e5.prev == nil)
> -assert(e4.prev == nil)
> -
> --- Another splitting option: e1->e2 and e5->e3->e4
> --- But firstly restore to one single list e1->e2->e3->e4
> ---
> -e2:set_prev(e3)
> -e5:set_prev(e3)
> -assert(e1.prev == e2)
> -assert(e2.prev == nil)
> -assert(e5.prev == e3)
> -assert(e3.prev == e4)
> -assert(e4.prev == nil)
> -
> -- 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.
> @@ -212,5 +192,3 @@ assert(e2.code == 111)
> box.error.set(e1)
> box.error.clear()
> assert(e1.prev == e2)
> -
> -space:drop()
More information about the Tarantool-patches
mailing list