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

[Kirill Shcherbatov]

> 1. If to_array is failed, the error is reset in opts_parse_key:
> 
> snprintf(errmsg, sizeof(errmsg), "'%.*s' must be %s",
> 	 key_len, key, opt_type_strs[def->type]);
> diag_set(ClientError, errcode, field_no, errmsg);
> 
> So the original error is lost. You should refactor opt_set
> so that it sets diag on type mismatch instead of doing it in
> opts_parse_key. And opts_parse_key does nothing on error in opt_set -
> just return -1.

diff --git a/src/box/opt_def.c b/src/box/opt_def.c
index c8440c9..f4dd05b 100644
--- a/src/box/opt_def.c
+++ b/src/box/opt_def.c
@@ -49,10 +49,11 @@ const char *opt_type_strs[] = {
 
 static int
 opt_set(void *opts, const struct opt_def *def, const char **val,
-	struct region *region)
+	struct region *region, uint32_t errcode, uint32_t field_no)
 {
 	int64_t ival;
 	uint64_t uval;
+	char errmsg[DIAG_ERRMSG_MAX];
 	double dval;
 	uint32_t str_len;
 	const char *str;
@@ -61,30 +62,30 @@ opt_set(void *opts, const struct opt_def *def, const char **val,
 	switch (def->type) {
 	case OPT_BOOL:
 		if (mp_typeof(**val) != MP_BOOL)
-			return -1;
+			goto type_mismatch_err;
 		store_bool(opt, mp_decode_bool(val));
 		break;
 	case OPT_UINT32:
 		if (mp_typeof(**val) != MP_UINT)
-			return -1;
+			goto type_mismatch_err;
 		uval = mp_decode_uint(val);
 		if (uval > UINT32_MAX)
-			return -1;
+			goto type_mismatch_err;
 		store_u32(opt, uval);
 		break;
 	case OPT_INT64:
 		if (mp_read_int64(val, &ival) != 0)
-			return -1;
+			goto type_mismatch_err;
 		store_u64(opt, ival);
 		break;
 	case OPT_FLOAT:
 		if (mp_read_double(val, &dval) != 0)
-			return -1;
+			goto type_mismatch_err;
 		store_double(opt, dval);
 		break;
 	case OPT_STR:
 		if (mp_typeof(**val) != MP_STR)
-			return -1;
+			goto type_mismatch_err;
 		str = mp_decode_str(val, &str_len);
 		str_len = MIN(str_len, def->len - 1);
 		memcpy(opt, str, str_len);
@@ -92,7 +93,7 @@ opt_set(void *opts, const struct opt_def *def, const char **val,
 		break;
 	case OPT_STRPTR:
 		if (mp_typeof(**val) != MP_STR)
-			return -1;
+			goto type_mismatch_err;
 		str = mp_decode_str(val, &str_len);
 		if (str_len > 0) {
 			ptr = (char *) region_alloc(region, str_len + 1);
@@ -111,7 +112,7 @@ opt_set(void *opts, const struct opt_def *def, const char **val,
 		break;
 	case OPT_ENUM:
 		if (mp_typeof(**val) != MP_STR)
-			return -1;
+			goto type_mismatch_err;
 		str = mp_decode_str(val, &str_len);
 		if (def->to_enum == NULL) {
 			ival = strnindex(def->enum_strs, str, str_len,
@@ -138,7 +139,7 @@ opt_set(void *opts, const struct opt_def *def, const char **val,
 		break;
 	case OPT_ARRAY:
 		if (mp_typeof(**val) != MP_ARRAY)
-			return -1;
+			goto type_mismatch_err;
 		ival = mp_decode_array(val);
 		assert(def->to_array != NULL);
 		if (def->to_array(val, ival, opt) != 0)
@@ -148,6 +149,12 @@ opt_set(void *opts, const struct opt_def *def, const char **val,
 		unreachable();
 	}
 	return 0;
+
+type_mismatch_err:
+	snprintf(errmsg, sizeof(errmsg), "'%s' must be %s",
+		 def->name, opt_type_strs[def->type]);
+	diag_set(ClientError, errcode, field_no, errmsg);
+	return -1;
 }
 
 int
@@ -163,12 +170,8 @@ opts_parse_key(void *opts, const struct opt_def *reg, const char *key,
 		    memcmp(key, def->name, key_len) != 0)
 			continue;
 
-		if (opt_set(opts, def, data, region) != 0) {
-			snprintf(errmsg, sizeof(errmsg), "'%.*s' must be %s",
-				 key_len, key, opt_type_strs[def->type]);
-			diag_set(ClientError, errcode, field_no, errmsg);
+		if (opt_set(opts, def, data, region, errcode, field_no) != 0)
 			return -1;
-		}
 		found = true;
 		break;
 	}

And some other changes

+/**
+ * Decode MsgPack array callback.
+ * All memory allocations returned by opt_def_to_array_cb with opt
+ * [out] argument should be managed manually.
+ * @param str encoded data pointer (next to MsgPack ARRAY header).
+ * @param len array length (items count).
+ * @param [out] opt pointer to store resulting value.
+ * @param errcode Code of error to set if something is wrong.
+ * @param field_no Field number of an option in a parent element.
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
+typedef int (*opt_def_to_array_cb)(const char **str, uint32_t len, char *opt,
+                                  uint32_t errcode, uint32_t field_no);


+       case OPT_ARRAY:
+               if (mp_typeof(**val) != MP_ARRAY)
+                       goto type_mismatch_err;
+               ival = mp_decode_array(val);
+               assert(def->to_array != NULL);
+               if (def->to_array(val, ival, opt, errcode, field_no) != 0)
+                       return -1;
+               break;