Fix #210, Add SB API to get pipe by name #404
Conversation
|
CCB 20191113 - pipe id and queue id are of different sizes/types, could look into collapsing into 1 type, associated with the type safe ticket also, could also optimize by mapping them 1 to 1. |
|
CCB 20191123 - approved for integration candidate, under additional review by jphickey and acudmore |
skliper
added
the
CCB:Approved
There was a problem hiding this comment.
The CFE_SB_GetPipeIdByName function needs some changes in how it handles the locking. It is holding the lock for too long and calling into other subsystems while holding the lock. We just had to fix a similar issue in EVS where it was sending events while holding a global lock.
The "core" of the function should take the global lock, do the necessary searching, and either copy the pipeid value to the output buffer (success) or increment an appropriate counter (failure). It should then release the SB lock, and then to the actual CFE_EVS_SendEvent associated with it (debug event on success, or appropriate error event on failure).
Also, since CFE_SB_GetPipeIdByName is being added to the public SB API, it needs a stub function implemented in ut_sb_stubs.c too. I'd suggest using CFE_ES_GetAppIdByName stub as a guide but that one is more complex than necessary (I think it had to support some existing oddball test scenarios). It should be just as simple as using the UT_DEFAULT_IMPL macro followed up by a UT_Stub_CopyToLocal() to allow a test case to provide its own outputs.
Looking at the CFE_ES_GetAppIDByName, I realize there is no CFE_SB_GetPipeName(char *buf, PipeId_t id) ...should I add it? Also while a bit nit-picky, it's good to resolve these things for a style guide. I note two things:
I've captured both in my draft "style" doc (in the fix-404-contributing branch) as "to be decided." Along with other TBD's. |
|
CCB - 12/4/2019: Consider adding an OSAL API which just retrieves the name of a resource, simplifies this issue and the other's related |
|
I need to squash |
|
Is this ready for merge to integration branch? I don't see all the requests being closed out. @acudmore @jphickey |
|
I would still like to see this squashed and rebased into a single commit. There is lots of back and forth on the branch now (incl. some bogus/mistake commits). This should be cleaned up before merging. |
skliper
force-pushed
the
fix-210-getpipidbyname
branch
from
a9fd5ce
to
f9b3e31
Compare
|
I rebased on current master and squashed for review. @acudmore and @jphickey please re-review. |
fsw/cfe-core/src/sb/cfe_sb_api.c
Outdated
| }else{ | ||
| if (OS_QueueGetInfo(CFE_SB.PipeTbl[PipeId].SysQueueId, &queue_prop) | ||
| == OS_SUCCESS){ | ||
| strncpy(PipeNameBuf, queue_prop.name, OS_MAX_API_NAME-1); |
There was a problem hiding this comment.
This needs to use PipeNameSize, not OS_MAX_API_NAME-1. It should also ensure a null-terminated output.
There was a problem hiding this comment.
Hate to be a pain, but still need to make sure that the output is null terminated. This should also handle the case if the passed in size is 0 (since it is handling a passed-in buffer of NULL, should handle this too).
|
Analyzing log I see I should have done the squash differently (new method used successfully on #452) to retain authorship by Chris. |
CDKnightNASA
force-pushed
the
fix-210-getpipidbyname
branch
from
0e76026
to
3916896
Compare
|
re-squashed |
There was a problem hiding this comment.
Happy with the sizeof() operations now, but still a couple issues in the CFE_SB_GetPipeName() API. Should handle a passed in buffer size of 0 (this can be checked at the same time as the NULL buffer, first conditional) and should also ensure that the buffer output is always null terminated.
Moves CFE_SB_GetPipeName to public API Adds CFE_SB_GetPipeIdByName Adds 6 related events Adds to SB HK packet (GetPipeIdByNameErrorCounter) Updates associated internal name logic Unit tests and stubs added
CDKnightNASA
force-pushed
the
fix-210-getpipidbyname
branch
from
3916896
to
302b68a
Compare
|
CCB 20200122 - Reviewed and approved for CI |
Fix #210, Add SB API to get pipe by name
Describe the contribution
Fixes issue #210
Testing performed
make mission-all; run cpu1/cfe_core_default_cpu1/unit-test/cfe_core_default_cpu1_sb_UT
Expected behavior changes
Adds a new function, CFE_SB_GetPipeIdByName, which retrieves the pipe ID given a name of a pipe. As this would conflict with the removal of the PipeName from the PipeTbl, this code uses the OS_QueueGetIdByName and iterates across the PipeTbl to find the matching entry.
System(s) tested on:
Ubuntu 64-bit Linux 19.04.
Contributor Info
Chris Knight, NASA Ames Research Center.