Re: [Tarantool-patches] [PATCH v2 1/1] fiber: extend max fiber name length to 255

[Nikita Pettik <korablev@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 ***
> >