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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

>>> diff --git a/src/box/iproto.cc <http://iproto.cc> b/src/box/iproto.cc <http://iproto.cc>
>>> index f313af6ae..d5891391d 100644
>>> --- a/src/box/iproto.cc <http://iproto.cc>
>>> +++ b/src/box/iproto.cc <http://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?

I changed 'rc' and port_destroy(), because you changed the same lines. So
my diff does not make the final patch bigger and touches only lines which
would be changed anyway, except 'rc' declaration. I couldn't declare it in
place, because the compiler complained about 'rc' not declared for 'end'
label, if jumped from unprepare execution.

> 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.

Did you check assembler?

> If you (or someone else) insist, however, I will apply it and resend.

I don't insist. You can keep it as is.

LGTM.