Fix #1440, Split up BinSemGetInfo() to avoid partial success returns #1480

thnkslprpt · 2024-10-06T19:01:51Z

Checklist

I reviewed the Contributing Guide.
I signed and emailed the appropriate Contributor License Agreement to [email protected] and copied [email protected].

Describe the contribution

Fixes SemGetInfo partial success cases #1440
- Splits the BinSemGetInfo() API into 3, allowing clear success/failure (or not implemented) for each.

Testing performed
GitHub CI actions all passing successfully (incl. Build + Run, Unit/Functional Tests etc.).
Tested locally to confirm no loss of coverage compared with the current main branch, and all lines/branches/functions in the new version are covered.

Expected behavior changes
Removes this source of 'partial success' which can be confusing and also increases complexity for dealing with the return for users calling into this API.

System(s) tested on
Debian 12 using the current main branch of cFS bundle.

Contributor Info
Avi Weiss @thnkslprpt

src/os/shared/src/osapi-binsem.c

@@ -249,7 +249,7 @@
 *           See description in API and header file for detail
 *
 *-----------------------------------------------------------------*/
-int32 OS_BinSemGetInfo(osal_id_t sem_id, OS_bin_sem_prop_t *bin_prop)
+int32 OS_BinSemGetName(osal_id_t sem_id, OS_bin_sem_prop_t *bin_prop)


src/os/shared/src/osapi-binsem.c

+ *           See description in API and header file for detail
+ *
+ *-----------------------------------------------------------------*/
+int32 OS_BinSemGetCreator(osal_id_t sem_id, OS_bin_sem_prop_t *bin_prop)


src/os/shared/src/osapi-binsem.c

+ *           See description in API and header file for detail
+ *
+ *-----------------------------------------------------------------*/
+int32 OS_BinSemGetValue(osal_id_t sem_id, OS_bin_sem_prop_t *bin_prop)


src/ut-stubs/osapi-binsem-handlers.c

 * -----------------------------------------------------------------
 */
-void UT_DefaultHandler_OS_BinSemGetInfo(void *UserObj, UT_EntryKey_t FuncKey, const UT_StubContext_t *Context)
+void UT_DefaultHandler_OS_BinSemGetName(void *UserObj, UT_EntryKey_t FuncKey, const UT_StubContext_t *Context)


src/ut-stubs/osapi-binsem-handlers.c

+ * Default handler implementation for 'OS_BinSemGetCreator' stub
+ * -----------------------------------------------------------------
+ */
+void UT_DefaultHandler_OS_BinSemGetCreator(void *UserObj, UT_EntryKey_t FuncKey, const UT_StubContext_t *Context)


src/ut-stubs/osapi-binsem-handlers.c

+    if (status == OS_SUCCESS &&
+        UT_Stub_CopyToLocal(UT_KEY(OS_BinSemGetCreator), bin_prop, sizeof(*bin_prop)) < sizeof(*bin_prop))


src/ut-stubs/osapi-binsem-stubs.c

 * ----------------------------------------------------
 */
-int32 OS_BinSemGetInfo(osal_id_t sem_id, OS_bin_sem_prop_t *bin_prop)
+int32 OS_BinSemGetName(osal_id_t sem_id, OS_bin_sem_prop_t *bin_prop)


src/ut-stubs/osapi-binsem-stubs.c

+ * Generated stub function for OS_BinSemGetCreator()
+ * ----------------------------------------------------
+ */
+int32 OS_BinSemGetCreator(osal_id_t sem_id, OS_bin_sem_prop_t *bin_prop)


thnkslprpt · 2024-10-06T19:04:17Z

Pretty simple change-set, although the change is 'breaking' I guess, so has to wait for a major release?

Note - there is a single reference to BinSemGetInfo() in cFE which will need to be updated along with this if it is merged.

skliper · 2024-10-08T14:59:31Z

Pretty simple change-set, although the change is 'breaking' I guess, so has to wait for a major release?

Note - there is a single reference to BinSemGetInfo() in cFE which will need to be updated along with this if it is merged.

I recommend just deprecating the old API vs fully removing, then it doesn't have to be a major release (at least per the old rules I used). See other uses of OSAL_OMIT_DEPRECATED.

thnkslprpt · 2024-10-08T16:56:57Z

I recommend just deprecating the old API vs fully removing, then it doesn't have to be a major release (at least per the old rules I used). See other uses of OSAL_OMIT_DEPRECATED.

Good point. Will update it.

src/os/posix/src/os-impl-binsem.c

src/os/shared/src/osapi-binsem.c

thnkslprpt · 2024-10-09T18:44:38Z

