From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 2BE2524C5E for ; Fri, 25 May 2018 07:54:16 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ZiG7tQn9h7-t for ; Fri, 25 May 2018 07:54:16 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id DB64E24C57 for ; Fri, 25 May 2018 07:54:15 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v7 2/7] box: introduce OPT_ARRAY opt_type to decode arrays References: <73780eb4-fbd6-68cd-fdcf-bec1e6c354a3@tarantool.org> From: Kirill Shcherbatov Message-ID: <126d3d80-f249-6216-7974-c31cb49ead8f@tarantool.org> Date: Fri, 25 May 2018 14:54:14 +0300 MIME-Version: 1.0 In-Reply-To: <73780eb4-fbd6-68cd-fdcf-bec1e6c354a3@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: "v.shpilevoy@tarantool.org" > 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;