Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use of globals to store ephemeral / in-transit data #91

Open
jphickey opened this issue Dec 6, 2021 · 0 comments
Open

Use of globals to store ephemeral / in-transit data #91

jphickey opened this issue Dec 6, 2021 · 0 comments

Comments

@jphickey
Copy link
Contributor

jphickey commented Dec 6, 2021

CF stores its current working pointers in a global variable called CF_AppData.engine:

In particular:
CF_AppData.engine.in.msg has a pointer to the buffer last received from SB
CF_AppData.engine.in.bytes_received has the size of that buffer (and is actually updated during the course of processing)
CF_AppData.engine.in.src and CF_AppData.engine.in.dst have the data extracted from the header of the most recent message.
and so forth...

Importantly: none of these values are supposed to be carried across wakeups. All values are reset in their entirety on every wakeup, and in fact with each channel. All data is ephemeral and is only valid while the CFDP app is actively processing that packet. As soon as processing of the current packet completes, the data is no longer valid. When the wakeup cycle completes, only the CF_AppData.engine.in.msg is actively cleared. All other fields will be left with whatever data was in them.

Also notable - there is a mixture of API calls where sometimes the pointer to the packet data is passed in directly, as it is here via the ph argument:

CF/fsw/src/cf_cfdp_r.c

Lines 436 to 447 in 7b99b91

static void CF_CFDP_R1_SubstateRecvFileData(transaction_t *t, const pdu_header_t *ph)
{
CFE_MSG_Size_t bytes_received; /* initialized in CF_CFDP_R_ProcessFd() */
/* got file data pdu? */
if (CF_CFDP_RecvFd(t) || CF_CFDP_R_ProcessFd(t, &bytes_received))
{
goto err_out;
}
/* class 1 digests crc */
CF_CRC_Digest(&t->crc, STATIC_CAST(ph, pdu_fd_t)->fdd.data, (uint32)bytes_received);

The call to CF_CFDP_R_ProcessFd also needs the packet data, but it does not pass it along. Instead, this function grabs it from the global (theoretically the same packet):

CF/fsw/src/cf_cfdp_r.c

Lines 221 to 224 in 7b99b91

static int CF_CFDP_R_ProcessFd(transaction_t *t, CFE_MSG_Size_t *bytes_received)
{
pdu_header_t *ph = &((pdu_r_msg_t *)CF_AppData.engine.in.msg)->ph;
*bytes_received = CF_AppData.engine.in.bytes_received;

This inconsistency should be addressed. If the intent is to always store the current packet in a global, then code needing to access it should always retrieve it from the global. There should not be some APIs which pass a pointer to the structure, mixed with others that get it directly from the global (where they are supposed to be acting on the same data), as this creates the opportunity that they could diverge.

For ephemeral data, it is fine to pre-allocate a buffer in a global to avoid dynamic allocation, but the pointer to this data should be passed consistently down through the API where needed. This design allows for safer evolution, permitting the use of multiple buffers or even multiple threads should that become a requirement (e.g. create a child task per CFDP channel to make them more independent).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants