[Tarantool-patches] [PATCH v5 28/52] sql: introduce mem_copy_str() and mem_copy_str0()
Mergen Imeev
imeevma at tarantool.org
Wed Apr 14 01:00:25 MSK 2021
Thank you for the review! My answers and new patch below. There won't be diff
since I added mem_copy_strl() during merge caused by previous patch.
On Tue, Apr 13, 2021 at 01:35:19AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
>
> See 2 comments below.
>
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index 87e1bcfd1..045c44e8f 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -394,6 +394,34 @@ mem_set_str0_allocated(struct Mem *mem, char *value)
> > mem->flags |= MEM_Term;
> > }
> >
> > +int
> > +mem_copy_str(struct Mem *mem, const char *value, uint32_t len)
> > +{
> > + if ((mem->flags & (MEM_Agg | MEM_Frame)) != 0)
> > + mem_clear(mem);
>
> 1. Why don't you call clear always? Anyway 'dynamic' memory can't
> be reused. It is freed in sqlVdbeMemGrow() AFAIS.
>
It is possible that value given to mem_copy_*() is own value of the MEM. If it
is of MEM_Dyn type it will be lost in case of clear() being called. Function
sqlVdbeMemGrow() will free this memory after value will be copied.
> > + bool is_own_value = (mem->flags & (MEM_Str | MEM_Blob)) != 0 &&
> > + mem->z == value;
> > + if (sqlVdbeMemGrow(mem, len, is_own_value) != 0)
> > + return -1;
> > diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> > index e0903e3b0..ee095f36e 100644
> > --- a/src/box/sql/vdbeapi.c
> > +++ b/src/box/sql/vdbeapi.c
> > @@ -798,8 +803,14 @@ bindText(sql_stmt * pStmt, /* The statement to bind against */
> > mem_set_str0_dynamic(pVar, (char *)zData);
> > else
> > mem_set_str_dynamic(pVar, (char *)zData, nData);
> > - } else if (sqlVdbeMemSetStr(pVar, zData, nData, 1, xDel) != 0) {
> > - return -1;
> > + } else {
> > + if (nData < 0) {
> > + if (mem_copy_str0(pVar, zData) != 0)
> > + return -1;
> > + } else {
> > + if (mem_copy_str(pVar, zData, nData) != 0)
> > + return -1;
> > + }
>
> 2. For mem_set_str* and mem_copy_str* you could probably have 3
> versions: mem_set_strn(const char *, uint32 len),
> mem_set_str0(const char *), and mem_set_str(const char *, uint32 len_hint).
> In the last version len_hint might be -1 and then strlen() is called.
> It would keep the places like that as simple as they were.
>
Done, in previous patch and here. I named functions with len_hint as
mem_set_strl_*() and mem_copy_strl().
> > }
> > return sql_bind_type(p, i, "text");
> > }
> >
New patch:
commit 3e3f08f8c3bc2fb45a62c992cdf4f9ce20890641
Author: Mergen Imeev <imeevma at gmail.com>
Date: Mon Mar 15 16:59:27 2021 +0300
sql: introduce mem_copy_str() and mem_copy_str0()
This patch introduces mem_copy_str() and mem_copy_str0() functions.
These functions copy given string to newly allocated by MEM memory.
String given to mem_copy_str0() should be NULL-terminated. This is not
necessary for mem_copy_str().
Part of #5818
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 5cf067453..ded20315b 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -252,6 +252,34 @@ mem_set_str0_allocated(struct Mem *mem, char *value)
mem->flags |= MEM_Term;
}
+int
+mem_copy_str(struct Mem *mem, const char *value, uint32_t len)
+{
+ if ((mem->flags & (MEM_Agg | MEM_Frame)) != 0)
+ mem_clear(mem);
+ bool is_own_value = (mem->flags & (MEM_Str | MEM_Blob)) != 0 &&
+ mem->z == value;
+ if (sqlVdbeMemGrow(mem, len, is_own_value) != 0)
+ return -1;
+ if (!is_own_value)
+ memcpy(mem->z, value, len);
+ mem->n = len;
+ mem->flags = MEM_Str;
+ mem->field_type = FIELD_TYPE_STRING;
+ return 0;
+}
+
+int
+mem_copy_str0(struct Mem *mem, const char *value)
+{
+ uint32_t len = strlen(value);
+ if (mem_copy_str(mem, value, len + 1) != 0)
+ return -1;
+ mem->n = len;
+ mem->flags |= MEM_Term;
+ return 0;
+}
+
int
mem_copy(struct Mem *to, const struct Mem *from)
{
@@ -1441,8 +1469,8 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
assert(MEM_Str == (MEM_Blob >> 3));
if ((pMem->flags & MEM_Bool) != 0) {
const char *str_bool = SQL_TOKEN_BOOLEAN(pMem->u.b);
- sqlVdbeMemSetStr(pMem, str_bool, strlen(str_bool), 1,
- SQL_TRANSIENT);
+ if (mem_copy_str0(pMem, str_bool) != 0)
+ return -1;
return 0;
}
pMem->flags |= (pMem->flags & MEM_Blob) >> 3;
@@ -2903,9 +2931,8 @@ port_lua_get_vdbemem(struct port *base, uint32_t *size)
val[i].u.i = field.ival;
break;
case MP_STR:
- if (sqlVdbeMemSetStr(&val[i], field.sval.data,
- field.sval.len, 1,
- SQL_TRANSIENT) != 0)
+ if (mem_copy_str(&val[i], field.sval.data,
+ field.sval.len) != 0)
goto error;
break;
case MP_NIL:
@@ -2980,8 +3007,7 @@ port_c_get_vdbemem(struct port *base, uint32_t *size)
break;
case MP_STR:
str = mp_decode_str(&data, &len);
- if (sqlVdbeMemSetStr(&val[i], str, len,
- 1, SQL_TRANSIENT) != 0)
+ if (mem_copy_str(&val[i], str, len) != 0)
goto error;
break;
case MP_NIL:
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index f029ee0e5..aaecd064a 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -409,6 +409,25 @@ mem_set_strl_allocated(struct Mem *mem, char *value, int len_hint)
mem_set_str_allocated(mem, value, len_hint);
}
+/** Copy string to a newly allocated memory. The MEM type becomes STRING. */
+int
+mem_copy_str(struct Mem *mem, const char *value, uint32_t len);
+
+/**
+ * Copy NULL-terminated string to a newly allocated memory. The MEM type becomes
+ * STRING.
+ */
+int
+mem_copy_str0(struct Mem *mem, const char *value);
+
+static inline int
+mem_copy_strl(struct Mem *mem, const char *value, int len_hint)
+{
+ if (len_hint < 0)
+ return mem_copy_str0(mem, value);
+ return mem_copy_str(mem, value, len_hint);
+}
+
/**
* Copy content of MEM from one MEM to another. In case source MEM contains
* string or binary and allocation type is not STATIC, this value is copied to
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 484c66b29..69dd1fb0d 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -131,7 +131,7 @@ setResultStrOrError(sql_context * pCtx, /* Function context */
return mem_set_strl_allocated(pCtx->pOut, (char *)z, n);
if (xDel != SQL_TRANSIENT)
return mem_set_strl_dynamic(pCtx->pOut, (char *)z, n);
- if (sqlVdbeMemSetStr(pCtx->pOut, z, n, 1, xDel) != 0)
+ if (mem_copy_strl(pCtx->pOut, z, n) != 0)
pCtx->is_aborted = true;
}
@@ -774,7 +774,7 @@ bindText(sql_stmt * pStmt, /* The statement to bind against */
mem_set_strl_allocated(pVar, (char *)zData, nData);
else if (xDel != SQL_TRANSIENT)
mem_set_strl_dynamic(pVar, (char *)zData, nData);
- else if (sqlVdbeMemSetStr(pVar, zData, nData, 1, xDel) != 0)
+ else if (mem_copy_strl(pVar, zData, nData) != 0)
return -1;
return sql_bind_type(p, i, "text");
}
More information about the Tarantool-patches
mailing list