[Tarantool-patches] [PATCH v5 46/52] sql: introduce mem_get_uint()
Mergen Imeev
imeevma at tarantool.org
Wed Apr 14 04:21:43 MSK 2021
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 <imeevma at gmail.com>
> 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;
}
More information about the Tarantool-patches
mailing list