Repetitive AppID lookups in CFE_SB_SendMsgFull

[jphickey]

Describe the bug
SB has some code to filter an app's own messages via CFE_SB_PIPEOPTS_IGNOREMINE. This gets the appid and compares to the AppID of the pipe creator, and skips the destination if its a match:

cFE/fsw/cfe-core/src/sb/cfe_sb_api.c

Lines 1359 to 1369 in 8a7dc8f

	if(PipeDscPtr->Opts & CFE_SB_PIPEOPTS_IGNOREMINE)
	{
	CFE_ES_ResourceID_t AppId;
	CFE_ES_GetAppID(&AppId);
	if( CFE_ES_ResourceID_Equal(PipeDscPtr->AppId, AppId) )
	{
	continue;
	}
	}/* end if */

The problem is that this is "inside the loop" of all destinations in the routing entry. So it will (potentially) call CFE_ES_GetAppId() multiple times. This also creates a double locking situation, because the SB lock is being held at the time this executes, and the ES lock needs to be acquired by CFE_ES_GetAppId()

Expected behavior
Code should query the caller AppID early, before taking the SB lock.

Additional context
The code works but is inefficient, and double locking is a potential deadlock.

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

[jphickey]

I'm calling this a bug even though it runs due to the double lock and that it is a potential code standards issue - as (I think?) we are not supposed to create local vars inside the function code, only at the beginning?

[astrogeco]

I'm calling this a bug even though it runs due to the double lock and that it is a potential code standards issue - as (I think?) we are not supposed to create local vars inside the function code, only at the beginning?

Do you have access to the style guide? Funny enough I stumbled on to this stack overflow discussion that discussed it. I'll need to go through the code standard and see where it mentions it.

[skliper]

3.7 - (6) "Function scope variable declarations shall precede any code statements. "

[jphickey]

There are other examples of block-scope variables being used, such as here:

cFE/fsw/cfe-core/src/sb/cfe_sb_api.c

Lines 1395 to 1403 in 8a7dc8f

	if (DestPtr->PipeId < CFE_SB_TLM_PIPEDEPTHSTATS_SIZE)
	{
	CFE_SB_PipeDepthStats_t *StatObj =
	&CFE_SB.StatTlmMsg.Payload.PipeDepthStats[DestPtr->PipeId];
	StatObj->InUse++;
	if(StatObj->InUse > StatObj->PeakInUse){
	StatObj->PeakInUse = StatObj->InUse;
	}/* end if */
	}

To clarify this is being declared in a block so it has block scope, not in a function scope. And they are at least at the beginning of the block, before code in that block.

Still, I think it is best to avoid the practice when possible, I consider it almost the same thing....