From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 E884E469719 for ; Wed, 4 Mar 2020 11:14:15 +0300 (MSK) From: Chris Sosnin Message-Id: <465B5192-A6D8-45BE-8FA7-627BA804E22D@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_DE10557B-0998-491D-B789-C6F5327F9031" Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.40.2.2.4\)) Date: Wed, 4 Mar 2020 11:14:12 +0300 In-Reply-To: References: <20200303161649.62470-1-k.sosnin@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org --Apple-Mail=_DE10557B-0998-491D-B789-C6F5327F9031 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Thank you for the review! > 4 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 2020 =D0=B3., =D0=B2 01:33, Vladislav = Shpilevoy =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0= =D0=BB(=D0=B0): >=20 > Hi! Thanks for the patch! >=20 > 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. >>=20 >> Closes #4769 >> --- >> branch: = https://github.com/tarantool/tarantool/tree/ksosnin/gh-4769-unprepare-resp= onse-body >> issue: https://github.com/tarantool/tarantool/issues/4769 >>=20 >> As Nikita suggested, I created box/iproto.test.lua, and basically >> inserted wrappers for requests testing from box-py for future usage. >>=20 >> 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 >>=20 >> 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 =3D msg->connection->tx.p_obuf; >> struct obuf_svp header_svp; >> + if (is_unprepare) { >=20 > Looks like you don't need that flag anymore. Consider the diff: >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > 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 =3D NULL; > - int bind_count =3D 0; > + int rc, bind_count =3D 0; > const char *sql; > uint32_t len; > - bool is_unprepare =3D false; >=20 > if (tx_check_schema(msg->header.schema_version)) > goto error; > @@ -1771,7 +1770,12 @@ tx_process_sql(struct cmsg *m) > uint32_t stmt_id =3D mp_decode_uint(&sql); > if (sql_unprepare(stmt_id) !=3D 0) > goto error; > - is_unprepare =3D true; > + out =3D msg->connection->tx.p_obuf; > + if (iproto_reply_ok(out, msg->header.sync, > + schema_version) !=3D 0) > + goto error; > + iproto_wpos_create(&msg->wpos, out); > + return; > } > } >=20 > @@ -1781,23 +1785,17 @@ tx_process_sql(struct cmsg *m) > */ > out =3D msg->connection->tx.p_obuf; > struct obuf_svp header_svp; > - if (is_unprepare) { > - if (iproto_reply_ok(out, msg->header.sync, = schema_version) !=3D 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) = !=3D 0) { > port_destroy(&port); > goto error; > } > - if (port_dump_msgpack(&port, out) !=3D 0) { > - port_destroy(&port); > + rc =3D port_dump_msgpack(&port, out); > + port_destroy(&port); > + if (rc !=3D 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,=20 everything after is about preparing the message to send. Maybe it=E2=80=99s 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 =3D 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) = !=3D 0) { This variable is not needed in case of unprepare, so we could declare it = later. --Apple-Mail=_DE10557B-0998-491D-B789-C6F5327F9031 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Thank= you for the review!

4 =D0=BC=D0=B0=D1=80=D1=82=D0=B0 = 2020 =D0=B3., =D0=B2 01:33, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):

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-unp= repare-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 =3D = 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:

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
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 =3D NULL;
- int bind_count =3D 0;
+ int rc, bind_count =3D 0;
= const char = *sql;
uint32_t len;
- bool is_unprepare =3D 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 =3D mp_decode_uint(&sql);
= if (sql_unprepare(stmt_id) !=3D 0)
= goto = error;
- = is_unprepare = =3D true;
+ = out =3D = msg->connection->tx.p_obuf;
+ = if = (iproto_reply_ok(out, msg->header.sync,
+ = =     schema_version) = !=3D 0)
+ = goto = error;
+ = iproto_wpos_create(&msg->wpos, out);
+ = return;
= }
}

@@ -1781,23 +1785,17 @@ = tx_process_sql(struct cmsg *m)
=  */
out =3D = msg->connection->tx.p_obuf;
= struct = obuf_svp header_svp;
- if (is_unprepare) {
- = if = (iproto_reply_ok(out, msg->header.sync, schema_version) !=3D = 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) !=3D 0) = {
= port_destroy(&port);
= goto = error;
}
- if = (port_dump_msgpack(&port, out) !=3D 0) {
- port_destroy(&port);
+ rc =3D port_dump_msgpack(&port, out);
+ port_destroy(&port);
+ if (rc !=3D 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=E2=80=99s = 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 =3D = 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) !=3D 0) = {

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

= --Apple-Mail=_DE10557B-0998-491D-B789-C6F5327F9031--