fix-288-remove_pipename changes #401

CDKnightNASA · 2019-11-08T17:31:58Z

Describe the contribution
Removes the PipeName field from the CFE_SB_PipeD_t struct.

Testing performed
make install
make test

Expected behavior changes
A clear and concise description of how this contribution will change behavior and level of impact.

API Change: internal change to CFE_SB_GetPipeName
Behavior Change: reduced memory impact of the CFE_SB_PipeD_t table

System(s) tested on:

Hardware: VM
OS: Linux
Versions: "master"

Contributor Info
Chris Knight, NASA Ames Research Center

skliper · 2019-11-13T17:19:15Z

CCB 20191123 - Discussed the string copies vs passing pointers, or use the get properties but could result in fixing implementation (os_queues dependency would get built in).

This implementation saves memory, but is a performance hit with the string copies. Not in the standard get/send path (just for error processing), and in the create/delete so not really a significant real time operation impact.

skliper · 2019-11-13T18:20:58Z

CCB 20191123 - approved for integration candidate, under additional review by jphickey and acudmore

ghost

Looks good. Approve.

ghost · 2019-11-13T21:23:54Z

fsw/cfe-core/src/sb/cfe_sb_priv.c

+        if (OS_QueueGetInfo(CFE_SB.PipeTbl[PipeId].SysQueueId, &queue_prop)
+            == OS_SUCCESS)
+        {
+            strcpy(queue_prop.name, PipeNameBuf);


Should you use strncpy here?

ghost · 2019-11-13T21:25:15Z

fsw/cfe-core/src/sb/cfe_sb_priv.c

-char *CFE_SB_GetPipeName(CFE_SB_PipeId_t PipeId){
-
-    static char PipeName4ErrCase[1] = {'\0'};
+int32 CFE_SB_GetPipeName(CFE_SB_PipeId_t PipeId, char *PipeNameBuf){


Is it worth checking to see if PipeNameBuf is non-NULL? If it's only used internally, then it might not be necessary.

I would also prefer to see this have a third parameter, which is the size of the output buffer, rather than assuming it is always exactly OS_MAX_API_NAME. There are several reasons for this, but mainly that OS_MAX_API_NAME is an OSAL definition, not a CFE platform definition, and theoretically a single build of CFE could be linked with a different build of OSAL without necessarily having the same definition.

Also note that the CFE_ES_GetAppName() function, which is similar in concept/purpose, has the following prototype:
int32 CFE_ES_GetAppName(char *AppName, uint32 AppId, uint32 BufferLength)

The output/destination buffer pointer is first, which mirrors the way many C library string functions work.

…nit test a priv? anyways...)

jphickey

I would like to see some updates to the CFE_SB_GetPipeName() to always fill the buffer with a null terminated string (i.e. an empty string) even in cases of error, to also pass in a separate size, and also reverse one case where it was called before releasing the SB lock.

jphickey · 2019-11-15T02:27:31Z

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

+        CFE_SB_GetPipeName(PipeId, PipeName);
        CFE_SB_UnlockSharedData(__func__,__LINE__);


In this case the CFE_SB_GetPipeName is called while the SB global lock is still held. This should probably be switched. In all other cases it looks like CFE_SB_GetPipeName is invoked outside the lock, which is probably preferable, as it avoids a double lock with OSAL. OSAL already has protections in case a delete occurs in close proximity to this call.

jphickey · 2019-11-15T02:33:02Z

fsw/cfe-core/src/sb/cfe_sb_priv.c

-char *CFE_SB_GetPipeName(CFE_SB_PipeId_t PipeId){
-
-    static char PipeName4ErrCase[1] = {'\0'};
+int32 CFE_SB_GetPipeName(CFE_SB_PipeId_t PipeId, char *PipeNameBuf){


I would also prefer to see this have a third parameter, which is the size of the output buffer, rather than assuming it is always exactly OS_MAX_API_NAME. There are several reasons for this, but mainly that OS_MAX_API_NAME is an OSAL definition, not a CFE platform definition, and theoretically a single build of CFE could be linked with a different build of OSAL without necessarily having the same definition.

Also note that the CFE_ES_GetAppName() function, which is similar in concept/purpose, has the following prototype:
int32 CFE_ES_GetAppName(char *AppName, uint32 AppId, uint32 BufferLength)

The output/destination buffer pointer is first, which mirrors the way many C library string functions work.

jphickey · 2019-11-15T02:34:28Z

fsw/cfe-core/src/sb/cfe_sb_priv.c

-        return &CFE_SB.PipeTbl[PipeId].PipeName[0];
+        if (OS_QueueGetInfo(CFE_SB.PipeTbl[PipeId].SysQueueId, &queue_prop)
+            == OS_SUCCESS){
+            strncpy(queue_prop.name, PipeNameBuf, OS_MAX_API_NAME-1);


It looks like the source/destination is reversed here

GAH! I can't believe I did that. Fixed this.

jphickey · 2019-11-15T02:37:43Z

fsw/cfe-core/src/sb/cfe_sb_priv.c

    }/* end if */

+    return Status;


I would also like to see this guaranteed to return a null-terminated string so long as a valid buffer was passed. We also did this for CFE_ES_GetAppName() too -- although it returns a status as to whether or not it was successful, nobody checked the status, and it looks like this is also the case with this function. If it fails, return an empty string.

CDKnightNASA · 2019-11-15T15:05:23Z

I would like to see some updates to the CFE_SB_GetPipeName() to always fill the buffer with a null terminated string (i.e. an empty string) even in cases of error,

Generally I prefer the minimum of change on error. Is there "style guidance" we should adopt and that should be documented in the coding style guide?

The SB code is not particularly consistent:

CreatePipe does this--initializes *PipeIdPtr to an invalid ID at the start of the function.

GetPipeOpts does NOT do this for *OptsPtr.

CFE_SB_GetAppTskName ...sorta...does this, by adding a null at the end of the buffer (no matter how long the actual name is.) But that, of course, does not handle the intervening bytes.

Remember, also, that CFE_SB_GetPipeName is a "private" function, although I could easily see exposing this. (In fact, there's one unit test already, which is kinda odd for a private function.)

to also pass in a separate size,

Again this is not what CFE_SB_GetAppTskName does, a lot of functions assume (and are documented to assume) that the buffer is OS_MAX_API_NAME in length. If we want to change the "guidance" and start recommending array buffers be passed with size, I am good with that.

and also reverse one case where it was called before releasing the SB lock.

Ok I will evaluate the locking logic per this and your other comment.

skliper · 2019-12-04T17:31:00Z

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

CDKnightNASA · 2019-12-04T19:17:05Z

OBE - pull request #404

fix-288-remove_pipename changes

47ad1ef

skliper requested review from a user and jphickey November 13, 2019 17:10

skliper added the CCB:Approved Indicates code review and approval by community CCB label Nov 13, 2019

ghost approved these changes Nov 13, 2019

View reviewed changes

strncpy and null pointer checks per Alan, plus fixed unit test (why u…

e788f93

…nit test a priv? anyways...)

jphickey requested changes Nov 15, 2019

View reviewed changes

CDKnightNASA added 2 commits November 15, 2019 08:26

WHOOPS!

8d309f9

GetPipeName when no lock held

474a4e9

CDKnightNASA closed this Dec 4, 2019

skliper added invalid and removed CCB:Approved Indicates code review and approval by community CCB labels Dec 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix-288-remove_pipename changes #401

fix-288-remove_pipename changes #401

CDKnightNASA commented Nov 8, 2019

skliper commented Nov 13, 2019

skliper commented Nov 13, 2019

ghost left a comment

ghost Nov 13, 2019

CDKnightNASA Nov 15, 2019

ghost Nov 13, 2019

jphickey Nov 15, 2019

jphickey left a comment

jphickey Nov 15, 2019

CDKnightNASA Nov 15, 2019

jphickey Nov 15, 2019

jphickey Nov 15, 2019

CDKnightNASA Nov 15, 2019

jphickey Nov 15, 2019

CDKnightNASA commented Nov 15, 2019

skliper commented Dec 4, 2019

CDKnightNASA commented Dec 4, 2019 •

edited by skliper

Loading

		CFE_SB_GetPipeName(PipeId, PipeName);
		CFE_SB_UnlockSharedData(__func__,__LINE__);

fix-288-remove_pipename changes #401

fix-288-remove_pipename changes #401

Conversation

CDKnightNASA commented Nov 8, 2019

skliper commented Nov 13, 2019

skliper commented Nov 13, 2019

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jphickey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CDKnightNASA commented Nov 15, 2019

skliper commented Dec 4, 2019

CDKnightNASA commented Dec 4, 2019 • edited by skliper Loading

CDKnightNASA commented Dec 4, 2019 •

edited by skliper

Loading