Hi Lennart,

Thanks for your comments on the ticket, I will update the description of tet_mds_retrieve_for_change() as your suggestion in the next version of the patch.

Just have a concern with the comment about sleep(1) in test case tet_svc_subscr_VDEST_10 (and other). I agree that we should restrict sleep() in mdstest. But in these particular test cases, I think sleep() is a suitable approach.

I considered two other alternative approaches for this issue as following, but they do not seem to be the most fitting options for this.

1) Use tet_mds_retrieve_for_change() (or similar method) to retrieve the event. The problem is that the tet_mds_retrieve_for_change() does the same thing as the next steps of this test case (invokes mds_service_retrieve() then verifies the event by calling tet_verify_version()). So if we use tet_mds_retrieve_for_change() (or any similar method) here, these next steps of the test case seem to be meaningless.

2) Start a thread to poll on the selection object of the listening service for its events. I think this is not a good solution either, since we may catch the wrong event: "The first service up after successfully subscribing" instead of "the role change event". To clarify these events, we may have to reuse the tet_mds_retrieve_for_change() fuction.

Another possible approach is retrying the sequence mds_service_retrieve/tet_verify_version several times like in the following but technically, it is the same with the sleep() method.

/int num_retries = 0;//
//while (1) {//
//    if (mds_service_retrieve(gl_tet_adest.mds_pwe1_hdl, 500, //SA_DISPATCH_ALL) != NCSCC_RC_SUCCESS) {//
//        printf("\nFail to retreive events\n");//
//        FAIL = 1;//
//        break;//
//    }//
//    if ((tet_verify_version(gl_tet_adest.mds_pwe1_hdl, 500, 600, 1, //NCSMDS_NO_ACTIVE)) != 1 ||// //        (tet_verify_version(gl_tet_adest.mds_pwe1_hdl, 500, 700, 2, //NCSMDS_NO_ACTIVE) != 1)) {//
//        if (num_retries >= 3) {//
//            printf(//"\nFail to verifying for the versions for NO_ACTIVE event\n");//
//            FAIL = 1;//
//            break;//
//        } else {//
//            num_retries ++;//
//            sleep(1);//
//        }//
//    } else {//
//        break;//
//    }//
//}/

That was why I have chosen the sleep(1) approach in these test cases. What do you think about it?

--
Best regards,
Hoa Le

On 04/19/2018 09:34 PM, Lennart Lund wrote:
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:hoa...@dektech.com.au]
Sent: den 26 mars 2018 06:58
To: Anders Widell <anders.wid...@ericsson.com>; Hans Nordebäck
<hans.nordeb...@ericsson.com>
Cc: opensaf-devel@lists.sourceforge.net
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
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

------------------------------------------------------------------------------
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
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Reply via email to