Inconsistent parameter passing (chan_num vs. channel pointer)

[jphickey]

There does not seem to be any consistency in CF as to whether identifiers passed to functions are done in the form of a number (such as a channel number) where the function then gets the channel pointer internally by doing a table lookup, or by passing a pointer to the structure. The CF contains both forms, and sometimes passes a pointer when the implementation really needed the number.

Example:

CF/fsw/src/cf_cfdp.c

Lines 1357 to 1362 in 7b99b91

	static void CF_CFDP_ReceiveMessage(channel_t *c)
	{
	transaction_t *t; /* initialized below */
	uint32 count = 0;
	int32 status;
	const int chan_num = (c - CF_AppData.engine.channels);

In this case the CF_ReceiveMessage was (for some reason) declared as accepting a pointer to the channel structure, but it really needs the channel number, so it does a bit of pointer arithmetic (c - CF_AppData.engine.channels) to get the number.

This type of pointer manipulation can be error prone, particularly if the code evolves in such a way where the c pointer might not be pointing to an entry in the table, this might produce an out-of-range channel number. This can also happen during unit test where its common to pass in test values -- even if FSW never expects a value not within the table, its still possible to happen.

Recommendation: If channel numbers are generally always needed, pass only the channel number around. It is safer because it can be more easily range-checked if necessary.

Alternative: Store the channel number inside the channel structure, so the FSW can more simply look it up and does not need to recompute it (avoids assumption that the pointer is pointing to a chan table entry). This can avoid repetitiously looking up a chan_num to get the pointer, allowing direct pointers to be passed around. But does cost a little memory and introduces the risk that something (e.g. a bug somewhere else) can overwrite or change the chan_num and make it wrong.