Re: [Tarantool-patches] [PATCH 5/7] fiber: add dynamic option for parent backtrace

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

Thanks for the review!

On 05.07.2021 12:49, Cyrill Gorcunov via Tarantool-patches wrote:
> On Thu, Jul 01, 2021 at 11:24:43PM +0300, Egor Elchinov via Tarantool-patches wrote:
>> diff --git a/src/lua/fiber.c b/src/lua/fiber.c
>> index 026e30bc6..fe01ae23b 100644
>> --- a/src/lua/fiber.c
>> +++ b/src/lua/fiber.c
>> @@ -205,7 +205,7 @@ struct lua_parent_tb_ctx {
>>   	int tb_frame;
>>   };
>>   
>> -#ifdef ENABLE_BACKTRACE
>> +#if ENABLE_BACKTRACE
> 
> Please don't squash unrelated changes into the patch. If you prefer
> to shange #ifdef to #if better make it as a separate commit (though
> I don't understand yet what is wrong with existing code)
Changes rebased into refactoring commit.

I believe this is more conventional way to check the ENABLE_BACKTRACE 
option because it is defined as
#cmakedefine ENABLE_BACKTRACE 1
but not
#cmakedefine ENABLE_BACKTRACE

> 
>> @@ -446,15 +446,18 @@ lbox_fiber_statof_map(struct fiber *f, void *cb_ctx, bool backtrace)
>>   				  f != fiber() ? &f->ctx : NULL, &tb_ctx);
>>   		lua_settable(L, -3);
>>   
>> -		parent_tb_ctx.L = L;
>> -		parent_tb_ctx.bt = f->storage.lua.parent_bt;
>> -		parent_tb_ctx.tb_frame = 0;
>> -		lua_pushstring(L, "backtrace_parent");
>> -		lua_newtable(L);
>> -		backtrace_foreach_ip(fiber_parent_backtrace_cb,
>> -				     f->parent_bt_ip_buf,
>> -				     FIBER_PARENT_BT_MAX, &parent_tb_ctx);
>> -		lua_settable(L, -3);
>> +		if (fiber_parent_bt_is_enabled()) {
>> +			parent_tb_ctx.L = L;
>> +			parent_tb_ctx.bt = f->storage.lua.parent_bt;
>> +			parent_tb_ctx.tb_frame = 0;
>> +			lua_pushstring(L, "backtrace_parent");
>> +			lua_newtable(L);
>> +			backtrace_foreach_ip(fiber_parent_backtrace_cb,
>> +					     f->parent_bt_ip_buf,
>> +					     FIBER_PARENT_BT_MAX,
>> +					     &parent_tb_ctx);
>> +			lua_settable(L, -3);
>> +		}
> 
> Maybe it would worth to merge fiber_parent_bt_is_enabled option immediately
> into previous patch?
> 
Merged.