Deprecate OS_open and OS_creat

[jphickey]

Is your feature request related to a problem? Please describe.
For historical/backward compatibility reasons, the API of these two functions doesn't follow the typical flow. Rather than providing a uint32 ID output buffer as the first argument with a separate int32 return code, they return the OSAL ID cast as an int32 on success. For these functions, the caller is expected to check if the result is negative, and if so, consider it an error code. Whereas if it is non-negative, the caller is expected to cast it back to a uint32 type and interpret it as an OSAL ID.

Describe the solution you'd like
These should be like all other OSAL APIs and pass back the ID separately from the return/error status.

Describe alternatives you've considered
Leave as is. But these two functions present a challenge when making a distinct type for OSAL IDs .

Additional context
In the current implementation,. these are just compatibility wrappers anyway. They both call OS_OpenCreate() internally, which provides both open (existing file) and creat (new file) based on the flags it was passed. The OS_OpenCreate function does follow the correct pattern so one option would be to just expose this to the public API.

The other option is to create a new version of OS_open and OS_creat which follow the correct pattern. But in order to provide a transition they would have to use different names.

Requester Info
Joseph Hickey, Vantage Systems, Inc.

[jphickey]

One other possible compromise is to keep these as-is returning int32, but provide another function to decode the return value into a status and ID of the correct type (e.g. uint32) rather than relying on the user to cast it themselves and do it properly.

This has been a source of confusion in the past, see #498.

[jphickey]

Would like to get some feedback on this idea in the next CCB.

[skliper]

I'm in favor of the suggested approach (deprecate, expose OS_OpenCreate).

[skliper]

@acudmore @ejtimmon ping, deprecation proposal

[skliper]

CCB 2020-08-12: No objections, but solicited responses/review.

[jphickey]

Also @CDKnightNASA - you might have some thoughts here as well?

Summary of questions are:

1. Does the inconsistency/confusion around this API warrant making a breaking change to try to fix it and make it like the others?

If (1) is yes, then:

2. Should we keep a separate open & creat routine or combine them into a single API, using flags to differentiate whether a file should be created or not? Or should we keep it 1:1 and offer a direct replacement for OS_open and OS_creat, to make it easier for apps to update?

If (1) is no, then:

3. Is it worth providing a new API to help users check/decode the int32 value returned from these existing API calls? This would be backward compatible at least, as it doesn't change/remove any existing function, while alleviating the need for applications to directly cast/convert between OSAL IDs and other integer data types, which is the fundamental concern here.

[jwilmot]

No objections to just using OS_OpenCreate as this follows the normal API structure found in many file systems where the create option flag is passed in. Hopefully the change only impacts a limited number of applications.

[jphickey]

Worth noting, in case anyone is unconvinced about the benefits of being type-strict, I found a mistake/bug in the unit tests here, that's probably been lurking for quite some time, where it fails to handle this return code properly:

osal/src/tests/file-api-test/file-api-test.c

Lines 794 to 795 in 8cfd6fe

	fd1 = OS_open(newfilename1,OS_READ_ONLY,0644);
	UtAssert_True(status >= OS_SUCCESS, "status after open 1 = %d",(int)status);

This type of goof-up becomes very apparent/obvious when using a distinct type, and can actually trigger a compiler error if desired.

[ejtimmon]

@acudmore @ejtimmon ping, deprecation proposal

This will impact several apps, but I'm not opposed.

[skliper]

@jphickey - did you start implementing this change, or want me to take it?

[jphickey]

A previously merged PR has already exposed the OS_OpenCreate() API in the public header as discussed, so the next step was to replace current CFE references to OS_open/OS_creat.

I hadn't started on that apart yet because I needed to get nasa/cFE#868 in first, but now that is settled there shouldn't be any issue in replacing these refs now. I can submit a CFE ticket for that. If someone else is available and wants to take it on that would be fine, otherwise I'm happy to do it.

[jphickey]

See nasa/cFE#893 for the CFE work to be done.