From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 4175D2A507 for ; Wed, 20 Mar 2019 07:02:50 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zgYewnfePKIz for ; Wed, 20 Mar 2019 07:02:50 -0400 (EDT) Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id DE9BE2A504 for ; Wed, 20 Mar 2019 07:02:49 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 3/7] sql: refactor sql_src_list_append to set diag References: <390fc2f46ba80994aa4ebf97d3aeb8ea8c26c71c.1551265819.git.kshcherbatov@tarantool.org> <4dcd389e-2388-d145-a82c-2cabc2b9ed15@tarantool.org> <8743fe4b-6485-9b72-dd40-e4d2ecab5801@tarantool.org> <2c6214c0-dc40-bb33-283b-781234ad3360@tarantool.org> From: Kirill Shcherbatov Message-ID: Date: Wed, 20 Mar 2019 14:02:47 +0300 MIME-Version: 1.0 In-Reply-To: <2c6214c0-dc40-bb33-283b-781234ad3360@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, Vladislav Shpilevoy > 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