[tarantool-patches] Re: [PATCH v7 2/7] box: introduce OPT_ARRAY opt_type to decode arrays

[Konstantin Osipov <kostja@tarantool.org>]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [18/05/23 20:33]:
> As we need to store Checks array in opt_def MsgPack in future
> introduced special type and decode callback to_array used in opt_set
> function.
> 
> Part of #3272.


> ---
>  src/box/opt_def.c |  9 +++++++++
>  src/box/opt_def.h | 19 ++++++++++++++-----
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/opt_def.c b/src/box/opt_def.c
> index cd93c23..c8440c9 100644
> --- a/src/box/opt_def.c
> +++ b/src/box/opt_def.c
> @@ -44,6 +44,7 @@ const char *opt_type_strs[] = {
>  	/* [OPT_STR]	= */ "string",
>  	/* [OPT_STRPTR] = */ "string",
>  	/* [OPT_ENUM]   = */ "enum",
> +	/* [OPT_ARRAY]  = */ "array",
>  };
>  
>  static int
> @@ -135,6 +136,14 @@ opt_set(void *opts, const struct opt_def *def, const char **val,
>  			unreachable();
>  		};
>  		break;
> +	case OPT_ARRAY:
> +		if (mp_typeof(**val) != MP_ARRAY)
> +			return -1;
> +		ival = mp_decode_array(val);
> +		assert(def->to_array != NULL);
> +		if (def->to_array(val, ival, opt) != 0)
> +			return -1;
> +		break;
>  	default:
>  		unreachable();
>  	}
> diff --git a/src/box/opt_def.h b/src/box/opt_def.h
> index 633832a..71fef3a 100644
> --- a/src/box/opt_def.h
> +++ b/src/box/opt_def.h
> @@ -47,12 +47,14 @@ enum opt_type {
>  	OPT_STR,	/* char[] */
>  	OPT_STRPTR,	/* char*  */
>  	OPT_ENUM,	/* enum */
> +	OPT_ARRAY,	/* array */
>  	opt_type_MAX,
>  };
>  
>  extern const char *opt_type_strs[];
>  
>  typedef int64_t (*opt_def_to_enum_cb)(const char *str, uint32_t len);
> +typedef int (*opt_def_to_array_cb)(const char **str, uint32_t len, char *opt);

While the previous callback is self-explanatory, this one requires
a comment. Actually a comment for both won't hurt. What should the
callback return? who does own the memory used by the return value? 

What is the error handling convention?

>  struct opt_def {
>  	const char *name;
> @@ -64,19 +66,26 @@ struct opt_def {
>  	int enum_size;
>  	const char **enum_strs;
>  	uint32_t enum_max;
> -	/** If not NULL, used to get a enum value by a string. */
> -	opt_def_to_enum_cb to_enum;
> +	/** If not NULL, used to get a value by a string. */

The comment became worse, not better.

> +	union {
> +		opt_def_to_enum_cb to_enum;
> +		opt_def_to_array_cb to_array;
> +	};
>  };

Otherwise LGTM.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov