CFE_TBL_Load returns CFE_SUCCESS when initially loading an incomplete image file (CFE_TBL_WARN_SHORT_FILE internally)

[jphickey]

Describe the bug
During a CFE_TBL_Load call, the function is expected to return an error code if the loaded file was not complete (i.e. does not result in a complete image in memory).

In this case, if the image started at offset 0, but had fewer bytes than required for a complete table), it triggers the CFE_TBL_WARN_SHORT_FILE status internally inside CFE_TBL_LoadFromFile, but then this gets overwritten to CFE_SUCCESS in CFE_TBL_Load, making the return value to the user seem like the table was fully loaded/valid.

To Reproduce
Call CFE_TBL_Load() on a table which has not been initially loaded with a partial data file.

Expected behavior
Should return an error not CFE_SUCCESS, because the table image is only half loaded.

System observed on:
Ubuntu

Additional Context
Found as part of scrub in #1724

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

[jphickey]

Ping @skliper - need to decide whether this gets fixed for Caelum or just put in "known bugs" and deferred to next release (or better yet, rewrite of TBL).

AFAIK, "partial table loads" are a little-used (and little-tested) feature of TBL.

[astrogeco]

I used them in a previous project and they were extremely helpful since we had a pretty large parameter table

[jphickey]

After adding a lot more combinations and table content checks, this (luckily) seems to be a little more limited in scope than what I was worried about.

To summarize, the only "bad" outcome I've been able to confirm is that if the table hasn't yet been fully loaded and the user calls CFE_TBL_Load() with a partial image file (and note that it may or may not be known if the image is partial, there is no way to tell), then CFE_TBL_Load ends up returning CFE_SUCCESS, which normally would indicate a successful/complete table load. And it does put the table into a "fully loaded" internal state, i.e. one can get a pointer to it, and access the data. However, in this case, the data is only partially loaded.

After tracing through the code, it looks like there is no path whereby the internal warning codes CFE_TBL_WARN_PARTIAL_LOAD and CFE_TBL_WARN_SHORT_FILE can actually get returned to the caller thru CFE_TBL_Load. In all cases these warnings are overwritten somehow, whether its a first-time load or not.

An interesting discrepancy is that if the partial image file starts at a nonzero offset, it is detected and CFE_TBL_ERR_PARTIAL_LOAD is returned. However under the same conditions with a partial image file that starts at offset 0, it is not detected and CFE_SUCCESS is returned. Even though in both cases, only part of the image is valid...

[jphickey]

To summarize - the issue here is really in regard to consistency in how partial images on initial loads are detected.

The code catches the case of loading a "second half" partial image (nonzero start offset) and translates to an error (CFE_TBL_ERR_PARTIAL_LOAD) to indicate the loaded image in memory is not complete. However the exact same condition with a "first half" partial image (one that starts at offset 0) it does not catch and it returns SUCCESS - not correct. This should also be an error, because the image in memory is just as incorrect (half-loaded).

[skliper]

Note the rational in the CFE_TBL_Load associated requirement:

cFE/docs/cFE_FunctionalRequirements.csv

Line 451 in 5e41330

TBL: Initialize Contents - Partial,cTBL6302.2,"If the Request specified File contains less data than the size of the Request specified Table, the first portion of the Table Image will be initialized with the contents of the File and an Event Message shall be generated.",Some tables may allocate more space than is necessary at all times. The Event Message will serve as a warning but not prevent the initialization of the Table.

The rational seems be be saying this is intended behavior... initializing a table using CFE_TBL_Load with data starting at zero that doesn't fill the entire table isn't an error (unlike the requirements which including generating an error). Although I agree it should return some indication via the status code, but loading and using "short" tables seems to be the original intent. The path forward here may just be the redesign to remove the "partial load" concept from TBL in favor of file management which provides the same capabilities (upload/manage small chunks, assemble into a table, THEN load vs loading the individual small chunks and all the challenges of managing a partial load within TBL). You probably could still load a "short" file if that's really what is being required here... but it is a strange inconsistency between loading via command and loading via API (typically just at table initialization).

[jphickey]

Interesting. I must admit I can't quite grasp how loading only the first half of a table is OK, while loading only the second half of a table is an error. But nonetheless, if that's the intended behavior, then the implementation seems to be meeting that requirement.

Perhaps this bug should get categorized into the TBL requirements domain / redesign issues.

[skliper]

@jwilmot or @dmccomas may know better, but my guess would be related to the concept of a table containing a large array w/ unused slots (and an "end" marker). Something like an RTS or similar where the generic max table size may be 50... but the actual table only contains data for the first 3 slots (and a special 4 entry signifying the end). So it would be still an error to initialize with something that started at slot 7 (or whatever) since the previous slots would be garbage, but not to have a short table w/ an end marker.