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 462FE2CC93 for ; Mon, 19 Nov 2018 12:58:03 -0500 (EST) 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 K0hIzl5pdkVj for ; Mon, 19 Nov 2018 12:58:03 -0500 (EST) 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 D67E22CC77 for ; Mon, 19 Nov 2018 12:58:02 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v1 02/10] iproto: remove iproto functions from execute.c References: <9c83174c116a86f4c11a1ff2ea3a16052fab3e28.1542460773.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: <9df3b10d-45c0-41f8-2886-6e365c1b0974@tarantool.org> Date: Mon, 19 Nov 2018 20:58:00 +0300 MIME-Version: 1.0 In-Reply-To: <9c83174c116a86f4c11a1ff2ea3a16052fab3e28.1542460773.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed 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: imeevma@tarantool.org, tarantool-patches@freelists.org Thanks for the patch! On 17/11/2018 17:03, imeevma@tarantool.org wrote: > To make functions in execute.h more universal we should reduce > their dependence on IPROTO. This patch removes IPROTO functions > from execute.c. > > Needed for #3505 > --- > src/box/execute.c | 116 ++++++++++++++---------------------------------------- > src/box/execute.h | 34 ++++++++-------- > src/box/iproto.cc | 71 ++++++++++++++++++++++++++++++++- > src/box/xrow.c | 39 ------------------ > src/box/xrow.h | 14 ------- > 5 files changed, 115 insertions(+), 159 deletions(-) > 1. To summarize the patch: you have moved xrow_decode_sql from execute.c into iproto.cc. But I do not think we should do it. The only goal of the whole patchset is removal of any IPROTO mentions not from execute.c, but from sql_response_dump and functions which it calls. I did it in a separate commit on the branch. I've attached it to the end of the email, and pushed onto the branch. > diff --git a/src/box/iproto.cc b/src/box/iproto.cc > index cd61393..5fb2aff 100644 > --- a/src/box/iproto.cc > +++ b/src/box/iproto.cc > @@ -1118,6 +1118,67 @@ static const struct cmsg_hop error_route[] = { > { net_send_error, NULL }, > }; > > +/** > + * Parse the EXECUTE request. > + * @param row Encoded data. > + * @param[out] request Request to decode to. > + * @param region Allocator. > + * > + * @retval 0 Sucess. > + * @retval -1 Format or memory error. > + */ > +static inline int > +xrow_decode_sql(const struct xrow_header *row, struct sql_request *request, > + struct region *region) > +{ > + if (row->bodycnt == 0) { > + diag_set(ClientError, ER_INVALID_MSGPACK, "missing request body"); > + return 1; > + } > + assert(row->bodycnt == 1); > + const char *data = (const char *) row->body[0].iov_base; > + const char *end = data + row->body[0].iov_len; > + assert((end - data) > 0); > + > + if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) { > +error: > + diag_set(ClientError, ER_INVALID_MSGPACK, "packet body"); > + return -1; > + } > + > + uint32_t map_size = mp_decode_map(&data); > + request->sql_text = NULL; > + request->bind = NULL; > + request->bind_count = 0; > + request->sync = row->sync; > + for (uint32_t i = 0; i < map_size; ++i) { > + uint8_t key = *data; > + if (key != IPROTO_SQL_BIND && key != IPROTO_SQL_TEXT) { > + mp_check(&data, end); /* skip the key */ > + mp_check(&data, end); /* skip the value */ > + continue; > + } > + const char *value = ++data; /* skip the key */ > + if (mp_check(&data, end) != 0) /* check the value */ > + goto error; > + if (key == IPROTO_SQL_BIND) { > + if (sql_bind_list_decode(request, value, region) != 0) > + return -1; > + } else { > + request->sql_text = > + mp_decode_str(&value, &request->sql_text_len); 2. You have changed this lines: > - request->sql_text = value; > request->sql_text = > - mp_decode_str(&request->sql_text, > - &request->sql_text_len); > + mp_decode_str(&value, &request->sql_text_len); Looks like a bug of the previous patch, and should be merged into it. I did it. > + } > + } =============================================================================== commit b52d916348c65d35ec09d1a9842cc0244d9e6c9a Author: Vladislav Shpilevoy Date: Mon Nov 19 17:31:27 2018 +0300 Review fixes diff --git a/src/box/execute.c b/src/box/execute.c index 0cba23360..8d5caf910 100644 --- a/src/box/execute.c +++ b/src/box/execute.c @@ -194,7 +194,23 @@ sql_bind_decode(struct sql_bind *bind, int i, const char **packet) return 0; } -int +/** + * Parse MessagePack array of SQL parameters and store a result + * into the @request->bind, bind_count. + * @param request Request to save decoded parameters. + * @param data MessagePack array of parameters. Each parameter + * either must have scalar type, or must be a map with the + * following format: {name: value}. Name - string name of + * the named parameter, value - scalar value of the + * parameter. Named and positioned parameters can be mixed. + * For more details + * @sa https://www.sqlite.org/lang_expr.html#varparam. + * @param region Allocator. + * + * @retval 0 Success. + * @retval -1 Client or memory error. + */ +static inline int sql_bind_list_decode(struct sql_request *request, const char *data, struct region *region) { @@ -230,6 +246,58 @@ sql_bind_list_decode(struct sql_request *request, const char *data, return 0; } +int +xrow_decode_sql(const struct xrow_header *row, struct sql_request *request, + struct region *region) +{ + if (row->bodycnt == 0) { + diag_set(ClientError, ER_INVALID_MSGPACK, "missing request body"); + return 1; + } + assert(row->bodycnt == 1); + const char *data = (const char *) row->body[0].iov_base; + const char *end = data + row->body[0].iov_len; + assert((end - data) > 0); + + if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) { +error: + diag_set(ClientError, ER_INVALID_MSGPACK, "packet body"); + return -1; + } + + uint32_t map_size = mp_decode_map(&data); + request->sql_text = NULL; + request->bind = NULL; + request->bind_count = 0; + request->sync = row->sync; + for (uint32_t i = 0; i < map_size; ++i) { + uint8_t key = *data; + if (key != IPROTO_SQL_BIND && key != IPROTO_SQL_TEXT) { + mp_check(&data, end); /* skip the key */ + mp_check(&data, end); /* skip the value */ + continue; + } + const char *value = ++data; /* skip the key */ + if (mp_check(&data, end) != 0) /* check the value */ + goto error; + if (key == IPROTO_SQL_BIND) { + if (sql_bind_list_decode(request, value, region) != 0) + return -1; + } else { + request->sql_text = + mp_decode_str(&value, &request->sql_text_len); + } + } + if (request->sql_text == NULL) { + diag_set(ClientError, ER_MISSING_REQUEST_FIELD, + iproto_key_name(IPROTO_SQL_TEXT)); + return -1; + } + if (data != end) + goto error; + return 0; +} + /** * Serialize a single column of a result set row. * @param stmt Prepared and started statement. At least one @@ -466,15 +534,15 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out, int column_count) { assert(column_count > 0); - char *pos = (char *) obuf_alloc(out, IPROTO_KEY_HEADER_LEN); + int size = mp_sizeof_uint(IPROTO_METADATA) + + mp_sizeof_array(column_count); + char *pos = (char *) obuf_alloc(out, size); if (pos == NULL) { - diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN, "obuf_alloc", - "pos"); + diag_set(OutOfMemory, size, "obuf_alloc", "pos"); return -1; } - pos = mp_store_u8(pos, IPROTO_METADATA); - pos = mp_store_u8(pos, 0xdd); - pos = mp_store_u32(pos, column_count); + pos = mp_encode_uint(pos, IPROTO_METADATA); + pos = mp_encode_array(pos, column_count); for (int i = 0; i < column_count; ++i) { const char *name = sqlite3_column_name(stmt, i); const char *type = sqlite3_column_datatype(stmt, i); @@ -576,15 +644,15 @@ err: goto finish; } *keys = 2; - char *pos = (char *) obuf_alloc(out, IPROTO_KEY_HEADER_LEN); + int size = mp_sizeof_uint(IPROTO_DATA) + + mp_sizeof_array(port_tuple->size); + char *pos = (char *) obuf_alloc(out, size); if (pos == NULL) { - diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN, - "obuf_alloc", "pos"); + diag_set(OutOfMemory, size, "obuf_alloc", "pos"); goto err; } - pos = mp_store_u8(pos, IPROTO_DATA); - pos = mp_store_u8(pos, 0xdd); - pos = mp_store_u32(pos, port_tuple->size); + pos = mp_encode_uint(pos, IPROTO_DATA); + pos = mp_encode_array(pos, port_tuple->size); /* * Just like SELECT, SQL uses output format compatible * with Tarantool 1.6 @@ -599,19 +667,19 @@ err: struct stailq *autoinc_id_list = vdbe_autoinc_id_list((struct Vdbe *)stmt); uint32_t map_size = stailq_empty(autoinc_id_list) ? 1 : 2; - char *pos = (char *) obuf_alloc(out, IPROTO_KEY_HEADER_LEN); + int size = mp_sizeof_uint(IPROTO_SQL_INFO) + + mp_sizeof_map(map_size); + char *pos = (char *) obuf_alloc(out, size); if (pos == NULL) { - diag_set(OutOfMemory, IPROTO_KEY_HEADER_LEN, - "obuf_alloc", "pos"); + diag_set(OutOfMemory, size, "obuf_alloc", "pos"); goto err; } - pos = mp_store_u8(pos, IPROTO_SQL_INFO); - pos = mp_store_u8(pos, 0xdf); - pos = mp_store_u32(pos, map_size); + pos = mp_encode_uint(pos, IPROTO_SQL_INFO); + pos = mp_encode_map(pos, map_size); uint64_t id_count = 0; int changes = db->nChange; - int size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) + - mp_sizeof_uint(changes); + size = mp_sizeof_uint(SQL_INFO_ROW_COUNT) + + mp_sizeof_uint(changes); if (!stailq_empty(autoinc_id_list)) { struct autoinc_id_entry *id_entry; stailq_foreach_entry(id_entry, autoinc_id_list, link) { diff --git a/src/box/execute.h b/src/box/execute.h index 2a242e857..5f3d5eb59 100644 --- a/src/box/execute.h +++ b/src/box/execute.h @@ -114,24 +114,17 @@ int sql_response_dump(struct sql_response *response, int *keys, struct obuf *out); /** - * Parse MessagePack array of SQL parameters and store a result - * into the @request->bind, bind_count. - * @param request Request to save decoded parameters. - * @param data MessagePack array of parameters. Each parameter - * either must have scalar type, or must be a map with the - * following format: {name: value}. Name - string name of - * the named parameter, value - scalar value of the - * parameter. Named and positioned parameters can be mixed. - * For more details - * @sa https://www.sqlite.org/lang_expr.html#varparam. + * Parse the EXECUTE request. + * @param row Encoded data. + * @param[out] request Request to decode to. * @param region Allocator. * - * @retval 0 Success. - * @retval -1 Client or memory error. + * @retval 0 Sucess. + * @retval -1 Format or memory error. */ int -sql_bind_list_decode(struct sql_request *request, const char *data, - struct region *region); +xrow_decode_sql(const struct xrow_header *row, struct sql_request *request, + struct region *region); /** * Prepare and execute an SQL statement. diff --git a/src/box/iproto.cc b/src/box/iproto.cc index 5fb2affa3..7c11d05b3 100644 --- a/src/box/iproto.cc +++ b/src/box/iproto.cc @@ -1118,67 +1118,6 @@ static const struct cmsg_hop error_route[] = { { net_send_error, NULL }, }; -/** - * Parse the EXECUTE request. - * @param row Encoded data. - * @param[out] request Request to decode to. - * @param region Allocator. - * - * @retval 0 Sucess. - * @retval -1 Format or memory error. - */ -static inline int -xrow_decode_sql(const struct xrow_header *row, struct sql_request *request, - struct region *region) -{ - if (row->bodycnt == 0) { - diag_set(ClientError, ER_INVALID_MSGPACK, "missing request body"); - return 1; - } - assert(row->bodycnt == 1); - const char *data = (const char *) row->body[0].iov_base; - const char *end = data + row->body[0].iov_len; - assert((end - data) > 0); - - if (mp_typeof(*data) != MP_MAP || mp_check_map(data, end) > 0) { -error: - diag_set(ClientError, ER_INVALID_MSGPACK, "packet body"); - return -1; - } - - uint32_t map_size = mp_decode_map(&data); - request->sql_text = NULL; - request->bind = NULL; - request->bind_count = 0; - request->sync = row->sync; - for (uint32_t i = 0; i < map_size; ++i) { - uint8_t key = *data; - if (key != IPROTO_SQL_BIND && key != IPROTO_SQL_TEXT) { - mp_check(&data, end); /* skip the key */ - mp_check(&data, end); /* skip the value */ - continue; - } - const char *value = ++data; /* skip the key */ - if (mp_check(&data, end) != 0) /* check the value */ - goto error; - if (key == IPROTO_SQL_BIND) { - if (sql_bind_list_decode(request, value, region) != 0) - return -1; - } else { - request->sql_text = - mp_decode_str(&value, &request->sql_text_len); - } - } - if (request->sql_text == NULL) { - diag_set(ClientError, ER_MISSING_REQUEST_FIELD, - iproto_key_name(IPROTO_SQL_TEXT)); - return -1; - } - if (data != end) - goto error; - return 0; -} - static void iproto_msg_decode(struct iproto_msg *msg, const char **pos, const char *reqend, bool *stop_input) diff --git a/src/box/xrow.c b/src/box/xrow.c index 8a589cbd6..76c6f8166 100644 --- a/src/box/xrow.c +++ b/src/box/xrow.c @@ -445,12 +445,6 @@ iproto_prepare_header(struct obuf *buf, struct obuf_svp *svp, size_t size) return 0; } -struct PACKED iproto_key_bin { - uint8_t key; /* IPROTO_DATA/METADATA/SQL_INFO */ - uint8_t mp_type; - uint32_t mp_len; -}; - void iproto_reply_select(struct obuf *buf, struct obuf_svp *svp, uint64_t sync, uint32_t schema_version, uint32_t count) diff --git a/src/box/xrow.h b/src/box/xrow.h index 13071ff3b..ca8d04d44 100644 --- a/src/box/xrow.h +++ b/src/box/xrow.h @@ -52,11 +52,6 @@ enum { IPROTO_HEADER_LEN = 28, /** 7 = sizeof(iproto_body_bin). */ IPROTO_SELECT_HEADER_LEN = IPROTO_HEADER_LEN + 7, - /** - * mp_sizeof(IPROTO_DATA/METADATA/SQL_INFO) + - * mp_sizeof_array(UINT32_MAX). - */ - IPROTO_KEY_HEADER_LEN = 1 + 5, /** * Header of message + header of body with one or two * keys: IPROTO_DATA and IPROTO_METADATA or