From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id F39C0469719 for ; Thu, 5 Mar 2020 01:39:09 +0300 (MSK) References: <20200303161649.62470-1-k.sosnin@tarantool.org> <465B5192-A6D8-45BE-8FA7-627BA804E22D@tarantool.org> From: Vladislav Shpilevoy Message-ID: <380d14ba-1008-8edc-eb9b-5ebfdd205b4a@tarantool.org> Date: Wed, 4 Mar 2020 23:39:08 +0100 MIME-Version: 1.0 In-Reply-To: <465B5192-A6D8-45BE-8FA7-627BA804E22D@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH v2] iproto: add an empty body to the unprepare response List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chris Sosnin Cc: tarantool-patches@dev.tarantool.org >>> 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  b/src/box/iproto.cc >> index d5891391d..28095f5b8 100644 >> --- a/src/box/iproto.cc >> +++ b/src/box/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.