From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 B7271469719 for ; Thu, 19 Mar 2020 02:57:49 +0300 (MSK) Date: Wed, 18 Mar 2020 23:57:48 +0000 From: Nikita Pettik Message-ID: <20200318235748.GA9035@tarantool.org> References: <7720185c4456a63f0d31bfbe11cdb101e864a3c4.1584284911.git.v.shpilevoy@tarantool.org> <3dfcb826-c92d-7458-0f5f-3d25d3c9e748@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <3dfcb826-c92d-7458-0f5f-3d25d3c9e748@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org On 18 Mar 22:35, Vladislav Shpilevoy wrote: > Nikita, please, do a second review. I'm okay with this patch (https://lists.tarantool.org/pipermail/tarantool-patches/2020-March/014824.html) Will push tomorrow if nobody has objections. > On 15/03/2020 16:11, Vladislav Shpilevoy wrote: > > 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. > > --- > > Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4394-fiber-name-inline > > Issue: https://github.com/tarantool/tarantool/issues/4394 > > > > Changes in v2: > > - Short fiber names are kept inside struct fiber. > > > > src/lib/core/fiber.c | 28 ++++++++++++++++++++++++++-- > > src/lib/core/fiber.h | 14 +++++++++++--- > > test/app/fiber.result | 8 ++++---- > > test/app/fiber.test.lua | 2 +- > > test/unit/fiber.cc | 6 +++--- > > test/unit/fiber.result | 2 +- > > 6 files changed, 46 insertions(+), 14 deletions(-) > > > > diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c > > index ada7972cb..359ef1187 100644 > > --- a/src/lib/core/fiber.c > > +++ b/src/lib/core/fiber.c > > @@ -912,8 +912,26 @@ 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 len = strlen(name); > > + if (len <= 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; > > + char *new_name; > > + if (fiber->name != fiber->inline_name) > > + new_name = realloc(fiber->name, len + 1); > > + else > > + new_name = malloc(len + 1); > > + if (new_name == NULL) > > + panic("fiber_set_name() failed with OOM"); > > + fiber->name = new_name; > > + } > > + memcpy(fiber->name, name, len); > > + fiber->name[len] = 0; > > } > > > > static inline void * > > @@ -1242,6 +1260,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 +1378,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 +1427,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..fedc37c87 100644 > > --- a/src/lib/core/fiber.h > > +++ b/src/lib/core/fiber.h > > @@ -111,7 +111,10 @@ struct cpu_stat { > > > > #endif /* ENABLE_FIBER_TOP */ > > > > -enum { FIBER_NAME_MAX = 32 }; > > +enum { > > + FIBER_NAME_INLINE = 32, > > + FIBER_NAME_MAX = 255 > > +}; > > > > /** > > * Fiber ids [0; 100] are reserved. > > @@ -510,7 +513,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 + 1]; > > }; > > > > /** Invoke on_stop triggers and delete them. */ > > @@ -578,7 +586,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 + 1]; > > }; > > > > extern __thread struct cord *cord_ptr; > > 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 *** > >