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 A0AF0293F1 for ; Thu, 7 Mar 2019 12:34:28 -0500 (EST) 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 APvgboFB0oan for ; Thu, 7 Mar 2019 12:34:28 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 5847E293B9 for ; Thu, 7 Mar 2019 12:34:28 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v2 4/7] sql: refactor sql_name_from_token to set diag References: <97dde83dd0ad392cfe47a315ce7ad931cb750a4f.1551265819.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 7 Mar 2019 20:34:26 +0300 MIME-Version: 1.0 In-Reply-To: <97dde83dd0ad392cfe47a315ce7ad931cb750a4f.1551265819.git.kshcherbatov@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed 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, Kirill Shcherbatov See 5 comments. On 27/02/2019 14:13, Kirill Shcherbatov wrote: > Refactored sql_name_from_token routine to use diag_set in case > of memory allocation error. > This change is necessary because the sql_name_from_token body has > a sqlNameFromToken call that will be changed in subsequent > patches. > > Needed for #3931 > --- > src/box/sql/alter.c | 8 +- > src/box/sql/analyze.c | 47 ++++---- > src/box/sql/build.c | 241 +++++++++++++++++++++++------------------- > src/box/sql/expr.c | 2 +- > src/box/sql/parse.y | 15 ++- > src/box/sql/pragma.c | 24 +++-- > src/box/sql/sqlInt.h | 35 +++++- > src/box/sql/trigger.c | 4 +- > 8 files changed, 228 insertions(+), 148 deletions(-) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 4fe838608..2084dbfeb 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -332,7 +320,10 @@ sqlStartTable(Parse *pParse, Token *pName, int noErr) > goto cleanup; > sqlVdbeCountChanges(v); > > - zName = sqlNameFromToken(db, pName); > + assert(pName != NULL); 1. You already have the same assertion in sql_name_from_token. Why do you need so many new equal assertions? Check other places like build.c:1792 by yourself, please. > + zName = sql_name_from_token(db, pName); > + if (zName == NULL) > + goto tnt_error; > > pParse->sNameToken = *pName; > if (zName == 0) 2. Double check for zName == NULL. > diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c > index a7224f5c2..1e4ba3518 100644 > --- a/src/box/sql/pragma.c > +++ b/src/box/sql/pragma.c > @@ -452,19 +452,27 @@ sqlPragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ > sqlVdbeRunOnlyOnce(v); > pParse->nMem = 2; > > - zLeft = sqlNameFromToken(db, pId); > - if (!zLeft) { > + if (pId == NULL) { > printActivePragmas(user_session); > return; > } > - > + zLeft = sql_name_from_token(db, pId); > + if (zLeft == NULL) > + goto tnt_error; > if (minusFlag) { > zRight = sqlMPrintf(db, "-%T", pValue); > } else { > - zRight = sqlNameFromToken(db, pValue); > + if (pValue != NULL) { 3. You can make it shorter writing } else if (pValue != NULL) { ... } Instead of } else { if (pValue != NULL) { ... } } > + zRight = sql_name_from_token(db, pValue); > + if (zRight == NULL) > + goto tnt_error; > + } > + } > + if (pValue2 != NULL) { > + zTable = sql_name_from_token(db, pValue2); > + if (zTable == NULL) > + goto tnt_error; > } > - zTable = sqlNameFromToken(db, pValue2); > - > /* Locate the pragma in the lookup table */ > pPragma = pragmaLocate(zLeft); > if (pPragma == 0) { > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index eae2ec8e8..10943d3c8 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -3424,7 +3424,20 @@ 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. > + * A new IdList is returned, or NULL if malloc() fails. 4. The last sentence is exactly what you wrote after @retvals. Please, do not duplicate it. > + * @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. 5. Sentences are started from capital letters. I will not write that again. Please, find and fix other places. > + */ > +struct IdList * > +sql_id_list_append(struct sql *db, struct IdList *pList, struct Token *pToken); > + > int sqlIdListIndex(IdList *, const char *); > > /**