[tarantool-patches] Re: [PATCH v1 02/10] iproto: remove iproto functions from execute.c
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Nov 19 20:58:00 MSK 2018
Thanks for the patch!
On 17/11/2018 17:03, imeevma at 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 <v.shpilevoy at tarantool.org>
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
More information about the Tarantool-patches
mailing list