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

Improve resource management and internal consistency in ES #797

Closed
jphickey opened this issue Aug 10, 2020 · 3 comments · Fixed by #859
Closed

Improve resource management and internal consistency in ES #797

jphickey opened this issue Aug 10, 2020 · 3 comments · Fixed by #859
Assignees
Milestone

Comments

@jphickey
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Executive Services (ES) maintains many internal tables of resources/objects, which track applications, libraries, tasks, counters, and memory pools, etc.

There is a lot of inconsistency in how these internal objects are managed/tracked. Some have a RecordUsed boolean that is set true/false depending on whether the record is used. App table uses the AppState member. The memory pool passes around direct pointers which are dereferenced (potentially dangerous).

Furthermore, all "ID" values issued to external apps are zero based, and therefore can easily alias other object types or even other objects of the same type. For instance, if one app had ID "5" and it was deleted, and after this a new/different app was started, the new app might also be assigned ID "5" ... this means any old/stale reference to AppID 5 will now be referring to the wrong app.

Describe the solution you'd like
Define a properly abstract "resource ID" type and use it consistently across all these various internal tables. The abstraction should be based on/compatible with what OSAL does for its internal records.

  • The "id" value also serves as a marker to indicate whether the respective table entry is in use or not.
  • Zero is reserved as an invalid value, and marks entries which are NOT in use. (e.g. so a memset() to all zero can consistently clear an entry). Valid entries/ids are never zero.
  • Valid values are split into a "type" and sequential "index" value
  • Type is unique for apps. libs, counters, etc so these cannot get crossed/misinterpreted (i.e. can't pass an appID in place of a libID or vice versa).
  • Index is sequential and does not immediately repeat (i.e. don't wrap until 0xFFFF, do not recycle/reassign IDs after deletion).
  • Provide a consistent mechanism to convert ID to a zero based index where an array/table is needed.

Additional context
This internal cleanup is a prerequisite to several related tickets:

Requester Info
Joseph Hickey, Vantage Systems, Inc.

@jphickey
Copy link
Contributor Author

Note - This came up in discussion in #28 -- split into a separate ticket as it probably warrants a separate review of its own.

@skliper
Copy link
Contributor

skliper commented Aug 11, 2020

@tngo67 - is this approach acceptable for your team? Mostly under the hood, but ends with a much more consistent implementation of ID's.

@tngo67
Copy link

tngo67 commented Aug 11, 2020

Yes! I vote for moving away from using array indices for IDs, at least for this specific issue, for reasons jphickey stated.

@jphickey jphickey self-assigned this Sep 2, 2020
jphickey added a commit to jphickey/cFE that referenced this issue Sep 2, 2020
Introduce wrapper/accessor functions to look up table
entries by ID for ES & EVS subsystems.

__Do not use AppID as a table index__.

Note - This does not change existing external APIs and AppIDs
are still zero-based uint32.  This only changes the internal
structures to remove use of ID as an array index, and to use a lookup
function to locate the table entry from an ID.  All entry access
is then performed via the table entry pointer, rather than
as an array index.

This provides the groundwork for abstract IDs without actually
changing anything fundamental about resource IDs.
jphickey added a commit to jphickey/cFE that referenced this issue Sep 2, 2020
Introduce wrapper/accessor functions to look up table
entries by ID for ES & EVS subsystems.

__Do not use AppID as a table index__.

Note - This does not change existing external APIs and AppIDs
are still zero-based uint32.  This only changes the internal
structures to remove use of ID as an array index, and to use a lookup
function to locate the table entry from an ID.  All entry access
is then performed via the table entry pointer, rather than
as an array index.

This provides the groundwork for abstract IDs without actually
changing anything fundamental about resource IDs.
jphickey added a commit to jphickey/cFE that referenced this issue Sep 2, 2020
Update the EVS subsystem to follow the ES pattern for
internal table management.

Do not use AppID directly as a table index.  Instead,
use a separate lookup routine to get a pointer to the
entry, then access the entry via the pointer.

Also introduce inline helper functions to get/set
status of entry (free/not free), etc.
jphickey added a commit to jphickey/cFE that referenced this issue Sep 2, 2020
Minor fixup for use of IDs when logging
jphickey added a commit to jphickey/cFE that referenced this issue Sep 2, 2020
Change SB to get its task ID from ES rather than getting
it directly from OSAL.  Update syslog calls to use the
ES-supplied conversion to integer rather than direct cast.
jphickey added a commit to jphickey/cFE that referenced this issue Sep 2, 2020
Update TBL to use the new ES-supplied ID manipulations.  Update
all syslog calls to convert to integer using ES wrapper.
jphickey added a commit to jphickey/cFE that referenced this issue Sep 2, 2020
Update the TIME subsystem to use the new ES-supplied ID
abstractions.  Do not use AppID directly as array index
when registering sync callbacks, use the ES-supplied
conversion to array index before accessing local table.

Also update logging to use ES-supplied conversion
astrogeco added a commit that referenced this issue Sep 4, 2020
Fix #797, refactor internal table/id management
@astrogeco astrogeco added this to the 7.0.0 milestone Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants