Re: [Tarantool-patches] [PATCH v2] iproto: add an empty body to the unprepare response

[Chris Sosnin <k.sosnin@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 4147 bytes --]

Thank you for the review!

> 4 марта 2020 г., в 01:33, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> написал(а):
> 
> Hi! Thanks for the patch!
> 
> On 03/03/2020 17:16, Chris Sosnin wrote:
>> Absence of the body in the unprepare response forces users to perform
>> additional checks to avoid errors. Adding an empty body fixes this problem.
>> 
>> Closes #4769
>> ---
>> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4769-unprepare-response-body
>> issue: https://github.com/tarantool/tarantool/issues/4769
>> 
>> As Nikita suggested, I created box/iproto.test.lua, and basically
>> inserted wrappers for requests testing from box-py for future usage.
>> 
>> src/box/iproto.cc        | 17 ++++----
>> test/box/iproto.result   | 86 ++++++++++++++++++++++++++++++++++++++++
>> test/box/iproto.test.lua | 54 +++++++++++++++++++++++++
>> 3 files changed, 150 insertions(+), 7 deletions(-)
>> create mode 100644 test/box/iproto.result
>> create mode 100644 test/box/iproto.test.lua
>> 
>> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
>> index f313af6ae..d5891391d 100644
>> --- a/src/box/iproto.cc
>> +++ b/src/box/iproto.cc
>> @@ -1781,20 +1781,23 @@ tx_process_sql(struct cmsg *m)
>> 	 */
>> 	out = msg->connection->tx.p_obuf;
>> 	struct obuf_svp header_svp;
>> +	if (is_unprepare) {
> 
> Looks like you don't need that flag anymore. Consider the diff:
> 
> ====================
> diff --git a/src/box/iproto.cc <http://iproto.cc/> b/src/box/iproto.cc <http://iproto.cc/>
> index d5891391d..28095f5b8 100644
> --- a/src/box/iproto.cc <http://iproto.cc/>
> +++ b/src/box/iproto.cc <http://iproto.cc/>
> @@ -1716,10 +1716,9 @@ tx_process_sql(struct cmsg *m)
> 	struct obuf *out;
> 	struct port port;
> 	struct sql_bind *bind = NULL;
> -	int bind_count = 0;
> +	int rc, bind_count = 0;
> 	const char *sql;
> 	uint32_t len;
> -	bool is_unprepare = false;
> 
> 	if (tx_check_schema(msg->header.schema_version))
> 		goto error;
> @@ -1771,7 +1770,12 @@ tx_process_sql(struct cmsg *m)
> 			uint32_t stmt_id = mp_decode_uint(&sql);
> 			if (sql_unprepare(stmt_id) != 0)
> 				goto error;
> -			is_unprepare = true;
> +			out = msg->connection->tx.p_obuf;
> +			if (iproto_reply_ok(out, msg->header.sync,
> +					    schema_version) != 0)
> +				goto error;
> +			iproto_wpos_create(&msg->wpos, out);
> +			return;
> 		}
> 	}
> 
> @@ -1781,23 +1785,17 @@ tx_process_sql(struct cmsg *m)
> 	 */
> 	out = msg->connection->tx.p_obuf;
> 	struct obuf_svp header_svp;
> -	if (is_unprepare) {
> -		if (iproto_reply_ok(out, msg->header.sync, schema_version) != 0)
> -			goto error;
> -		iproto_wpos_create(&msg->wpos, out);
> -		return;
> -	}
> 	/* Prepare memory for the iproto header. */
> 	if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0) {
> 		port_destroy(&port);
> 		goto error;
> 	}
> -	if (port_dump_msgpack(&port, out) != 0) {
> -		port_destroy(&port);
> +	rc = port_dump_msgpack(&port, out);
> +	port_destroy(&port);
> +	if (rc != 0) {
> 		obuf_rollback_to_svp(out, &header_svp);
> 		goto error;
> 	}
> -	port_destroy(&port);
> 	iproto_reply_sql(out, &header_svp, msg->header.sync, schema_version);
> 	iproto_wpos_create(&msg->wpos, out);
> 	return;

The change with rc and avoiding code duplication is more about refactoring, should we apply
it here? As for the flag, the current code is well separated, all switch-cases perform the execution, 
everything after is about preparing the message to send.

Maybe it’s only me, but it looks cleaner without this diff, not to mention it produces the same code.
If you (or someone else) insist, however, I will apply it and resend.

I added one small change to the branch:
        out = msg->connection->tx.p_obuf;
-       struct obuf_svp header_svp;
        if (is_unprepare) {
	...
	}
+       struct obuf_svp header_svp;
        /* Prepare memory for the iproto header. */
        if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0) {

This variable is not needed in case of unprepare, so we could declare it later.


[-- Attachment #2: Type: text/html, Size: 92978 bytes --]