Re: [Tarantool-patches] [PATCH v2 2/4] fiber: add option and PoC for Lua parent backtrace

[Egor Elchinov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On 12.07.2021 15:09, Cyrill Gorcunov via Tarantool-patches wrote:
> On Fri, Jul 09, 2021 at 02:03:51PM +0300, Egor Elchinov via Tarantool-patches wrote:
>>
>> diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
>> index 924ff3c82..dd26e2f13 100644
>> --- a/src/lib/core/fiber.c
>> +++ b/src/lib/core/fiber.c
>> @@ -220,6 +220,10 @@ fiber_mprotect(void *addr, size_t len, int prot)
>>   static __thread bool fiber_top_enabled = false;
>>   #endif /* ENABLE_FIBER_TOP */
>>   
>> +#if ENABLE_BACKTRACE
>> +static __thread bool fiber_parent_bt_enabled = false;
>> +#endif /* ENABLE_BACKTRACE */
>> +
> 
> So this variable is declared as TLS in a manner of fiber_top
> functionality. You know I somehow find this fishy because
> it is unclear why it is done this way. I think we should
> revisit this moment for @fiber_top_enabled enabled as well
> but not in this series, so lets leave it as is.
> 
> Please revisit snprintf calls in the patch and strncpy
> as well because they _do_not_ append terminating \0 symbol
> if destination size is not enough. I think we should add
> \0 explicitly all the time.
Thanks, fixed hereinafter.
> 
> I mean
> 
> +static int
> +fiber_parent_backtrace_cb(int frameno, void *frameret, const char *func,
> +			  size_t offset, void *cb_ctx)
> +{
> 	...
> +	char buf[512];
> +	int l = snprintf(buf, sizeof(buf), "#%-2d %p in ", frameno, frameret);
> +	if (func)
> +		snprintf(buf + l, sizeof(buf) - l, "%s+%zu", func, offset);
> +	else
> +		snprintf(buf + l, sizeof(buf) - l, "?");
> 
> we could add
> 
> 	buf[sizeof(buf)-1] = '\0';
> 
> Actually 512 bytes for function name should be more than enough but
> better be on a safe side.
> 
> Other than that the patch looks ok to me, great job, thanks!
>