[tarantool-patches] Re: [PATCH v2 3/7] sql: refactor sql_src_list_append to set diag

[Kirill Shcherbatov]

> 1. Stray empty line. Please, do self-review before sending a patch.
> Such a stray change says that you did not scan the patch before a
> send.
Fixed.

> 2. This should be said in @retval sections.
> 
>> + * The returned SrcList might be the same as the list that was
>> + * input or it might be a new one. If an OOM error does occurs,
>> + * then the prior value of list that is input to this routine is
>> + * automatically freed.
>> + *
>> + * @param db The database connection.
>> + * @param list Append to this SrcList. NULL creates a new SrcList.
>> + * @param name_token Token representing table name.
>> + * @retval Not NULL SrcList pointer on success.
>> + * @retval NULL Otherwise. The diag message is set.
>> + */
>> +struct SrcList *
>> +sql_src_list_append(struct sql *db, struct SrcList *list,
>> +		    struct Token *name_token);
/**
 * Append a new table name to the given list. Create a new
 * SrcList if need be. A new entry is created in the list even
 * if name_token is NULL.
 *
 * @param db The database connection.
 * @param list Append to this SrcList. NULL creates a new SrcList.
 * @param name_token Token representing table name.
 * @retval Not NULL SrcList pointer is returned. The returned
 *         SrcList might be the same as the list that was input
 *         or it might be a new one.
 * @retval NULL Otherwise. The diag message is set. The prior
 *         value of list that is input to this routine is
 *         automatically freed.
 */
===================================================

Refactored sqlIdListAppend routine as sql_id_list_append and
reworked it to use diag_set in case of memory allocation error.
This change is necessary because the sql_id_list_append body has
a sqlNameFromToken call that will be changed in subsequent
patches.

This patch refers to a series of preparatory patches that provide
the use of Tarantool errors in the call tree that includes
sqlNormalizeName, since this call can later return errors.

This patch is not self-sufficient, its sqlNameFromToken call
remained to be non-Tarantool (for now). It means, that if
sqlNameFromToken fails in sql_id_list_append there is no
diag message created.

Part of #3931
---
 src/box/sql/build.c  | 44 +++++++++++++++++++++-----------------------
 src/box/sql/expr.c   |  8 +++++---
 src/box/sql/parse.y  | 19 +++++++++++++++----
 src/box/sql/sqlInt.h | 16 +++++++++++++++-
 4 files changed, 56 insertions(+), 31 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index ef38503da..dae582d1f 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2516,30 +2516,28 @@ sqlArrayAllocate(sql * db,	/* Connection to notify of malloc failures */
 	return pArray;
 }
 
-/*
- * Append a new element to the given IdList.  Create a new IdList if
- * need be.
- *
- * A new IdList is returned, or NULL if malloc() fails.
- */
-IdList *
-sqlIdListAppend(sql * db, IdList * pList, Token * pToken)
-{
-	int i;
-	if (pList == 0) {
-		pList = sqlDbMallocZero(db, sizeof(IdList));
-		if (pList == 0)
-			return 0;
-	}
-	pList->a = sqlArrayAllocate(db,
-					pList->a,
-					sizeof(pList->a[0]), &pList->nId, &i);
-	if (i < 0) {
-		sqlIdListDelete(db, pList);
-		return 0;
+struct IdList *
+sql_id_list_append(struct sql *db, struct IdList *list,
+		   struct Token *name_token)
+{
+	if (list == NULL &&
+	    (list = sqlDbMallocZero(db, sizeof(struct IdList))) == NULL) {
+		diag_set(OutOfMemory, sizeof(struct IdList), "sqlDbMallocZero",
+			 "list");
+		return NULL;
 	}
-	pList->a[i].zName = sqlNameFromToken(db, pToken);
-	return pList;
+	int i;
+	list->a = sqlArrayAllocate(db, list->a, sizeof(list->a[0]),
+				   &list->nId, &i);
+	if (i < 0)
+		goto error;
+	list->a[i].zName = sqlNameFromToken(db, name_token);
+	if (list->a[i].zName == NULL)
+		goto error;
+	return list;
+error:
+	sqlIdListDelete(db, list);
+	return NULL;
 }
 
 /*
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index a2c70935e..0457ff833 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -1658,9 +1658,11 @@ sqlIdListDup(sql * db, IdList * p)
 		sqlDbFree(db, pNew);
 		return 0;
 	}
-	/* Note that because the size of the allocation for p->a[] is not
-	 * necessarily a power of two, sqlIdListAppend() may not be called
-	 * on the duplicate created by this function.
+	/*
+	 * Note that because the size of the allocation for p->a[]
+	 * is not necessarily a power of two, sql_id_list_append()
+	 * may not be called on the duplicate created by this
+	 * function.
 	 */
 	for (i = 0; i < p->nId; i++) {
 		struct IdList_item *pNewItem = &pNew->a[i];
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index ead71dfc0..daeb6e84b 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -804,10 +804,21 @@ insert_cmd(A) ::= REPLACE.            {A = ON_CONFLICT_ACTION_REPLACE;}
 
 idlist_opt(A) ::= .                       {A = 0;}
 idlist_opt(A) ::= LP idlist(X) RP.    {A = X;}
-idlist(A) ::= idlist(A) COMMA nm(Y).
-    {A = sqlIdListAppend(pParse->db,A,&Y);}
-idlist(A) ::= nm(Y).
-    {A = sqlIdListAppend(pParse->db,0,&Y); /*A-overwrites-Y*/}
+idlist(A) ::= idlist(A) COMMA nm(Y). {
+  A = sql_id_list_append(pParse->db,A,&Y);
+  if (A == NULL) {
+    pParse->is_aborted = true;
+    return;
+  }
+}
+idlist(A) ::= nm(Y). {
+  /* A-overwrites-Y. */
+  A = sql_id_list_append(pParse->db,0,&Y);
+  if (A == NULL) {
+    pParse->is_aborted = true;
+    return;
+  }
+}
 
 /////////////////////////// Expression Processing /////////////////////////////
 //
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index b6c89893a..8f56f3e63 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3411,7 +3411,21 @@ sql_drop_table(struct Parse *, struct SrcList *, bool, bool);
 void sqlInsert(Parse *, SrcList *, Select *, IdList *,
 	       enum on_conflict_action);
 void *sqlArrayAllocate(sql *, void *, int, int *, int *);
-IdList *sqlIdListAppend(sql *, IdList *, Token *);
+
+/**
+ * Append a new element to the given IdList. Create a new IdList
+ * if need be.
+ *
+ * @param db The database connection.
+ * @param list The pointer to existent Id list if exists.
+ * @param name_token The token containing name.
+ * @retval Not NULL IdList pointer is returned on success.
+ * @retval NULL Otherwise. Diag message is set.
+ */
+struct IdList *
+sql_id_list_append(struct sql *db, struct IdList *list,
+		   struct Token *name_token);
+
 int sqlIdListIndex(IdList *, const char *);
 
 /**
-- 
2.21.0