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

CFE_ES_GetPoolBufInfo Header has incorrect return description #1606

Closed
zanzaben opened this issue Jun 4, 2021 · 1 comment · Fixed by #1671 or #1665
Closed

CFE_ES_GetPoolBufInfo Header has incorrect return description #1606

zanzaben opened this issue Jun 4, 2021 · 1 comment · Fixed by #1671 or #1665
Assignees
Labels
docs This change only affects documentation.
Milestone

Comments

@zanzaben
Copy link
Contributor

zanzaben commented Jun 4, 2021

Describe the bug
The return in the header says it returns Execution status but it actually returns the buffer size or error code.

Expected behavior
Header file should be changed to match the implementation.

Additional context
The code that does this has a comment saying it's not workable so we could also change the implementation

if (Status == CFE_SUCCESS)
{
/*
* Historically this function returns the size of the buffer
* as an int32. This is not workable for large (64 bit) pools.
*/
Status = (int32)DataSize;
}
return Status;

Reporter Info
Alex Campbell GSFC

@jphickey
Copy link
Contributor

jphickey commented Jun 4, 2021

Yeah, it would be nice to fix this implementation, but that is an API change. Internally, the code should support any block size up to size_t , but with a historical int32 return type that returns the size this is limited to 2GB in practice. Which is still beyond what is typically needed - in our test configurations it only goes up to 80k bytes - so 2GB is still far above what is typically used here.

Ideally we should note in the user guide and/or release notes that pools (currently) should not exceed 2GB in size. Maybe in a future cFE we can fix the rest of the APIs. But most flight hardware doesn't even have 2GB of RAM total, so I doubt any users will have an issue with that.

@skliper skliper added this to the 7.0.0 milestone Jun 7, 2021
@skliper skliper added the docs This change only affects documentation. label Jun 7, 2021
jphickey added a commit to jphickey/cFE that referenced this issue Jul 16, 2021
Corrects the return type documentation, on success this function
returns the size of the buffer, it does not return CFE_SUCCESS.

Additionally, this updates the general description of the pool
implementation to reflect the variances in platform architectures
(i.e. the buffer descriptor/overhead may be 12 bytes on a CPU
with 32 bit size_t, but will be greater on a CPU with a 64 bit
size_t).
astrogeco added a commit that referenced this issue Jul 20, 2021
Fix #1606, update documentation for CFE_ES_GetPoolBufInfo
paulober pushed a commit to paulober/cFE that referenced this issue Aug 1, 2021
Corrects the return type documentation, on success this function
returns the size of the buffer, it does not return CFE_SUCCESS.

Additionally, this updates the general description of the pool
implementation to reflect the variances in platform architectures
(i.e. the buffer descriptor/overhead may be 12 bytes on a CPU
with 32 bit size_t, but will be greater on a CPU with a 64 bit
size_t).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change only affects documentation.
Projects
None yet
3 participants