Hi,

I have reviewed your fix instead of Hans N.
I have a few comments. See the attached diff file. It can be applied on a clone 
of your review.

Thanks
Lennart

> -----Original Message-----
> From: Hoa Le [mailto:[email protected]]
> Sent: den 26 mars 2018 06:58
> To: Anders Widell <[email protected]>; Hans Nordebäck
> <[email protected]>
> Cc: [email protected]
> Subject: [devel] [PATCH 1/1] mds: Correct timing issues in mdstest [#2798]
> 
> In some bad thread scheduling situations, the API service request
> on the testing thread may be executed before the corresponding
> event being received on the MDS thread. This will lead to
> unexpected behavior of the service request and cause the failure
> of this test case.
> 
> This patch adds a sleep period for mdstest to wait for the expected
> event being received on MDS thread before invoking the testing
> service request.
> ---
>  src/mds/apitest/mdstipc_api.c | 142
> ++++++++++++++++++++++++++++++------------
>  1 file changed, 102 insertions(+), 40 deletions(-)
> 
> diff --git a/src/mds/apitest/mdstipc_api.c b/src/mds/apitest/mdstipc_api.c
> index 669c770..3a6f4ed 100644
> --- a/src/mds/apitest/mdstipc_api.c
> +++ b/src/mds/apitest/mdstipc_api.c
> @@ -1715,6 +1715,7 @@ void tet_svc_subscr_VDEST_10()
>               FAIL = 1;
>       }
>       // Retrieving the events
> +     sleep(1);
>       if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl, 500,
>                                SA_DISPATCH_ALL) != NCSCC_RC_SUCCESS)
> {
>               printf("\nRetrieve servive 500 Fail\n");
> @@ -1735,6 +1736,7 @@ void tet_svc_subscr_VDEST_10()
>               FAIL = 1;
>       }
>       // Retrieving the events
> +     sleep(1);
>       if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl, 500,
>                                SA_DISPATCH_ALL) != NCSCC_RC_SUCCESS)
> {
>               printf("\nFail to retreive events\n");
> @@ -2001,6 +2003,7 @@ void tet_svc_subscr_VDEST_12()
>       }
> 
>       printf("\nAction: Retrieving the events\n");
> +     sleep(1);
>       if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl, 500,
>                                SA_DISPATCH_ALL) != NCSCC_RC_SUCCESS)
> {
>               printf("Retrieve Fail\n");
> @@ -2165,6 +2168,7 @@ void tet_svc_subscr_ADEST_1()
>               FAIL = 1;
>       } else {
>               printf("\nAction: Retrieve only ONE event\n");
> +             sleep(1);
>               if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl, 500,
>                                        SA_DISPATCH_ONE) !=
> NCSCC_RC_SUCCESS) {
>                       printf("Fail, retrieve ONE\n");
> @@ -2492,6 +2496,7 @@ void tet_svc_subscr_ADEST_9()
>               FAIL = 1;
>       } else {
>               printf("\nAction: Retreive three times, third shall fail\n");
> +             sleep(1);
>               if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl, 500,
>                                        SA_DISPATCH_ONE) !=
> NCSCC_RC_SUCCESS) {
>                       printf("\nFail mds_service_retrieve\n");
> @@ -6056,6 +6061,41 @@ void tet_adest_all_rcvr_thread()
>       free(mesg);
>  }
> 
> +uint32_t tet_mds_retrieve_for_change(MDS_HDL mds_hdl, MDS_SVC_ID
> your_scv_id,
> +             SaDispatchFlagsT dispatchFlags, NCSMDS_SVC_ID
> req_svc_id,
> +             MDS_SVC_PVT_SUB_PART_VER svc_pvt_ver, NCSMDS_CHG
> change)
> +{
> +     unsigned int num_tries = 1;
> +     unsigned int max_tries = 30;
> +     uint32_t rc = NCSCC_RC_FAILURE;
> +
> +     while (1) {
> +             if (mds_service_retrieve(mds_hdl, your_scv_id,
> +                             dispatchFlags) != NCSCC_RC_SUCCESS) {
> +                     printf("\nFail mds_service_retrieve\n");
> +                     break;
> +             }
> +             // Verify if the correct service event was retrieved.
> +             if (tet_verify_version(mds_hdl, your_scv_id, req_svc_id,
> +                             svc_pvt_ver, change) == 1) {
> +                     rc = NCSCC_RC_SUCCESS;
> +                     break;
> +             }
> +             if (num_tries > max_tries) {
> +                     printf("\nCould not retrieve the expected event\n");
> +                     break;
> +             }
> +             else {
> +                     printf("\nExpected event was still not retrieved,"
> +                             " try again\n");
> +                     usleep(100000);
> +                     num_tries++;
> +             }
> +     }
> +
> +     return rc;
> +}
> +
>  void tet_send_all_tp_1()
>  {
>       int FAIL = 0;
> @@ -6093,12 +6133,14 @@ void tet_send_all_tp_1()
>               FAIL = 1;
>       }
> 
> -     if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl,
> -                              NCSMDS_SVC_ID_EXTERNAL_MIN,
> -                              SA_DISPATCH_ALL) != NCSCC_RC_SUCCESS)
> {
> +     if (tet_mds_retrieve_for_change(gl_tet_adest.mds_pwe1_hdl,
> +                     NCSMDS_SVC_ID_EXTERNAL_MIN,
> SA_DISPATCH_ALL,
> +                     NCSMDS_SVC_ID_EXTERNAL_MIN, 1,
> +                     NCSMDS_NO_ACTIVE) != NCSCC_RC_SUCCESS) {
>               printf("\nFail\n");
>               FAIL = 1;
>       }
> +
>       /*SND*/
>       printf("\t Sending the message to no active instance\n");
>       if ((mds_just_send(
> @@ -6135,9 +6177,11 @@ void tet_send_all_tp_1()
>               printf("\nFail\n");
>               FAIL = 1;
>       }
> -     if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl,
> -                              NCSMDS_SVC_ID_EXTERNAL_MIN,
> -                              SA_DISPATCH_ALL) != NCSCC_RC_SUCCESS)
> {
> +
> +     if (tet_mds_retrieve_for_change(gl_tet_adest.mds_pwe1_hdl,
> +                     NCSMDS_SVC_ID_EXTERNAL_MIN,
> SA_DISPATCH_ALL,
> +                     NCSMDS_SVC_ID_EXTERNAL_MIN, 1,
> +                     NCSMDS_NEW_ACTIVE) != NCSCC_RC_SUCCESS) {
>               printf("\nFail\n");
>               FAIL = 1;
>       }
> @@ -6261,12 +6305,14 @@ void tet_send_all_tp_2()
>               FAIL = 1;
>       }
> 
> -     if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl,
> -                              NCSMDS_SVC_ID_EXTERNAL_MIN,
> -                              SA_DISPATCH_ALL) != NCSCC_RC_SUCCESS)
> {
> +     if (tet_mds_retrieve_for_change(gl_tet_adest.mds_pwe1_hdl,
> +                     NCSMDS_SVC_ID_EXTERNAL_MIN,
> SA_DISPATCH_ALL,
> +                     NCSMDS_SVC_ID_EXTERNAL_MIN, 1,
> +                     NCSMDS_NO_ACTIVE) != NCSCC_RC_SUCCESS) {
>               printf("\nFail\n");
>               FAIL = 1;
>       }
> +
>       printf("\n Sending the message to no active instance\n");
>       if ((mds_just_send(gl_tet_adest.mds_pwe1_hdl,
>                          NCSMDS_SVC_ID_EXTERNAL_MIN,
> @@ -6282,25 +6328,29 @@ void tet_send_all_tp_2()
>               printf("\nFail\n");
>               FAIL = 1;
>       }
> -     if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl,
> -                              NCSMDS_SVC_ID_EXTERNAL_MIN,
> -                              SA_DISPATCH_ALL) != NCSCC_RC_SUCCESS)
> {
> -             printf("\nFail mds_service_retrieve\n");
> +
> +     if (tet_mds_retrieve_for_change(gl_tet_adest.mds_pwe1_hdl,
> +                     NCSMDS_SVC_ID_EXTERNAL_MIN,
> SA_DISPATCH_ALL,
> +                     NCSMDS_SVC_ID_EXTERNAL_MIN, 1,
> +                     NCSMDS_NEW_ACTIVE) != NCSCC_RC_SUCCESS) {
> +             printf("\nFail\n");
>               FAIL = 1;
>       }
> +
>       /*SNDACK*/
>       printf("\nChange role to standby\n");
>       if (vdest_change_role(200, V_DEST_RL_STANDBY) !=
> NCSCC_RC_SUCCESS) {
>               printf("\nFail\n");
>               FAIL = 1;
>       }
> -     if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl,
> -                              NCSMDS_SVC_ID_EXTERNAL_MIN,
> -                              SA_DISPATCH_ALL) != NCSCC_RC_SUCCESS)
> {
> +
> +     if (tet_mds_retrieve_for_change(gl_tet_adest.mds_pwe1_hdl,
> +                     NCSMDS_SVC_ID_EXTERNAL_MIN,
> SA_DISPATCH_ALL,
> +                     NCSMDS_SVC_ID_EXTERNAL_MIN, 1,
> +                     NCSMDS_NO_ACTIVE) != NCSCC_RC_SUCCESS) {
>               printf("\nFail\n");
>               FAIL = 1;
>       }
> -
>       else
>               printf("\nSuccess\n");
>       /*Create thread to change role*/
> @@ -6337,15 +6387,16 @@ void tet_send_all_tp_2()
>               printf("\nFail\n");
>               FAIL = 1;
>       }
> -     if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl,
> -                              NCSMDS_SVC_ID_EXTERNAL_MIN,
> -                              SA_DISPATCH_ALL) != NCSCC_RC_SUCCESS)
> {
> +     if (tet_mds_retrieve_for_change(gl_tet_adest.mds_pwe1_hdl,
> +                     NCSMDS_SVC_ID_EXTERNAL_MIN,
> SA_DISPATCH_ALL,
> +                     NCSMDS_SVC_ID_EXTERNAL_MIN, 1,
> +                     NCSMDS_NO_ACTIVE) != NCSCC_RC_SUCCESS) {
>               printf("\nFail\n");
>               FAIL = 1;
>       }
> -
>       else
>               printf("\nSuccess\n");
> +
>       if (tet_create_task((NCS_OS_CB)tet_vdest_all_rcvr_thread,
>                           gl_tet_vdest[1].svc[1].task.t_handle) ==
>           NCSCC_RC_SUCCESS) {
> @@ -8977,12 +9028,14 @@ void tet_direct_send_all_tp_5()
>                       FAIL = 1;
>               }
> 
> -             if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl,
> -                                      NCSMDS_SVC_ID_EXTERNAL_MIN,
> -                                      SA_DISPATCH_ALL) !=
> NCSCC_RC_SUCCESS) {
> +             if
> (tet_mds_retrieve_for_change(gl_tet_adest.mds_pwe1_hdl,
> +                             NCSMDS_SVC_ID_EXTERNAL_MIN,
> SA_DISPATCH_ALL,
> +                             NCSMDS_SVC_ID_EXTERNAL_MIN, 1,
> +                             NCSMDS_NO_ACTIVE) !=
> NCSCC_RC_SUCCESS) {
>                       printf("\nFail\n");
>                       FAIL = 1;
>               }
> +
>               printf("\n Sending the message to no active instance\n");
>               if (mds_direct_send_message(
>                       gl_tet_adest.mds_pwe1_hdl,
> NCSMDS_SVC_ID_EXTERNAL_MIN,
> @@ -8999,12 +9052,15 @@ void tet_direct_send_all_tp_5()
>                       printf("\nFail\n");
>                       FAIL = 1;
>               }
> -             if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl,
> -                                      NCSMDS_SVC_ID_EXTERNAL_MIN,
> -                                      SA_DISPATCH_ALL) !=
> NCSCC_RC_SUCCESS) {
> +
> +             if
> (tet_mds_retrieve_for_change(gl_tet_adest.mds_pwe1_hdl,
> +                             NCSMDS_SVC_ID_EXTERNAL_MIN,
> SA_DISPATCH_ALL,
> +                             NCSMDS_SVC_ID_EXTERNAL_MIN, 1,
> +                             NCSMDS_NEW_ACTIVE) !=
> NCSCC_RC_SUCCESS) {
>                       printf("\nFail\n");
>                       FAIL = 1;
>               }
> +
>               /*SNDACK*/
>               printf("\nChange role to standby\n");
>               if (vdest_change_role(200, V_DEST_RL_STANDBY) !=
> @@ -9012,9 +9068,11 @@ void tet_direct_send_all_tp_5()
>                       printf("\nFail\n");
>                       FAIL = 1;
>               }
> -             if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl,
> -                                      NCSMDS_SVC_ID_EXTERNAL_MIN,
> -                                      SA_DISPATCH_ALL) !=
> NCSCC_RC_SUCCESS) {
> +
> +             if
> (tet_mds_retrieve_for_change(gl_tet_adest.mds_pwe1_hdl,
> +                             NCSMDS_SVC_ID_EXTERNAL_MIN,
> SA_DISPATCH_ALL,
> +                             NCSMDS_SVC_ID_EXTERNAL_MIN, 1,
> +                             NCSMDS_NO_ACTIVE) !=
> NCSCC_RC_SUCCESS) {
>                       printf("\nFail\n");
>                       FAIL = 1;
>               }
> @@ -9057,13 +9115,14 @@ void tet_direct_send_all_tp_5()
>                       printf("\nFail\n");
>                       FAIL = 1;
>               }
> -             if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl,
> -                                      NCSMDS_SVC_ID_EXTERNAL_MIN,
> -                                      SA_DISPATCH_ALL) !=
> NCSCC_RC_SUCCESS) {
> +
> +             if
> (tet_mds_retrieve_for_change(gl_tet_adest.mds_pwe1_hdl,
> +                             NCSMDS_SVC_ID_EXTERNAL_MIN,
> SA_DISPATCH_ALL,
> +                             NCSMDS_SVC_ID_EXTERNAL_MIN, 1,
> +                             NCSMDS_NO_ACTIVE) !=
> NCSCC_RC_SUCCESS) {
>                       printf("\nFail\n");
>                       FAIL = 1;
>               }
> -
>               else
>                       printf("\nSuccess\n");
> 
> @@ -9255,9 +9314,10 @@ void tet_direct_send_all_tp_6()
>                       FAIL = 1;
>               }
> 
> -             if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl,
> -                                      NCSMDS_SVC_ID_EXTERNAL_MIN,
> -                                      SA_DISPATCH_ALL) !=
> NCSCC_RC_SUCCESS) {
> +             if
> (tet_mds_retrieve_for_change(gl_tet_adest.mds_pwe1_hdl,
> +                             NCSMDS_SVC_ID_EXTERNAL_MIN,
> SA_DISPATCH_ALL,
> +                             NCSMDS_SVC_ID_EXTERNAL_MIN, 1,
> +                             NCSMDS_NO_ACTIVE) !=
> NCSCC_RC_SUCCESS) {
>                       printf("\nFail\n");
>                       FAIL = 1;
>               }
> @@ -9313,9 +9373,11 @@ void tet_direct_send_all_tp_6()
>                       printf("\nFail\n");
>                       FAIL = 1;
>               }
> -             if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl,
> -                                      NCSMDS_SVC_ID_EXTERNAL_MIN,
> -                                      SA_DISPATCH_ALL) !=
> NCSCC_RC_SUCCESS) {
> +
> +             if
> (tet_mds_retrieve_for_change(gl_tet_adest.mds_pwe1_hdl,
> +                             NCSMDS_SVC_ID_EXTERNAL_MIN,
> SA_DISPATCH_ALL,
> +                             NCSMDS_SVC_ID_EXTERNAL_MIN, 1,
> +                             NCSMDS_NEW_ACTIVE) !=
> NCSCC_RC_SUCCESS) {
>                       printf("\nFail\n");
>                       FAIL = 1;
>               }
> --
> 2.7.4
> 
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Opensaf-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Attachment: mds_2798_elunlen_comments.diff
Description: mds_2798_elunlen_comments.diff

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Reply via email to