commit eaada025e478aeb66d0b8f8f31c91045b0f0089f Author: Vladislav Shpilevoy 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()