Re: [Tarantool-patches] [PATCH 4/5] error: provide MP_ERROR extension serializer

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Tue, May 12, 2020 at 01:45:51AM +0200, Vladislav Shpilevoy wrote:
> Msgpuck functions mp_snprint() and mp_fprint() now support
> customizable MP_EXT serializer. This patch adds such for MP_ERROR.
> All extension serializers will be activated in a separate commit.
> 
> Part of #4719
> ---
>  src/box/mp_error.cc       | 161 ++++++++++++++++++++++-
>  src/box/mp_error.h        |  29 ++++
>  test/unit/mp_error.cc     | 270 +++++++++++++++++++++++++++++++++++++-
>  test/unit/mp_error.result |  72 +++++++++-
>  4 files changed, 529 insertions(+), 3 deletions(-)
> 
> diff --git a/src/box/mp_error.cc b/src/box/mp_error.cc
> index 0491a7a50..fed2ce288 100644
> --- a/src/box/mp_error.cc
> +++ b/src/box/mp_error.cc
> @@ -28,6 +28,8 @@
>   * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>   * SUCH DAMAGE.
>   */
> +#ifndef MP_ERROR_PRINT_DEFINITION
> +
>  #include "box/mp_error.h"
>  #include "box/error.h"
>  #include "mpstream/mpstream.h"
> @@ -86,7 +88,8 @@ enum {
>  	 * Type-specific fields stored as a map
>  	 * {string key = value}.
>  	 */
> -	MP_ERROR_FIELDS = 0x06
> +	MP_ERROR_FIELDS = 0x06,
> +	MP_ERROR_MAX,
>  };
>  
>  /**
> @@ -552,3 +555,159 @@ error_unpack_unsafe(const char **data)
>  	}
>  	return err;
>  }
> +
> +#define MP_ERROR_PRINT_DEFINITION
> +#define MP_PRINT_FUNC snprintf
> +#define MP_PRINT_SUFFIX snprint
> +#define MP_PRINT_2(total, func, ...)						\
> +	SNPRINT(total, func, buf, size, __VA_ARGS__)
> +#define MP_PRINT_ARGS_DECL char *buf, int size
> +#include __FILE__

Don't get it -- include current file?! This a good sign of
some kind of problem in the code structure... If we need
a template then it should be some .cc/.c file explicily
included instead of __FILE__, no?

> +#define MP_ERROR_PRINT_DEFINITION
> +#define MP_PRINT_FUNC fprintf
> +#define MP_PRINT_SUFFIX fprint
> +#define MP_PRINT_2(total, func, ...) do {							\
> +	int bytes = func(file, __VA_ARGS__);					\
> +	if (bytes < 0)								\
> +		return -1;							\
> +	total += bytes;								\
> +} while (0)
> +#define MP_PRINT_ARGS_DECL FILE *file
> +#include __FILE__
> +
> +/* !defined(MP_ERROR_PRINT_DEFINITION) */
> +#else
> +/* defined(MP_ERROR_PRINT_DEFINITION) */
> +
> +/**
> + * MP_ERROR extension string serializer.
> + * There are two applications for string serialization - into a
> + * buffer, and into a file. Structure of both is exactly the same
> + * except for the copying/writing itself. To avoid code
> + * duplication the code is templated and expects some macros to do
> + * the actual output.
> + */
> +
> +#define MP_CONCAT4_R(a, b, c, d) a##b##c##d
> +#define MP_CONCAT4(a, b, c, d) MP_CONCAT4_R(a, b, c, d)
> +#define MP_PRINT(total, ...) MP_PRINT_2(total, MP_PRINT_FUNC, __VA_ARGS__)
> +
> +#define mp_func_name(name) MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name)
> +#define mp_print_error_one mp_func_name(error_one)
> +#define mp_print_error_stack mp_func_name(error_stack)
> +#define mp_print_error mp_func_name(error)
> +#define mp_print_common mp_func_name(recursion)

Maybe we should align the assignments to be able to read it, like

#define MP_CONCAT4_R(a, b, c, d)	a##b##c##d
#define MP_CONCAT4(a, b, c, d)		MP_CONCAT4_R(a, b, c, d)
#define MP_PRINT(total, ...)		MP_PRINT_2(total, MP_PRINT_FUNC, __VA_ARGS__)

#define mp_func_name(name)	MP_CONCAT4(mp_, MP_PRINT_SUFFIX, _, name)
#define mp_print_error_one	mp_func_name(error_one)
#define mp_print_error_stack	mp_func_name(error_stack)
#define mp_print_error		mp_func_name(error)
#define mp_print_common		mp_func_name(recursion)

> +
> +static int
> +mp_print_error_one(MP_PRINT_ARGS_DECL, const char **data, int depth)
> +{
> +	int total = 0;
> +	MP_PRINT(total, "{");
> +	if (depth <= 0) {
> +		MP_PRINT(total, "...}");
> +		return total;
> +	}
> +	const char *field_to_key[MP_ERROR_MAX] = {
> +		/* MP_ERROR_TYPE = */ "\"type\": ",
> +		/* MP_ERROR_FILE = */ "\"file\": ",
> +		/* MP_ERROR_LINE = */ "\"line\": ",
> +		/* MP_ERROR_MESSAGE = */ "\"message\": ",
> +		/* MP_ERROR_ERRNO = */ "\"errno\": ",
> +		/* MP_ERROR_CODE = */ "\"code\": ",
> +		/* MP_ERROR_FIELDS = */ "\"fields\": ",
> +	};

Please use designited initializers in a readable way
instead of this bloody mess

	const char *field_to_key[] = {
		[MP_ERROR_TYPE]		= "\"type\": ",
		...
		[MP_ERROR_FILE]		= ...,
		...
		[MP_ERROR_FIELDS]	= "\"fields\": ",
	}