@skliper I'm not sure if I did the deprecation guards correctly, but do you know if it's possible or have the debug and release builds pass the tests given that the deprecated elements are only removed in the debug workflow?

osal/.github/workflows/standalone-build.yml

Line 60 in 8270166

-DOSAL_OMIT_DEPRECATED=${{ env.is_debug }}

Is it trying to run the same set of tests, but with and without the deprecated code? (that doesn't seem to make sense right)

(I'm assuming we only upkeep tests for the non-deprecated code...)

skliper · 2024-10-09T19:10:15Z

@skliper I'm not sure if I did the deprecation guards correctly...

I'm not really following your full question, but I recommend only wrapping the old API in the omit deprecated ifndef not the new stuff. Then hopefully the tests would pass?

src/os/posix/src/os-impl-binsem.c

@@ -466,6 +466,8 @@
    return (OS_GenericBinSemTake_Impl(token, &ts));
 }

+#ifdef OSAL_OMIT_DEPRECATED


src/os/shared/src/osapi-binsem.c

@@ -243,13 +243,45 @@
    return return_code;
 }

+#ifdef OSAL_OMIT_DEPRECATED


thnkslprpt · 2024-10-10T10:47:51Z

src/tests/bin-sem-test/bin-sem-test.c

    UtAssert_INT32_EQ(OS_BinSemCreate(NULL, "Test_Sem", 0, 0), OS_INVALID_POINTER);
    UtAssert_INT32_EQ(OS_BinSemCreate(&sem_id[0], NULL, 0, 0), OS_INVALID_POINTER);
-    UtAssert_INT32_EQ(OS_BinSemGetInfo(sem_id[0], NULL), OS_INVALID_POINTER);
+
+    // Initialize sem_id[0] to a valid value for the following NULL checks


Tests were not passing on 1/4 builds without this addition (Debug, Ubuntu 22.04).
Somehow, it behaves differently with the sem_id[0] not being set to a valid value on that target on the preceding line. With the added code though, all builds are fine.

thnkslprpt · 2024-10-10T10:50:30Z

@skliper I'm not sure if I did the deprecation guards correctly...

I'm not really following your full question, but I recommend only wrapping the old API in the omit deprecated ifndef not the new stuff. Then hopefully the tests would pass?

OK all good now - it wasn't building for me locally but that might have been a symptom of my own build command. Tests passing with a small addition as per the above comment.

github-advanced-security bot found potential problems Oct 6, 2024

View reviewed changes

thnkslprpt force-pushed the fix-1440-split-bin-sem-get-info branch 2 times, most recently from 0e2d950 to 6459099 Compare October 8, 2024 18:19

github-advanced-security bot found potential problems Oct 8, 2024

View reviewed changes

src/os/posix/src/os-impl-binsem.c Fixed Show fixed Hide fixed

src/os/shared/src/osapi-binsem.c Fixed Show fixed Hide fixed

Fix nasa#1440, Split up BinSemGetInfo() to avoid partial success returns

f8855ab

thnkslprpt force-pushed the fix-1440-split-bin-sem-get-info branch from 6459099 to f8855ab Compare October 10, 2024 09:28

github-advanced-security bot found potential problems Oct 10, 2024

View reviewed changes

thnkslprpt commented Oct 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1440, Split up BinSemGetInfo() to avoid partial success returns #1480

Fix #1440, Split up BinSemGetInfo() to avoid partial success returns #1480

thnkslprpt commented Oct 6, 2024

thnkslprpt commented Oct 6, 2024 •

edited

Loading

skliper commented Oct 8, 2024

thnkslprpt commented Oct 8, 2024

thnkslprpt commented Oct 9, 2024

skliper commented Oct 9, 2024

thnkslprpt Oct 10, 2024

thnkslprpt commented Oct 10, 2024

		if (status == OS_SUCCESS &&
		UT_Stub_CopyToLocal(UT_KEY(OS_BinSemGetCreator), bin_prop, sizeof(bin_prop)) < sizeof(bin_prop))

Fix #1440, Split up BinSemGetInfo() to avoid partial success returns #1480

Are you sure you want to change the base?

Fix #1440, Split up BinSemGetInfo() to avoid partial success returns #1480

Conversation

thnkslprpt commented Oct 6, 2024

thnkslprpt commented Oct 6, 2024 • edited Loading

skliper commented Oct 8, 2024

thnkslprpt commented Oct 8, 2024

thnkslprpt commented Oct 9, 2024

skliper commented Oct 9, 2024

thnkslprpt Oct 10, 2024

Choose a reason for hiding this comment

thnkslprpt commented Oct 10, 2024

thnkslprpt commented Oct 6, 2024 •

edited

Loading