Ack with minor comments inlined.

Thanks
-Nagu

> -----Original Message-----
> From: Venkata Mahesh Alla
> Sent: 06 November 2015 11:04
> To: Nagendra Kumar; Ramesh Babu Betham
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: [PATCH 1 of 1] cpsv: validated SaTimeT timeout argument value for
> all CPA APIs [#1583]
> 
>  osaf/libs/agents/saf/cpa/cpa_api.c |  111
> ++++++++++++++++++++++++++++++------
>  1 files changed, 91 insertions(+), 20 deletions(-)
> 
> 
> If we test upper and lower limits of SaTimeT timeout arguments to CPSV API ,
> some of CPA APIs are doesn't returns or hungs.
> 
> Current cpa code doesn't validating or passing SaTimeT timeout arguments
> to CPSV API ,
> which are below default API timeout limit of CPSV_WAIT_TIME (1400 X
> 10ms) and
> above SaTimeT, type long long int Maximum value 9223372036854775807
> (2^63-1)
> 
> For example :
> 
>   i)     const SaTimeT timeout = 1215752192;
>   ii)     const SaTimeT timeout = 2 * SA_TIME_ONE_MINUTE;
>   iii)     const SaTimeT timeout = 9223372036854775809;
> 
>     saCkptCheckpointOpen(....,   timeout, ...);
> 
> Because of that in some of variables are over flowing and , API calls are not
> working properly
> 
> Fix  :
> 
> 1 -  Validated if (time_out < CPSV_WAIT_TIME) return
> SA_AIS_ERR_INVALID_PARAM
> 2 -  Validates if (expirationTime < 0 ) return SA_AIS_ERR_INVALID_PARAM
> 3 -  Pass proper timeout values to internal functions of Opensaf
> 
> diff --git a/osaf/libs/agents/saf/cpa/cpa_api.c
> b/osaf/libs/agents/saf/cpa/cpa_api.c
> --- a/osaf/libs/agents/saf/cpa/cpa_api.c
> +++ b/osaf/libs/agents/saf/cpa/cpa_api.c
> @@ -27,7 +27,6 @@
> 
> **************************************************************
> ****************/
> 
>  #include "cpa.h"
> -#define NCS_SAF_MIN_ACCEPT_TIME 10
> 
> 
> /*************************************************************
> ***************
>    Name          :  SaCkptInitialize
> @@ -889,6 +888,14 @@ SaAisErrorT saCkptCheckpointOpen(SaCkptH
>                  return SA_AIS_ERR_INVALID_PARAM;
>          }
> 
> +     if (timeout < 0 ) {
> +             TRACE_4("Cpa CkptOpen: failed invalid timeout return
> value:%d,ckptHandle:%llx",
> +                             SA_AIS_ERR_INVALID_PARAM, ckptHandle);
> +             TRACE_LEAVE2("API return code = %u", rc);
> +             return SA_AIS_ERR_INVALID_PARAM;
> +     }
> +
> +
>       /* Draft Validations */
>       rc = cpa_open_attr_validate(checkpointCreationAttributes,
> checkpointOpenFlags, checkpointName);
>       if (rc != SA_AIS_OK) {
> @@ -975,13 +982,13 @@ SaAisErrorT saCkptCheckpointOpen(SaCkptH
>       }
> 
>       evt.info.cpnd.info.openReq.ckpt_flags = checkpointOpenFlags;
> -
>       /* convert the timeout to 10 ms value and add it to the sync send
> timeout */
>       time_out = m_CPSV_CONVERT_SATIME_TEN_MILLI_SEC(timeout);
> 
> -     if (time_out < NCS_SAF_MIN_ACCEPT_TIME) {
> +     if (time_out < CPSV_WAIT_TIME) {
>               rc = SA_AIS_ERR_TIMEOUT;
> -             TRACE_4("Cpa CkptOpen failed Api failed with return
> value:%d,ckptHandle:%llx,timeout:%llu", rc, ckptHandle, timeout);
> +             TRACE_4("Cpa CkptOpen failed Api failed with return
> value:%d,ckptHandle:%llx,timeout:%llu",
> +                             rc, ckptHandle, timeout);
>               goto mds_send_fail;
>       }
> 
> @@ -998,9 +1005,8 @@ SaAisErrorT saCkptCheckpointOpen(SaCkptH
>               TRACE_4("cpa CkptOpen:CPND_DOWN Api failed with return
> value:%d,ckptHandle:%llx", rc, ckptHandle);
>               goto mds_send_fail;
>       }
> -
>         /* Send the evt to CPND */
> -       proc_rc = cpa_mds_msg_sync_send(cb->cpa_mds_hdl, &(cb-
> >cpnd_mds_dest), &evt, &out_evt, timeout);
> +     proc_rc = cpa_mds_msg_sync_send(cb->cpa_mds_hdl, &(cb-
> >cpnd_mds_dest), &evt, &out_evt, time_out);
> 
>         /* Generate rc from proc_rc */
>         switch (proc_rc) {
> @@ -1709,6 +1715,13 @@ SaAisErrorT saCkptCheckpointRetentionDur
>               goto done;
>       }
> 
> +     if (retentionDuration < 0 ) {
> +                TRACE_4("Cpa RetentionDurationSet: failed invalid
> retentionDuration return value:%d,ckptHandle:%llx",
> +                             SA_AIS_ERR_INVALID_PARAM,
> checkpointHandle);
> +                rc = SA_AIS_ERR_INVALID_PARAM;
> +             goto done;
> +        }

[Nagu]: White spaces in above lines.

> +
>       /* get the CB Lock */
>       if (m_NCS_LOCK(&cb->cb_lock, NCS_LOCK_WRITE) !=
> NCSCC_RC_SUCCESS) {
>               rc = SA_AIS_ERR_LIBRARY;
> @@ -2183,6 +2196,7 @@ SaAisErrorT saCkptSectionCreate(SaCkptCh
>       bool gen_sec_flag = false;
>       SaCkptSectionIdT app_ptr;
>       CPA_CLIENT_NODE *cl_node = NULL;
> +     uint32_t time_out;
> 
>       TRACE_ENTER2("SaCkptCheckpointHandleT passed is
> %llx",checkpointHandle);
>       /* Validate the Input Parameters */
> @@ -2312,8 +2326,21 @@ SaAisErrorT saCkptSectionCreate(SaCkptCh
>       evt.info.cpnd.info.sec_creatReq.init_size = initialDataSize;
> 
>       m_NCS_UNLOCK(&cb->cb_lock, NCS_LOCK_WRITE);
> +
> +     time_out = CPA_WAIT_TIME(initialDataSize);
> +
> +     if (time_out < 0 ) {
> +             rc = SA_AIS_ERR_TIMEOUT;
> +             TRACE_4("cpa SectCreate Api failed because of larg
> initialDataSize return :%d,ckptHandle:%llx",
> +                             rc, checkpointHandle);
> +             goto fail1;
> +     }
> +     else if (time_out < CPSV_WAIT_TIME) {
> +             time_out = CPSV_WAIT_TIME;
> +     }
> +
>       proc_rc = cpa_mds_msg_sync_send(cb->cpa_mds_hdl, &(gc_node-
> >active_mds_dest),
> -                                     &evt, &out_evt,
> CPA_WAIT_TIME(initialDataSize));
> +                     &evt, &out_evt, time_out);
> 
>       /* Generate rc from proc_rc */
>       switch (proc_rc) {
> @@ -2684,6 +2711,14 @@ SaAisErrorT saCkptSectionExpirationTimeS
>               return rc;
>       }
> 
> +     if (expirationTime < 0 ) {
> +                TRACE_4("Cpa ExpirationTimeSet: failed invalid expirationTime
> return value:%d,ckptHandle:%llx",
> +                             SA_AIS_ERR_INVALID_PARAM,
> checkpointHandle);
> +                rc = SA_AIS_ERR_INVALID_PARAM;
> +             TRACE_LEAVE2("API return code = %u", rc);
> +             return rc;
> +        }

[Nagu]: White spaces in above lines.

> +
>       /* retrieve CPA CB */
>       m_CPA_RETRIEVE_CB(cb);
>       if (!cb) {
> @@ -2847,6 +2882,14 @@ SaAisErrorT saCkptSectionIterationInitia
>               return rc;
>       }
> 
> +     if (expirationTime < 0 ) {
> +             TRACE_4("Cpa SectionIterationInitialize: failed invalid
> expirationTime return value:%d,ckptHandle:%llx",
> +                             SA_AIS_ERR_INVALID_PARAM,
> checkpointHandle);
> +             rc = SA_AIS_ERR_INVALID_PARAM;
> +             TRACE_LEAVE2("API return code = %u", rc);
> +             return rc;
> +        }

[Nagu]: White spaces in above line.

> +
>       /* retrieve CPA CB */
>       m_CPA_RETRIEVE_CB(cb);
>       if (!cb) {
> @@ -3360,6 +3403,7 @@ SaAisErrorT saCkptCheckpointWrite(SaCkpt
>       CPA_CLIENT_NODE *cl_node = NULL;
>       SaSizeT all_ioVector_size = 0;
>       uint32_t err_flag = 0;
> +     uint32_t time_out;
> 
>       TRACE_ENTER2("SaCkptCheckpointHandleT passed is
> %llx",checkpointHandle);
> 
> @@ -3479,18 +3523,27 @@ SaAisErrorT saCkptCheckpointWrite(SaCkpt
> 
>       /* Unlock cpa_lock before calling mds api */
>       m_NCS_UNLOCK(&cb->cb_lock, NCS_LOCK_WRITE);
> -#if 1
> -     for (iter = 0; iter < numberOfElements; iter++)
> +
> +     for (iter = 0; iter < numberOfElements; iter++) {
>               all_ioVector_size += ioVector[iter].dataSize;
> -
> -     proc_rc = cpa_mds_msg_sync_send(cb->cpa_mds_hdl, &(gc_node-
> >active_mds_dest),
> -                                     &evt, &out_evt,
> CPA_WAIT_TIME(all_ioVector_size));
> -#else
> -     proc_rc = cpa_mds_msg_sync_send(cb->cpa_mds_hdl, &(gc_node-
> >active_mds_dest), &evt, &out_evt, CPSV_WAIT_TIME);
> -#endif
> -   /* Generate rc from proc_rc */
> -   switch (proc_rc)
> -   {
> +             time_out = CPA_WAIT_TIME(all_ioVector_size);
> +
> +             if (time_out < 0 ) {

[Nagu]: time_out is not going to be negative here, so no need of this check.

> +                     rc = SA_AIS_ERR_TIMEOUT;
> +                     TRACE_4("cpa ckpointWrite Api failed because of
> larg ioVector size return :%d,ckptHandle:%llx",
> +                                     rc, checkpointHandle);
> +                     goto done;
> +             }
> +             else if (time_out < CPSV_WAIT_TIME) {
> +                     time_out = CPSV_WAIT_TIME;
> +             }
> +             proc_rc = cpa_mds_msg_sync_send(cb->cpa_mds_hdl,
> &(gc_node->active_mds_dest),
> +                             &evt, &out_evt, time_out);
> +
> +     }
> +     /* Generate rc from proc_rc */
> +     switch (proc_rc)
> +     {
>         case NCSCC_RC_SUCCESS:
>             break;
>         case NCSCC_RC_REQ_TIMOUT:
> @@ -3577,6 +3630,7 @@ SaAisErrorT saCkptSectionOverwrite(SaCkp
>       CPA_GLOBAL_CKPT_NODE *gc_node = NULL;
>       bool add_flag = false;
>       CPA_CLIENT_NODE *cl_node = NULL;
> +     uint32_t time_out;
> 
>       TRACE_ENTER2("SaCkptCheckpointHandleT passed is
> %llx",checkpointHandle);
>       memset(&ckpt_data, '\0', sizeof(CPSV_CKPT_DATA));
> @@ -3684,8 +3738,19 @@ SaAisErrorT saCkptSectionOverwrite(SaCkp
>       /* Unlock cpa_lock before calling mds api */
>       m_NCS_UNLOCK(&cb->cb_lock, NCS_LOCK_WRITE);
> 
> +     time_out = CPA_WAIT_TIME(dataSize);
> +
> +     if (time_out < 0 ) {


[Nagu]: time_out is not going to be negative here, so no need of this check.

> +             rc = SA_AIS_ERR_TIMEOUT;
> +             TRACE_4("cpa SectionOverwrite Api failed because of larg
> dataSize return :%d,ckptHandle:%llx",                                 rc,
> checkpointHandle);
> +             goto done;
> +     }
> +     else if (time_out < CPSV_WAIT_TIME) {
> +             time_out = CPSV_WAIT_TIME;
> +     }
> +
>       proc_rc = cpa_mds_msg_sync_send(cb->cpa_mds_hdl, &(gc_node-
> >active_mds_dest),
> -                                     &evt, &out_evt,
> CPA_WAIT_TIME(dataSize));
> +                     &evt, &out_evt, time_out);
> 
>       /* Generate rc from proc_rc */
>       switch (proc_rc) {
> @@ -4053,6 +4118,12 @@ SaAisErrorT saCkptCheckpointSynchronize(
>       /* For Ckpt Sync CPA will not perform any operation
>          other than passing the request to CPND */
> 
> +     if (timeout < 0 ) {
> +             TRACE_4("Cpa CheckpointSynchronize: failed with invalied
> timeout return value:%d,ckptHandle:%llx",
> SA_AIS_ERR_INVALID_PARAM, checkpointHandle);

[Nagu]: White space in above lines.

> +             rc =SA_AIS_ERR_INVALID_PARAM;
> +             goto done;
> +     }
> +
>       /* retrieve CPA CB */
>       m_CPA_RETRIEVE_CB(cb);
>       if (!cb) {
> @@ -4139,7 +4210,7 @@ SaAisErrorT saCkptCheckpointSynchronize(
>       /* Convert the time from saTimeT to millisecs */
>       timeout = m_CPSV_CONVERT_SATIME_TEN_MILLI_SEC(timeout);
> 
> -     if (timeout < NCS_SAF_MIN_ACCEPT_TIME) {
> +        if (timeout < CPSV_WAIT_TIME) {
>               rc = SA_AIS_ERR_TIMEOUT;
>               TRACE_4("cpa CkptSynchronize Api failed with return
> value:%d,ckptHandle:%llx ", rc, checkpointHandle);
>               goto fail1;

------------------------------------------------------------------------------
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Reply via email to