Hi,
- The timeout here is the duration for which the API should wait before
returning to the user.
- The patch does not changes the minimum value of 14 seconds. IE. Before
this patch also the default value was 14 seconds.
This value was set based on implementation specific fine tuning,
particularly in large size cluster.
- Even though it is not a practical case of passing 49 days (greater
than or equal to 0x98968000000000) as a timeout value, yes what you say
can be done across all services.
Thanks,
AV.
On 12/18/2015 2:43 PM, Anders Widell wrote:
> 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
