Re: [Tarantool-patches] [PATCH 4/7] box: introduce stacked diagnostic area

[Vladislav Shpilevoy <v.shpilevoy@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 <stdbool.h>
>  #include <assert.h>
>  #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.