Re: [Tarantool-patches] [PATCH v5 28/52] sql: introduce mem_copy_str() and mem_copy_str0()

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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@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");
 }