From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 89F2E6E459; Thu, 18 Nov 2021 02:03:34 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 89F2E6E459 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1637190214; bh=G6AUAeQL8IVJD/oTI8QfXjBxz4zhMM5H7vgAnuW+9aA=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=h35blPYxlul1JRyxqzdHnPw/vGCfBRO6Ae0bhLDIM3zr5j+IGr18BUj3ibBO0EaNA FRIWPnQKJwx5J7MMzyRF0lhu5noPL6dyTtG88R0YU/ATylBzQTdLuuyCNuRs/LLCxO FKqx6nuMaEBID3lLUa4yS40KyzLJFUjIV7yh+Qfs= Received: from smtpng3.i.mail.ru (smtpng3.i.mail.ru [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 07BD16E459 for ; Thu, 18 Nov 2021 02:03:31 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 07BD16E459 Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1mnTxj-00086m-3C; Thu, 18 Nov 2021 02:03:31 +0300 Message-ID: <8e6f11df-1fae-8b02-b7d6-34165e35e707@tarantool.org> Date: Thu, 18 Nov 2021 00:03:18 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.3.1 Content-Language: en-US To: imeevma@tarantool.org Cc: tarantool-patches@dev.tarantool.org References: In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9731B3922EC0639792B05E85F1235528D4452084448829B2800894C459B0CD1B99F1CDA2210E2ECB28C40F0547AC73D3BDA8FB3F14522F0916EC242FFF24CD075 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F8E53417176C7207EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637F72512A705E00D288638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D81386074E95ACF6E66E08828C6859514E117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18CB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B613439FA09F3DCB32089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B505C4815367AB30B837388CD31B8A08E5F2 X-C1DE0DAB: 0D63561A33F958A51F641DF5E1DB71523C89AA6631AC9FE940A43B82D2D2C72BD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7506FE1F977233B9BB410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34C1D376EF32BB0896DF79732A069C8436871EBE928A03EDA4431404BBEEA117EA9D04962520D09F981D7E09C32AA3244CF36C954F8D9EFE490886AE7688BAC41595A9E0DC41E9A4CF729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj81eoY04PbomQ+Eo7/7ik+g== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D0EDFFEEDBFAC3EE0BBB5795B5098CA7B3841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce mem_snprintf() X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thanks for the patch! See 5 comments below. > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index 5abaf490d..0f8f0b5cc 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > @@ -869,9 +869,14 @@ func_printf(struct sql_context *ctx, int argc, const struct Mem *argv) > if (argc < 1 || mem_is_null(&argv[0])) > return; > if (argc == 1 || !mem_is_str(&argv[0])) { > - struct Mem *mem = ctx->pOut; > - if (mem_copy(mem, &argv[0]) != 0 || mem_to_str(mem) != 0) > + uint32_t size = mem_snprintf(NULL, 0, &argv[0]); 1. You are not checking for an error here. mem_snprintf() returns int, not uint. I am fine with making it never failing, but then we need to at least add some assertions inside of mem_snprintf() that its internal snprintf() calls never fail. We can keep returning 'int' though because it makes the function compatible with SNPRINT macro. > + char *str = sqlDbMallocRawNN(sql_get(), size + 1); > + if (str == NULL) { > ctx->is_aborted = true; > + return; > + } > + mem_snprintf(str, size + 1, &argv[0]); > + mem_set_str_allocated(ctx->pOut, str, size); > return; > } > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > index 9ddeea5bb..6e1729f32 100644 > --- a/src/box/sql/mem.c > +++ b/src/box/sql/mem.c > @@ -116,30 +116,51 @@ mem_is_field_compatible(const struct Mem *mem, enum field_type type) > return field_mp_plain_type_is_compatible(type, mp_type, true); > } > > -const char * > -mem_str(const struct Mem *mem) > +int > +mem_snprintf(char *buf, uint32_t size, const struct Mem *mem) > { > - char buf[STR_VALUE_MAX_LEN]; > - const char *type = mem_type_to_str(mem); > switch (mem->type) { > case MEM_TYPE_NULL: > - return "NULL"; > + return snprintf(buf, size, "NULL"); > case MEM_TYPE_STR: > + case MEM_TYPE_BIN: > + return snprintf(buf, size, "%.*s", mem->n, mem->z); 2. Why is bin displayed as hex in mem_str() but as a plain string here? > + case MEM_TYPE_INT: > + return snprintf(buf, size, "%lld", mem->u.i); > + case MEM_TYPE_UINT: > + return snprintf(buf, size, "%llu", > + (unsigned long long)mem->u.u); > + case MEM_TYPE_DOUBLE: { > + char str[BUF_SIZE]; > + sql_snprintf(BUF_SIZE, str, "%!.15g", mem->u.r); > + return snprintf(buf, size, "%s", str); > + } > + case MEM_TYPE_DEC: > + return snprintf(buf, size, "%s", decimal_str(&mem->u.d)); > + case MEM_TYPE_MAP: > + case MEM_TYPE_ARRAY: > + return mp_snprint(buf, size, mem->z); > + case MEM_TYPE_UUID: > + return snprintf(buf, size, "%s", tt_uuid_str(&mem->u.uuid)); > + case MEM_TYPE_BOOL: > + return snprintf(buf, size, "%s", mem->u.b ? "TRUE" : "FALSE"); 3. You can drop '%s' here: return snprintf(buf, size, mem->u.b ? "TRUE" : "FALSE"); > + default: > + return snprintf(buf, size, "unknown"); 4. Maybe add an assertion that it is not possible? We don't have non-typed mems AFAIK. > + } > +} > diff --git a/src/box/sql/printf.c b/src/box/sql/printf.c > index 8da7c9878..08e18c15d 100644 > --- a/src/box/sql/printf.c > +++ b/src/box/sql/printf.c > @@ -160,19 +160,12 @@ getTextArg(PrintfArguments * p) > { > if (p->nArg <= p->nUsed) > return 0; > - struct Mem mem; > - mem_create(&mem); > - mem_copy_as_ephemeral(&mem, &p->apArg[p->nUsed++]); > - if (mem_to_str(&mem) != 0) { > - mem_destroy(&mem); > - return NULL; > - } > - char *str = sqlDbMallocRawNN(sql_get(), mem.n + 1); > + const struct Mem *mem = &p->apArg[p->nUsed++]; > + uint32_t size = mem_snprintf(NULL, 0, mem); > + char *str = sqlDbMallocRawNN(sql_get(), size + 1); > if (str == NULL) > return NULL; > - memcpy(str, mem.z, mem.n); > - str[mem.n] = '\0'; > - mem_destroy(&mem); > + mem_snprintf(str, size + 1, mem); 5. Hm. I suspect this pattern of snprintf + malloc + snprintf is going to be frequent. Maybe wrap it into a function like mem_strdup()? It would do these 3 actions inside. WDYT?