os/shared/osapi-module.c: the return value of the call to OS_ModuleLoad_Impl should be checked

[stanislaw]

Is your feature request related to a problem? Please describe.

I have noticed this issue while working on making the OSAL's tests pass on macOS (see #352). I am reporting this issue separately from the rest of the changeset suggested there.

The issue is in the following fragment of code as found in the current master branch (as of 64ad0f5):

    if(return_code == OS_SUCCESS)
    {
        /*
         * Save all the data to our own internal module table, but
         * only if OS_MAX_MODULES is greater than 0
         */
#if (OS_MAX_MODULES > 0)
        memset(&OS_module_table[local_id], 0, sizeof(OS_module_internal_record_t));
        strncpy(OS_module_table[local_id].module_name, module_name, OS_MAX_API_NAME);
        strncpy(OS_module_table[local_id].file_name, filename, OS_MAX_PATH_LEN);
        record->name_entry = OS_module_table[local_id].module_name;

        /* Now call the OS-specific implementation.  This reads info from the module table. */
        return_code = OS_ModuleLoad_Impl(local_id, translated_path);
#endif

        /* Check result, finalize record, and unlock global table. */
        return_code = OS_ObjectIdFinalizeNew(return_code, record, module_id);
#endif

We can see that the return value of the OS_ModuleLoad_Impl is not tested for != OS_SUCCESS so the execution can continue even if the call to this function returns an error.

This is a direct link to the line:

osal/src/os/shared/osapi-module.c

Line 244 in 64ad0f5

return_code = OS_ModuleLoad_Impl(local_id, translated_path);

Describe the solution you'd like

Most of the code in OSAL does check the return values: if(return_code == OS_SUCCESS) or if(return_code != OS_SUCCESS) so it makes sense to create such a check in this case too.

I am going to create a Pull Request that addresses this issue.

Describe alternatives you've considered

To the best of my knowledge, a proper return value checking is the only way to get fast feedback from a call to the OS_ModuleLoad_Impl if something goes wrong.

Additional context

None.

Requester Info

Stanislav Pankevich, independent contribution.

[jphickey]

The call to OS_ObjectIdFinalizeNew() handles clean up after the low level implementation runs. Basically after OS_ObjectIdAllocateNew() runs the position in the table is found but kept in an "intermediate" (reserved but not assigned) state. The OS_ObjectIdFinalizeNew() call then either completes the association by assigning the final object ID on success, or clears the entry on failure so the table slot can be re-used.

I don't think there is an issue here.

[skliper]

Wouldn't you want to pass back a failure if it occurs in the load?

[jphickey]

The OS_ObjectIdFinalizeNew function does return the status back. It is written that way (pass-thru) so it can adjust the return value if needed - Although in practice there probably shouldn't be a reason to do so. The current implementation basically always passes it through but the capability is there to e.g. re-map error codes for compatibility if necessary.

[skliper]

Thanks, I didn't look closely enough at the parameters! So I'm on the same page now, and concur it's implemented as intended.

[stanislaw]

I am closing this issue. Now, after the explanation by @jphickey, I am sure that this issue was caused by my misunderstanding of the code. I have left a comment that explains this in more detail: #360 (comment).