From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 9BA91469719 for ; Tue, 17 Mar 2020 01:28:47 +0300 (MSK) References: <7720185c4456a63f0d31bfbe11cdb101e864a3c4.1584284911.git.v.shpilevoy@tarantool.org> <20200316071927.GZ27301@uranus> From: Vladislav Shpilevoy Message-ID: Date: Mon, 16 Mar 2020 23:28:46 +0100 MIME-Version: 1.0 In-Reply-To: <20200316071927.GZ27301@uranus> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 1/1] fiber: extend max fiber name length to 255 List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review! > Thank, Vlad! I like the patch. There is only one concern I have: for > some reason we has been defining faiber name as > > char name[FIBER_NAME_MAX + 1]; > > where FIBER_NAME_MAX = 32 and finally this expands to "char name[33];" > I'm too lazy to find who exactly introduced this but it is bloody > wrong: compiler alings members to eliminate data access penalty, thus > _actually_ it will be defined as 8 multilier, ie 40 bytes. > > Thus, if you don't mind I propose make FIBER_NAME_INLINE = 40 > *including* string terminating zero. > > Actually we can make it on top then. Up to you. Anyway > > Reviewed-by: Cyrill Gorcunov Yes, good point. Here is diff. The whole patch is in the bottom. ==================== diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c index 359ef1187..5389ce467 100644 --- a/src/lib/core/fiber.c +++ b/src/lib/core/fiber.c @@ -912,26 +912,27 @@ fiber_loop(MAYBE_UNUSED void *data) void fiber_set_name(struct fiber *fiber, const char *name) { - size_t len = strlen(name); - if (len <= FIBER_NAME_INLINE) { + size_t size = strlen(name) + 1; + if (size <= FIBER_NAME_INLINE) { if (fiber->name != fiber->inline_name) { free(fiber->name); fiber->name = fiber->inline_name; } } else { - if (len > FIBER_NAME_MAX) - len = FIBER_NAME_MAX; + if (size > FIBER_NAME_MAX) + size = FIBER_NAME_MAX; char *new_name; if (fiber->name != fiber->inline_name) - new_name = realloc(fiber->name, len + 1); + new_name = realloc(fiber->name, size); else - new_name = malloc(len + 1); + new_name = malloc(size); if (new_name == NULL) panic("fiber_set_name() failed with OOM"); fiber->name = new_name; } - memcpy(fiber->name, name, len); - fiber->name[len] = 0; + --size; + memcpy(fiber->name, name, size); + fiber->name[size] = 0; } static inline void * diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h index fedc37c87..cd9346a55 100644 --- a/src/lib/core/fiber.h +++ b/src/lib/core/fiber.h @@ -112,8 +112,9 @@ struct cpu_stat { #endif /* ENABLE_FIBER_TOP */ enum { - FIBER_NAME_INLINE = 32, - FIBER_NAME_MAX = 255 + /** Both limits include terminating 0. */ + FIBER_NAME_INLINE = 40, + FIBER_NAME_MAX = 256 }; /** @@ -518,7 +519,7 @@ struct fiber { * name is allocated on the heap. */ char *name; - char inline_name[FIBER_NAME_INLINE + 1]; + char inline_name[FIBER_NAME_INLINE]; }; /** Invoke on_stop triggers and delete them. */ @@ -586,7 +587,7 @@ struct cord { struct slab_cache slabc; /** The "main" fiber of this cord, the scheduler. */ struct fiber sched; - char name[FIBER_NAME_INLINE + 1]; + char name[FIBER_NAME_INLINE]; }; extern __thread struct cord *cord_ptr; diff --git a/src/lua/fiber.c b/src/lua/fiber.c index 575a020d0..330eb7e54 100644 --- a/src/lua/fiber.c +++ b/src/lua/fiber.c @@ -582,16 +582,16 @@ lbox_fiber_name(struct lua_State *L) if (top == name_index || top == opts_index) { /* Set name. */ const char *name = luaL_checkstring(L, name_index); - int name_len = strlen(name); + int name_size = strlen(name) + 1; if (top == opts_index && lua_istable(L, opts_index)) { lua_getfield(L, opts_index, "truncate"); /* Truncate the name if needed. */ if (lua_isboolean(L, -1) && lua_toboolean(L, -1) && - name_len > FIBER_NAME_MAX) - name_len = FIBER_NAME_MAX; + name_size > FIBER_NAME_MAX) + name_size = FIBER_NAME_MAX; lua_pop(L, 1); } - if (name_len > FIBER_NAME_MAX) + if (name_size > FIBER_NAME_MAX) luaL_error(L, "Fiber name is too long"); fiber_set_name(f, name); return 0; ==================== The whole patch: ==================== fiber: extend max fiber name length to 255 Users keep complaining about too short fiber name. New limit is 255, should be enough for any sane name. Closes #4394 @TarantoolBot document Title: fiber.name length limit. It was 32, now it is 255. Besides, it seems like `fiber.name` `{truncate = true}` option is not documented. By default, if a new name is too long, `fiber.name(new_name)` fails with an exception. To make it always succeed there is an option 'truncate': `fiber.name(new_name, {truncate = true})`. It truncates the name to the max length if it is too long. diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c index ada7972cb..5389ce467 100644 --- a/src/lib/core/fiber.c +++ b/src/lib/core/fiber.c @@ -912,8 +912,27 @@ fiber_loop(MAYBE_UNUSED void *data) void fiber_set_name(struct fiber *fiber, const char *name) { - assert(name != NULL); - snprintf(fiber->name, sizeof(fiber->name), "%s", name); + size_t size = strlen(name) + 1; + if (size <= FIBER_NAME_INLINE) { + if (fiber->name != fiber->inline_name) { + free(fiber->name); + fiber->name = fiber->inline_name; + } + } else { + if (size > FIBER_NAME_MAX) + size = FIBER_NAME_MAX; + char *new_name; + if (fiber->name != fiber->inline_name) + new_name = realloc(fiber->name, size); + else + new_name = malloc(size); + if (new_name == NULL) + panic("fiber_set_name() failed with OOM"); + fiber->name = new_name; + } + --size; + memcpy(fiber->name, name, size); + fiber->name[size] = 0; } static inline void * @@ -1242,6 +1261,9 @@ fiber_destroy(struct cord *cord, struct fiber *f) region_destroy(&f->gc); fiber_stack_destroy(f, &cord->slabc); diag_destroy(&f->diag); + if (f->name != f->inline_name) + free(f->name); + f->name = NULL; } void @@ -1357,6 +1379,7 @@ cord_create(struct cord *cord, const char *name) fiber_reset(&cord->sched); diag_create(&cord->sched.diag); region_create(&cord->sched.gc, &cord->slabc); + cord->sched.name = NULL; fiber_set_name(&cord->sched, "sched"); cord->fiber = &cord->sched; @@ -1405,6 +1428,8 @@ cord_destroy(struct cord *cord) } region_destroy(&cord->sched.gc); diag_destroy(&cord->sched.diag); + if (cord->sched.name != cord->sched.inline_name) + free(cord->sched.name); slab_cache_destroy(&cord->slabc); } diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h index 8a3e5aef5..cd9346a55 100644 --- a/src/lib/core/fiber.h +++ b/src/lib/core/fiber.h @@ -111,7 +111,11 @@ struct cpu_stat { #endif /* ENABLE_FIBER_TOP */ -enum { FIBER_NAME_MAX = 32 }; +enum { + /** Both limits include terminating 0. */ + FIBER_NAME_INLINE = 40, + FIBER_NAME_MAX = 256 +}; /** * Fiber ids [0; 100] are reserved. @@ -510,7 +514,12 @@ struct fiber { struct ipc_wait_pad *wait_pad; /** Exception which caused this fiber's death. */ struct diag diag; - char name[FIBER_NAME_MAX + 1]; + /** + * Name points at inline_name in case it is short. Long + * name is allocated on the heap. + */ + char *name; + char inline_name[FIBER_NAME_INLINE]; }; /** Invoke on_stop triggers and delete them. */ @@ -578,7 +587,7 @@ struct cord { struct slab_cache slabc; /** The "main" fiber of this cord, the scheduler. */ struct fiber sched; - char name[FIBER_NAME_MAX]; + char name[FIBER_NAME_INLINE]; }; extern __thread struct cord *cord_ptr; diff --git a/src/lua/fiber.c b/src/lua/fiber.c index 575a020d0..330eb7e54 100644 --- a/src/lua/fiber.c +++ b/src/lua/fiber.c @@ -582,16 +582,16 @@ lbox_fiber_name(struct lua_State *L) if (top == name_index || top == opts_index) { /* Set name. */ const char *name = luaL_checkstring(L, name_index); - int name_len = strlen(name); + int name_size = strlen(name) + 1; if (top == opts_index && lua_istable(L, opts_index)) { lua_getfield(L, opts_index, "truncate"); /* Truncate the name if needed. */ if (lua_isboolean(L, -1) && lua_toboolean(L, -1) && - name_len > FIBER_NAME_MAX) - name_len = FIBER_NAME_MAX; + name_size > FIBER_NAME_MAX) + name_size = FIBER_NAME_MAX; lua_pop(L, 1); } - if (name_len > FIBER_NAME_MAX) + if (name_size > FIBER_NAME_MAX) luaL_error(L, "Fiber name is too long"); fiber_set_name(f, name); return 0; diff --git a/test/app/fiber.result b/test/app/fiber.result index 6d9604ad8..7331f61b3 100644 --- a/test/app/fiber.result +++ b/test/app/fiber.result @@ -1186,7 +1186,7 @@ fiber = nil --- ... -- --- gh-2622, gh-4011: fiber.name() truncates new name. +-- gh-2622, gh-4011, gh-4394: fiber.name() truncates new name. -- fiber = require('fiber') --- @@ -1214,14 +1214,14 @@ fiber.name(long_name, {truncate = true}) ... fiber.name() --- -- aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +- aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ... f = fiber.self() --- ... fiber.name(f) --- -- aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +- aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ... fiber.name(f, 'new_name') --- @@ -1239,7 +1239,7 @@ fiber.name(f, long_name, {truncate = true}) ... fiber.name(f) --- -- aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +- aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa ... -- -- gh-3493 fiber.create() does not roll back memtx transaction diff --git a/test/app/fiber.test.lua b/test/app/fiber.test.lua index 6df210d9c..b8e9abc6e 100644 --- a/test/app/fiber.test.lua +++ b/test/app/fiber.test.lua @@ -504,7 +504,7 @@ test_run:cmd("setopt delimiter ''"); fiber = nil -- --- gh-2622, gh-4011: fiber.name() truncates new name. +-- gh-2622, gh-4011, gh-4394: fiber.name() truncates new name. -- fiber = require('fiber') long_name = string.rep('a', 300) diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc index 91f7d43f9..b0dfc9b14 100644 --- a/test/unit/fiber.cc +++ b/test/unit/fiber.cc @@ -171,9 +171,9 @@ fiber_name_test() note("set new fiber name: %s.\n", fiber_name(fiber())); - const char *long_name = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" - "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"\ - "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + char long_name[FIBER_NAME_MAX + 30]; + memset(long_name, 'a', sizeof(long_name)); + long_name[sizeof(long_name) - 1] = 0; fiber_set_name(fiber(), long_name); diff --git a/test/unit/fiber.result b/test/unit/fiber.result index 7c9f85dcd..a61e0a2b8 100644 --- a/test/unit/fiber.result +++ b/test/unit/fiber.result @@ -5,7 +5,7 @@ SystemError Failed to allocate 42 bytes in allocator for exception: Cannot alloc # set new fiber name: Horace. -# fiber name is truncated: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa. +# fiber name is truncated: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa. *** fiber_name_test: done *** *** fiber_join_test ***