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

Add check before pthread_join #1599

Merged
merged 4 commits into from
Mar 22, 2021
Merged

Add check before pthread_join #1599

merged 4 commits into from
Mar 22, 2021

Conversation

pvyawaha
Copy link
Contributor

Description of changes:
This change adds check so that pthread_join is called only when ota thread is created.

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

@aggarw13
Copy link
Contributor

The set of bug improvements to the OTA demos (across different PRs since the previous release) would be good to add as a single CHANGELOG item point

@vikivivi
Copy link

vikivivi commented Mar 19, 2021

  • From the manpage of pthread_join. See possible errors code upon return.
RETURN VALUE
       On success, pthread_join() returns 0; on error, it returns an error number.

ERRORS
       EDEADLK
              A deadlock was detected (e.g., two threads tried to join with each other); or thread specifies the calling thread.

       EINVAL thread is not a joinable thread.

       EINVAL Another thread is already waiting to join with this thread.

       ESRCH  No thread with the ID thread could be found.
/**
 * @brief Start OTA demo.
 *
 * @return   EXIT_SUCCESS or EXIT_FAILURE.
 */
static int startOTADemo( void );
static int startOTADemo( void )
{
   ...
    if( returnStatus == EXIT_SUCCESS )
    {
        returnStatus = pthread_join( threadHandle, NULL );

        if( returnStatus != EXIT_SUCCESS )
        {
            LogError( ( "Failed to join thread"
                        ",error code = %d",
                        returnStatus ) );
        }
    }

    return returnStatus;
}
  • Since startOTADemo() is documented to return either EXIT_SUCCESS or EXIT_FAILURE, we should not use error code of pthread_join() as the returned error code from startOTADemo(). In case someone uses the demo code startOTADemo() like this.
if( startOTADemo() != EXIT_FAILURE )   /* Error code of pthread_join will have the same as EXIT_SUCCESS */
{
   ....
}
  • Proposed fix at the section of pthread_join().
    • Declare new variable returnJoin to LogError pthread_join() error code. Optional if just print failed to join thread.
    • On success of pthread_join() compare return 0. Just like all other pthread API in OTA demo. (I know EXIT_SUCCESS = 0 as well, but does not belong to the same list of error code)
    if( returnStatus == EXIT_SUCCESS )
    {
        if( ( returnJoin = pthread_join( threadHandle, NULL ) ) != 0 )
        {
            LogError( ( "Failed to join thread"
                        ",error code = %d",
                        returnJoin ) );
            returnStatus = EXIT_FAILURE;
        }
    }

    return returnStatus;

@pvyawaha
Copy link
Contributor Author

* From the manpage of `pthread_join`. See possible errors code upon return.
RETURN VALUE
       On success, pthread_join() returns 0; on error, it returns an error number.

ERRORS
       EDEADLK
              A deadlock was detected (e.g., two threads tried to join with each other); or thread specifies the calling thread.

       EINVAL thread is not a joinable thread.

       EINVAL Another thread is already waiting to join with this thread.

       ESRCH  No thread with the ID thread could be found.
/**
 * @brief Start OTA demo.
 *
 * @return   EXIT_SUCCESS or EXIT_FAILURE.
 */
static int startOTADemo( void );
static int startOTADemo( void )
{
   ...
    if( returnStatus == EXIT_SUCCESS )
    {
        returnStatus = pthread_join( threadHandle, NULL );

        if( returnStatus != EXIT_SUCCESS )
        {
            LogError( ( "Failed to join thread"
                        ",error code = %d",
                        returnStatus ) );
        }
    }

    return returnStatus;
}
* Since `startOTADemo()` is documented to return either `EXIT_SUCCESS` or `EXIT_FAILURE`, we should not use error code of `pthread_join()` as the returned error code from `startOTADemo()`. In case someone uses the demo code `startOTADemo()` like this.
if( startOTADemo() != EXIT_FAILURE )   /* Error code of pthread_join will have the same as EXIT_SUCCESS */
{
   ....
}
* Proposed fix at the section of `pthread_join()`.
  
  * Declare new variable `returnJoin` to LogError `pthread_join()` error code. Optional if just print failed to join thread.
  * On success of `pthread_join()` compare return `0`. Just like all other pthread API in OTA demo. (I know EXIT_SUCCESS = 0 as well, but does not belong to the same list of error code)
    if( returnStatus == EXIT_SUCCESS )
    {
        if( ( returnJoin = pthread_join( threadHandle, NULL ) ) != 0 )
        {
            LogError( ( "Failed to join thread"
                        ",error code = %d",
                        returnJoin ) );
            returnStatus = EXIT_FAILURE;
        }
    }

    return returnStatus;

Added in commit
0ad0670

@pvyawaha pvyawaha requested a review from divekarshubham March 19, 2021 17:52
@pvyawaha pvyawaha merged commit 307fd77 into aws:main Mar 22, 2021
@vikivivi
Copy link

@pvyawaha Should I raise another issue or you simply make another commit ? You forget to switch LogError to use the new variable returnJoin. See my above proposed fix. There is no point to LogError returnStatus as it will be EXIT_SUCCESS.

diff --git a/demos/ota/ota_demo_core_http/ota_demo_core_http.c b/demos/ota/ota_demo_core_http/ota_demo_core_http.c
index 5e0a8359..2bbb3cdb 100644
--- a/demos/ota/ota_demo_core_http/ota_demo_core_http.c
+++ b/demos/ota/ota_demo_core_http/ota_demo_core_http.c
@@ -2091,13 +2091,13 @@ static int startOTADemo( void )
         returnJoin = pthread_join( threadHandle, NULL );
 
         if( returnJoin != 0 )
         {
             LogError( ( "Failed to join thread"
                         ",error code = %d",
-                        returnStatus ) );
+                        returnJoin ) );
 
             returnStatus = EXIT_FAILURE;
         }
     }
 
     return returnStatus;
diff --git a/demos/ota/ota_demo_core_mqtt/ota_demo_core_mqtt.c b/demos/ota/ota_demo_core_mqtt/ota_demo_core_mqtt.c
index 4d14ebd5..ed43dca9 100644
--- a/demos/ota/ota_demo_core_mqtt/ota_demo_core_mqtt.c
+++ b/demos/ota/ota_demo_core_mqtt/ota_demo_core_mqtt.c
@@ -1638,13 +1638,13 @@ static int startOTADemo( void )
         returnJoin = pthread_join( threadHandle, NULL );
 
         if( returnJoin != 0 )
         {
             LogError( ( "Failed to join thread"
                         ",error code = %d",
-                        returnStatus ) );
+                        returnJoin ) );
 
             returnStatus = EXIT_FAILURE;
         }
     }
 
     return returnStatus;

@pvyawaha
Copy link
Contributor Author

Thank you for pointing it out, I will fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants