[tarantool-patches] Re: [PATCH v2 3/3] box: introduce stacked diagnostic area
Konstantin Osipov
kostja at tarantool.org
Tue Aug 27 01:34:23 MSK 2019
* Kirill Shcherbatov <kshcherbatov at tarantool.org> [19/08/23 13:03]:
> static inline void
> @@ -90,9 +92,11 @@ static inline void
> error_unref(struct error *e)
> {
> assert(e->refs > 0);
> - --e->refs;
> - if (e->refs == 0)
> + if (--e->refs == 0) {
> + if (e->reason != NULL)
> + error_unref(e->reason);
> e->destroy(e);
> + }
This looks fragile, can you rewrite this place without recursion?
> + * Remove all errors set in a given diagnostics area after a
> + * given savepoint.
> + *
> + * Operation removes reason for the error
> + * preceding the savepoint and releases a diagnostic area's
> + * reference on the most recent error (diag::last for the
> + * rollback beginning). This means that if user code have a
> + * pointer and have a reference to an error object from the
> + * rollback zone, this pointer and the following "reason" error
> + * objects are a valid error list.
> + */
BTW, did you check if anyone likes the name 'reason'? I'm not very
fond of it, previous error is not always the reason of the current
one. I would simply call it 'prev' or 'next'.
> + if (diag->last != begin) {
> + assert(prev != NULL && prev->reason == svp);
> + prev->reason = NULL;
> + error_unref(begin);
OK, this works because you terminate the list with NULL first...
> + prev->reason = svp;
> + error_ref(svp);
> + }
> +}
> +
> +++ 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;
> }
Nice. I bet there is a bunch of places like that, forgotten by
now.
With these two comments, LGTM.
--
Konstantin Osipov, Moscow, Russia
More information about the Tarantool-patches
mailing list