From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 424616B962; Wed, 14 Apr 2021 04:21:46 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 424616B962 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618363306; bh=zAqVZs/cxsFeZNh5ywVREV/msa6tfz66SccAjcWXI9k=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=y9tA/OsKlChwFcKRmbFxK+xjRsi+r+qxIUyfFU/DJKAl8+RNELM+1K1o+f1I6aYzb jYxjS1oewOw9+bsHb+IIaYLaPWacpxrw435rWML5InHsLwYumRrc5ghfE6Cb6SIafj vxHaRBL22aRmhFOvMWTO8spCBKZr43oh3xsFTx/4= Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 6C6DD6EC5B for ; Wed, 14 Apr 2021 04:21:45 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6C6DD6EC5B Received: by smtp45.i.mail.ru with esmtpa (envelope-from ) id 1lWUDw-0001Q8-MR; Wed, 14 Apr 2021 04:21:45 +0300 Date: Wed, 14 Apr 2021 04:21:43 +0300 To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org Message-ID: <20210414012143.GA163761@tarantool.org> References: <29a16259-d26a-4391-4a8d-0439fec146a0@tarantool.org> <20210414003948.GA140789@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210414003948.GA140789@tarantool.org> X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E74809299FB6B3996B874F289A033C44AD400182A05F538085040F336D9FB01527C7816B4F62622FFCFC8A33EBB2D0D62479FD3917B8CB6DAA3C9 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE793ECC5BC4DAF6EC5EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637EDC9855826FBDF5A8638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B246CE0332F62E7435A04C375C4342CD743896EE2C32233946D2E47CDBA5A96583C09775C1D3CA48CFCA5A41EBD8A3A0199FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C317B107DEF921CE79117882F4460429728AD0CFFFB425014E868A13BD56FB6657D81D268191BDAD3DC09775C1D3CA48CFD5430F8B4ED67681BA3038C0950A5D36C8A9BA7A39EFB766EC990983EF5C0329BA3038C0950A5D36D5E8D9A59859A8B60E0E8EDE14C8C9B276E601842F6C81A1F004C906525384307823802FF610243D2EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE72128E1227B50B07D731C566533BA786AA5CC5B56E945C8DA X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A24209795067102C07E8F7B195E1C97831EE106B6063199918DFDF0179112761E5 X-C1DE0DAB: 0D63561A33F958A5C67A792F420A6314B83A7801B155A36324577DAF916D73A9D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34FF70CCB59A6ED870440DF27EABF79D8EE9F943E49AF796DF48A838FD5318E6060E8D085853D3DA5D1D7E09C32AA3244C12BFC84FABACD3F95E674FCF6ED2A041A95CA90A1D8AC565729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojnA7/qPBUIXE4K1EpzeDx1A== X-Mailru-Sender: 5C3750E245F362008BC1685FEC6306ED345692CDE00CBFD016B4F62622FFCFC8F1FEA02A07AA46D65105BD0848736F9966FEC6BF5C9C28D97E07721503EA2E00ED97202A5A4E92BF7402F9BA4338D657ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5 46/52] sql: introduce mem_get_uint() X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Mergen Imeev via Tarantool-patches Reply-To: Mergen Imeev Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Now mem_get_uint_unsafe() returns 0 if mem_get_int() fails. Diff below. On Wed, Apr 14, 2021 at 03:39:48AM +0300, Mergen Imeev via Tarantool-patches wrote: > Thank you for the review! My answer, diff and new patch below. > > > On Wed, Apr 14, 2021 at 01:04:05AM +0200, Vladislav Shpilevoy wrote: > > Thanks for the fixes! > > > > On 09.04.2021 23:08, Mergen Imeev via Tarantool-patches wrote: > > > Thank you for the review! My answer and new patch below. > > > > > > > > > On 30.03.2021 02:08, Vladislav Shpilevoy wrote: > > >> Thanks for the patch! > > >> > > >> On 23.03.2021 10:36, Mergen Imeev via Tarantool-patches wrote: > > >>> This patch introduces mem_get_unsigned() function which is used to > > >>> receive unsigned value from MEM. > > >>> > > >>> Part of #5818 > > >>> --- > > >>> src/box/sql/func.c | 16 +++++++++++----- > > >>> src/box/sql/mem.c | 37 +++++++++++++++++++++++++++---------- > > >>> src/box/sql/mem.h | 6 +++--- > > >>> src/box/sql/sqlInt.h | 3 --- > > >>> src/box/sql/vdbeapi.c | 6 ------ > > >>> 5 files changed, 41 insertions(+), 27 deletions(-) > > >>> > > >>> diff --git a/src/box/sql/func.c b/src/box/sql/func.c > > >>> index 0fa0f6ac7..a851d98f2 100644 > > >>> --- a/src/box/sql/func.c > > >>> +++ b/src/box/sql/func.c > > >>> @@ -118,9 +118,12 @@ port_vdbemem_dump_lua(struct port *base, struct lua_State *L, bool is_flat) > > >>> luaL_pushint64(L, n); > > >>> break; > > >>> } > > >>> - case MP_UINT: > > >>> - luaL_pushuint64(L, sql_value_uint64(param)); > > >>> + case MP_UINT: { > > >>> + uint64_t u; > > >>> + mem_get_unsigned(param, &u); > > >>> + luaL_pushuint64(L, u); > > >> Maybe we could make 2 functions? One to get the value and ignore > > >> the errors, and the other to get as an out parameter + return an > > >> error? > > >> > > >> For instance, mem_to_uint() - returns uint64_t and internally asserts > > >> that the value is correct. And mem_get_uint() works like your version. > > >> > > >> The same for the other get functions whose result is often ignored. > > > For some functions I created a "proxy" functions in func.c the way you > > > described, but not for this function since it is only used in a few places of > > > sql/func.c. Should I do this for all functions? In func.c I mean. I see this as > > > temporary measure, since I hope we will rework built-in functions one day. > > > > Unfortunately, 'hope' is not enough. And it is highly possible the code > > will live for long. Therefore I think we need to make it solid where possible > > and clearly state it is unsafe or add assertions where it is not possible. > > > > Here mem_get_uint() result is ignored always. Even if it fails. I think it > > must be called something like mem_get_uint_unsafe() and return the uint as > > 'return', not via an out argument. Then at least we would see it is broken > > when we are around this code again, and it won't raise questions if it is a > > known issue, and why it is not fixed (this must be in a comment for the > > function). > Understood. I created a new function, mem_get_uint_unsafe(). > > > Diff: > > > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index 6e6978bbc..5503a9b16 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > @@ -201,9 +201,7 @@ absFunc(sql_context * context, int argc, sql_value ** argv) > UNUSED_PARAMETER(argc); > switch (sql_value_type(argv[0])) { > case MP_UINT: { > - uint64_t u; > - mem_get_uint(argv[0], &u); > - sql_result_uint(context, u); > + sql_result_uint(context, mem_get_uint_unsafe(argv[0])); > break; > } > case MP_INT: { > @@ -1171,7 +1169,7 @@ charFunc(sql_context * context, int argc, sql_value ** argv) > if (sql_value_type(argv[i]) == MP_INT) > x = 0xfffd; > else > - mem_get_uint(argv[i], &x); > + x = mem_get_uint_unsafe(argv[i]); > if (x > 0x10ffff) > x = 0xfffd; > c = (unsigned)(x & 0x1fffff); > diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h > index 4aadcd3f7..ca8a75b50 100644 > --- a/src/box/sql/mem.h > +++ b/src/box/sql/mem.h > @@ -776,6 +776,18 @@ mem_get_int_unsafe(const struct Mem *mem) > int > mem_get_uint(const struct Mem *mem, uint64_t *u); > > +/** > + * Return value of MEM converted to uint64_t. This function is not safe, since > + * its behaviour is undefined if mem_get_uint() returned an error. > + */ > +static inline uint64_t > +mem_get_uint_unsafe(const struct Mem *mem) > +{ > + uint64_t u; > + mem_get_uint(mem, &u); > + return u; > +} > + > /** > * Simple type to str convertor. It is used to simplify > * error reporting. > > > > New patch: > > > commit b9ca33e93110ecf167329a0f58473371de5c7c45 > Author: Mergen Imeev > Date: Wed Mar 17 13:55:37 2021 +0300 > > sql: introduce mem_get_uint() > > This patch introduces mem_get_uint() function. This function is used to > receive unsigned value from MEM. If value of MEM is not unsigned, it is > converted to unsigned if possible. MEM is not changed. > > Part of #5818 > > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index 0db698174..5503a9b16 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > @@ -201,7 +201,7 @@ absFunc(sql_context * context, int argc, sql_value ** argv) > UNUSED_PARAMETER(argc); > switch (sql_value_type(argv[0])) { > case MP_UINT: { > - sql_result_uint(context, sql_value_uint64(argv[0])); > + sql_result_uint(context, mem_get_uint_unsafe(argv[0])); > break; > } > case MP_INT: { > @@ -1169,7 +1169,7 @@ charFunc(sql_context * context, int argc, sql_value ** argv) > if (sql_value_type(argv[i]) == MP_INT) > x = 0xfffd; > else > - x = sql_value_uint64(argv[i]); > + x = mem_get_uint_unsafe(argv[i]); > if (x > 0x10ffff) > x = 0xfffd; > c = (unsigned)(x & 0x1fffff); > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > index adf5e236b..ab31029df 100644 > --- a/src/box/sql/mem.c > +++ b/src/box/sql/mem.c > @@ -1074,6 +1074,33 @@ mem_get_int(const struct Mem *mem, int64_t *i, bool *is_neg) > return -1; > } > > +int > +mem_get_uint(const struct Mem *mem, uint64_t *u) > +{ > + if ((mem->flags & MEM_Int) != 0) > + return -1; > + if ((mem->flags & MEM_UInt) != 0) { > + *u = mem->u.u; > + return 0; > + } > + if ((mem->flags & (MEM_Str | MEM_Blob)) != 0) { > + bool is_neg; > + if (sql_atoi64(mem->z, (int64_t *)u, &is_neg, mem->n) != 0 || > + is_neg) > + return -1; > + return 0; > + } > + if ((mem->flags & MEM_Real) != 0) { > + double d = mem->u.r; > + if (d >= 0 && d < (double)UINT64_MAX) { > + *u = (uint64_t)d; > + return 0; > + } > + return -1; > + } > + return -1; > +} > + > int > mem_copy(struct Mem *to, const struct Mem *from) > { > @@ -2283,16 +2310,6 @@ sql_value_boolean(sql_value *val) > return b; > } > > -uint64_t > -sql_value_uint64(sql_value *val) > -{ > - int64_t i = 0; > - bool is_neg; > - mem_get_int((struct Mem *) val, &i, &is_neg); > - assert(!is_neg); > - return i; > -} > - > const unsigned char * > sql_value_text(sql_value * pVal) > { > diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h > index f3d9043e5..ca8a75b50 100644 > --- a/src/box/sql/mem.h > +++ b/src/box/sql/mem.h > @@ -768,6 +768,26 @@ mem_get_int_unsafe(const struct Mem *mem) > return i; > } > > +/** > + * Return value for MEM of UNSIGNED type. For MEM of all other types convert > + * value of the MEM to UNSIGNED if possible and return converted value. Original > + * MEM is not changed. > + */ > +int > +mem_get_uint(const struct Mem *mem, uint64_t *u); > + > +/** > + * Return value of MEM converted to uint64_t. This function is not safe, since > + * its behaviour is undefined if mem_get_uint() returned an error. > + */ > +static inline uint64_t > +mem_get_uint_unsafe(const struct Mem *mem) > +{ > + uint64_t u; > + mem_get_uint(mem, &u); > + return u; > +} > + > /** > * Simple type to str convertor. It is used to simplify > * error reporting. > @@ -835,9 +855,6 @@ sql_value_double(struct Mem *); > bool > sql_value_boolean(struct Mem *val); > > -uint64_t > -sql_value_uint64(struct Mem *val); > - > const unsigned char * > sql_value_text(struct Mem *); > > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index 0af247ebf..6ead9b261 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -448,9 +448,6 @@ sql_column_double(sql_stmt *, int iCol); > bool > sql_column_boolean(struct sql_stmt *stmt, int column); > > -uint64_t > -sql_column_uint64(struct sql_stmt *stmt, int column); > - > const unsigned char * > sql_column_text(sql_stmt *, > int iCol); > diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c > index 5e5957496..1126425bc 100644 > --- a/src/box/sql/vdbeapi.c > +++ b/src/box/sql/vdbeapi.c > @@ -476,12 +476,6 @@ sql_column_boolean(struct sql_stmt *stmt, int i) > return sql_value_boolean(columnMem(stmt, i)); > } > > -uint64_t > -sql_column_uint64(sql_stmt * pStmt, int i) > -{ > - return sql_value_uint64(columnMem(pStmt, i)); > -} > - > const unsigned char * > sql_column_text(sql_stmt * pStmt, int i) > { Diff: diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index a4121cfac..c2b337414 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -778,14 +778,15 @@ int mem_get_uint(const struct Mem *mem, uint64_t *u); /** - * Return value of MEM converted to uint64_t. This function is not safe, since - * its behaviour is undefined if mem_get_uint() returned an error. + * Return value of MEM converted to uint64_t. This function is not safe, since it + * returns 0 if mem_get_uint() fails. There is no proper handling for this case. */ static inline uint64_t mem_get_uint_unsafe(const struct Mem *mem) { uint64_t u; - mem_get_uint(mem, &u); + if (mem_get_uint(mem, &u) != 0) + return 0; return u; }