search

nasa / CF

The Core Flight System (cFS) CFDP application.
Apache License 2.0
79 stars 45 forks source link

Replace custom strnlen implementation with OSAL version #134

Closed astrogeco closed 2 years ago

astrogeco commented 2 years ago

TODO in code references replacing this function with an osal version

https://github.com/nasa/CF/blob/a894069439316ef89ad7751f3a03036930158a07/fsw/src/cf_cfdp.c#L499-L522

Apps should avoid developing custom implementation of utilities especially generic ones like strnlen.

See osal implementation below

https://github.com/nasa/osal/blob/4cc6dbb5019d0589d5ce52e3755a0b7a012ade3c/src/os/shared/inc/os-shared-common.h#L138-L159

jphickey commented 2 years ago

Actually, I don't think this is worth it. It opens a big can of unintended consequences - because the OSAL function is actually private within OSAL (it is not part of the public OSAL API) and making it part of the public API creates havoc on unit tests.

The reality is, its such a simple function, there isn't much value in using the same implementation. "memchr()" is well defined and C99 standard, there is nothing wrong with duplicating these 4 lines of code.

Note that "strnlen" - in the operating systems that provide it - is a C library function, not really an OS function. So it doesn't really belong in OSAL public API either - it isn't part of the OS abstraction at all, it is simply a helpful utility. While its nice to have a common collection of helpful utilities and C library extensions somewhere, that isn't really OSAL's job either - OSAL should focus on being an OS abstraction.

astrogeco commented 2 years ago

Good point, I didn't realize it wasn't a public one. We can close this but I wonder if there's a place for stuff like this in osal or some type of cfe library?

astrogeco commented 2 years ago

Closing as invalid since OSAL function is not public