[tarantool-patches] Re: [PATCH v1 02/10] iproto: remove iproto functions from execute.c

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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 <v.shpilevoy@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