Hi!
I have some questions regarding this change:
1) This change is not backwards compatible. Have you considered to
either bump the minor version of the CKPT API, or to use ELF symbol
versioning, to avoid breaking old applications?
2) Why was the minimum possible time-out increased from 0.1 seconds to
14 seconds? 14 seconds is like an eternity in a real-time system.
3) If the user-specified timeout is less than 14 seconds, the functions
will always fail without even trying. Wouldn't it be better to at least
try to perform the operation within the time limit specified by the
user? The user may have a loop where it keeps retrying the API function
call when the SA_AIS_ERR_TIMEOUT error code is returned. I would expect
such a loop to eventually terminate, especially if the timeout parameter
has a reasonable value (and there are many reasonable timeout values
which are less than 14 seconds).
3) Can't the conversion from nanoseconds represented as a signed 64-bit
integer to a unit of 10 milliseconds stored in an unsigned 32-bit
integer ('uint32_t time_out' in the code below) overflow? I.e. if the
timeout parameter has a value greater than or equal to 0x98968000000000?
/ Anders Widell
On 11/06/2015 06:33 AM, mahesh.va...@oracle.com wrote:
> 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;
> + }
> +
> /* 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;
> + }
> +
> /* 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;
> + }
> +
> /* 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 ) {
> + 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 ) {
> + 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);
> + 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
>
------------------------------------------------------------------------------
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
