Hi Hung, comments inline.
On 2016/12/01 03:07 PM, Hung Nguyen wrote: > Hi Neel, > > Besides Zoran's comment, I have 2 more comments inline. > > BR, > Hung Nguyen - DEK Technologies > > -------------------------------------------------------------------------------- > From: Neelakanta reddyreddy.neelaka...@oracle.com > Sent: Wednesday, November 30, 2016 12:39PM > To: Zoran Milinkovic, Hung Nguyen > zoran.milinko...@ericsson.com,hung.d.ngu...@dektech.com.au > Cc: Opensaf-devel > opensaf-devel@lists.sourceforge.net > Subject: [PATCH 1 of 1] imm:allow augumentCcbInit with ROF as false in > completed callback[#1956] V2 > > > osaf/libs/agents/saf/imma/imma_cb.h | 5 + > osaf/libs/agents/saf/imma/imma_db.c | 99 > +++++++++++++++++++++++++++++++++ > osaf/libs/agents/saf/imma/imma_oi_api.c | 60 +++++++------------ > osaf/libs/agents/saf/imma/imma_proc.c | 64 ++------------------- > 4 files changed, 133 insertions(+), 95 deletions(-) > > > Two new variables objectName and adminOwnerName added to imma_oi_ccb_record. > For CCB create operation adminOwnerName will be added. > For CCB delete/modify operation objectName will be added > > diff --git a/osaf/libs/agents/saf/imma/imma_cb.h > b/osaf/libs/agents/saf/imma/imma_cb.h > --- a/osaf/libs/agents/saf/imma/imma_cb.h > +++ b/osaf/libs/agents/saf/imma/imma_cb.h > @@ -36,6 +36,10 @@ struct imma_oi_ccb_record { > struct imma_callback_info* ccbCallback; > SaImmHandleT privateAugOmHandle; > SaImmAdminOwnerHandleT privateAoHandle; > + SaNameT objectName;/* The Object name from the modif/delete ccb > operationi. The objectName is used to obtain > + adminOnerName when saImmOiAugmentCcbInitialize is > called in completed-callback*/ > + SaNameT adminOwnerName; /* adminowner name of the ccb id, assigned at > ccb-create-callback.The adminOwnerName is used > + saImmOiAugmentCcbInitialize is called in > completed-callback*/ > }; > > typedef struct imma_client_node { > @@ -197,6 +201,7 @@ int imma_oi_ccb_record_set_critical(IMMA > int imma_oi_ccb_record_terminate(IMMA_CLIENT_NODE *cl_node, SaImmOiCcbIdT > ccbId); > int imma_oi_ccb_record_abort(IMMA_CLIENT_NODE *cl_node, SaImmOiCcbIdT > ccbId); > int imma_oi_ccb_record_exists(IMMA_CLIENT_NODE *cl_node, SaImmOiCcbIdT > ccbId); > +struct imma_oi_ccb_record * imma_oi_ccb_record_find(IMMA_CLIENT_NODE > *cl_node, SaImmOiCcbIdT ccbId); > int imma_oi_ccb_record_set_error(IMMA_CLIENT_NODE *cl_node, SaImmOiCcbIdT > ccbId, const SaStringT errorString); > SaStringT imma_oi_ccb_record_get_error(IMMA_CLIENT_NODE *cl_node, > SaImmOiCcbIdT ccbId); > void imma_oi_ccb_allow_error_string(IMMA_CLIENT_NODE *cl_node, > SaImmOiCcbIdT ccbId); > diff --git a/osaf/libs/agents/saf/imma/imma_db.c > b/osaf/libs/agents/saf/imma/imma_db.c > --- a/osaf/libs/agents/saf/imma/imma_db.c > +++ b/osaf/libs/agents/saf/imma/imma_db.c > @@ -24,6 +24,8 @@ > > *****************************************************************************/ > > #include "imma.h" > +#include "osaf_extended_name.h" > + > > /**************************************************************************** > Name : imma_client_tree_init > Description : This routine is used to initialize the client tree > @@ -273,6 +275,9 @@ int imma_oi_ccb_record_delete(IMMA_CLIEN > to_delete->mCcbErrorString = NULL; > } > > + osaf_extended_name_free(&(to_delete->objectName)); > + osaf_extended_name_free(&(to_delete->adminOwnerName)); > + > free(to_delete); > TRACE_LEAVE(); > return 1; > @@ -541,6 +546,11 @@ int imma_oi_ccb_record_note_callback(IMM > */ > > int rs = 0; > + const SaImmAttrNameT admoNameAttr = SA_IMM_ATTR_ADMIN_OWNER_NAME; > + SaImmAttrValuesT_2 **attr, **attributes = NULL; > + SaImmAttrValuesT_2 *attrVal = NULL; > + size_t attrDataSize = 0; > + > struct imma_oi_ccb_record *tmp = imma_oi_ccb_record_find(cl_node, > ccbId); > > if(tmp && !(tmp->isAborted)) { > @@ -553,7 +563,96 @@ int imma_oi_ccb_record_note_callback(IMM > rs = 1; > } > } > + if(callback){ > > + > + int noOfAttributes = 0; > + > + /* NOTE: The code below is practically a copy of the code > + in immOm searchNext, for serving the attrValues structure. > + This code should be factored out into some common function. > + */ > + IMMSV_ATTR_VALUES_LIST *p = callback->attrValues; > + int i = 0; > + while (p) { > + ++noOfAttributes; > + p = p->next; > + } > + > + p = callback->attrValues; > + > + > + if(cl_node->isImmA2fCbk) { > + if(noOfAttributes) { > + p = p->next; // Skip RDN. RDN is the first > attribute > + noOfAttributes--; > + } > + } > + > + attrDataSize = sizeof(SaImmAttrValuesT_2 *) * (noOfAttributes + > 1); > + attr = calloc(1, attrDataSize); /*alloc-1 */ > + for (; i < noOfAttributes && p; i++, p = p->next) { > + IMMSV_ATTR_VALUES *q = &(p->n); > + attr[i] = calloc(1, sizeof(SaImmAttrValuesT_2)); > /*alloc-2 */ > + attr[i]->attrName = malloc(q->attrName.size + 1); > /*alloc-3 */ > + strncpy(attr[i]->attrName, (const char > *)q->attrName.buf, q->attrName.size + 1); > + attr[i]->attrName[q->attrName.size] = 0; > /*redundant. */ > + attr[i]->attrValuesNumber = q->attrValuesNumber; > + attr[i]->attrValueType = > (SaImmValueTypeT)q->attrValueType; > + if (q->attrValuesNumber) { > + attr[i]->attrValues = > + calloc(q->attrValuesNumber, > sizeof(SaImmAttrValueT)); /*alloc-4 */ > + /*alloc-5 */ > + attr[i]->attrValues[0] = > + imma_copyAttrValue3(q->attrValueType, > &(q->attrValue)); > + > + if (q->attrValuesNumber > 1) { > + int ix; > + IMMSV_EDU_ATTR_VAL_LIST *r = > q->attrMoreValues; > + for (ix = 1; ix < q->attrValuesNumber; > ++ix) { > + osafassert(r); > + attr[i]->attrValues[ix] = > /*alloc-5 */ > + > imma_copyAttrValue3(q->attrValueType, &(r->n)); > + r = r->next; > + }//for > + }//if > + }//if > + }//for > + attr[i] = NULL; /*redundant */ > + > + > + /* Save a copy of the attrvals pointer in the callback to allow > + saImmOiAugmentCcbInitialize to get access to the > ccbCreateContext. > + We cant use callback->attrValues because it is partially > stolen > + from (caused to be incomplete) in the loop above (see > imma_copyAttrValue3). > + */ > + > + callback->attrValsForCreateUc = (const SaImmAttrValuesT_2 > **)attr; > + > + if(callback->type == IMMA_CALLBACK_OI_CCB_CREATE && > osaf_is_extended_name_empty(&(tmp->adminOwnerName))) { > + attributes = (SaImmAttrValuesT_2 **) > callback->attrValsForCreateUc; > + int i=0; > + while((attrVal = attributes[i++]) != NULL) { > + if(strcmp(admoNameAttr, attrVal->attrName)==0) { > + TRACE("Found %s attribute in object > create upcall", admoNameAttr); > + break; > + } > + } > + > + if(!attrVal || strcmp(attrVal->attrName, admoNameAttr) > || (attrVal->attrValuesNumber!=1) || > + (attrVal->attrValueType != > SA_IMM_ATTR_SASTRINGT)) { > + > LOG_ER("imma_oi_ccb_record_note_callback:Missmatch on attribute %s", > admoNameAttr); > + abort(); > + } > + osaf_extended_name_alloc(*(SaStringT*) > attrVal->attrValues[0], &(tmp->adminOwnerName)); > + > + } else if (((callback->type == IMMA_CALLBACK_OI_CCB_DELETE) || > (callback->type == IMMA_CALLBACK_OI_CCB_MODIFY)) > + && > osaf_is_extended_name_empty(&(tmp->objectName))){ > + SaConstStringT tmpStr = > osaf_extended_name_borrow(&(callback->name)); > + osaf_extended_name_alloc(tmpStr, &(tmp->objectName)); > + } > + } > + > > [Hung] If it's IMMA_CALLBACK_OI_CCB_COMPLETED, we can update 'callback->name' > with 'tmp->objectName' right here. > That way, we can avoid adding extra param 'object' to getAdmoName(). > In getAdmoName(), 'cbi->name' will be always usable (even when it's a > CcbCompleted callback). It increases, code readability if we add extra parameter, instead of confusing. > > return rs; > } > > diff --git a/osaf/libs/agents/saf/imma/imma_oi_api.c > b/osaf/libs/agents/saf/imma/imma_oi_api.c > --- a/osaf/libs/agents/saf/imma/imma_oi_api.c > +++ b/osaf/libs/agents/saf/imma/imma_oi_api.c > @@ -3565,46 +3565,25 @@ extern void immsv_om_handle_finalize( > SaImmHandleT privateOmHandle) > __attribute__((weak)); > > static SaAisErrorT > -getAdmoName(SaImmHandleT privateOmHandle, IMMA_CALLBACK_INFO * cbi, SaNameT* > admoNameOut) > +getAdmoName(SaImmHandleT privateOmHandle, IMMA_CALLBACK_INFO * cbi, SaNameT* > object, SaNameT* admoNameOut) > { > SaAisErrorT rc = SA_AIS_OK; > const SaImmAttrNameT admoNameAttr = SA_IMM_ATTR_ADMIN_OWNER_NAME; > - SaImmAttrValuesT_2 **attributes = NULL; > - SaImmAttrValuesT_2 *attrVal = NULL; > + > TRACE_ENTER(); > > - if(cbi->type == IMMA_CALLBACK_OI_CCB_CREATE) { > - /* Admo attribute for created object should be in attr list.*/ > - int i=0; > - attributes = (SaImmAttrValuesT_2 **) cbi->attrValsForCreateUc; > - while((attrVal = attributes[i++]) != NULL) { > - if(strcmp(admoNameAttr, attrVal->attrName)==0) { > - TRACE("Found %s attribute in object create upcall", > admoNameAttr); > - break; > - } > - } > - > - if(!attrVal || strcmp(attrVal->attrName, admoNameAttr) || > (attrVal->attrValuesNumber!=1) || > - (attrVal->attrValueType != SA_IMM_ATTR_SASTRINGT)) { > - LOG_ER("Missmatch on attribute %s", admoNameAttr); > - abort(); > - } > - > - osaf_extended_name_alloc(*(SaStringT*) attrVal->attrValues[0], > admoNameOut); > - > - } else { > - /* modify or delete => fetch admo attribute for object from server. > */ > - if((cbi->type != IMMA_CALLBACK_OI_CCB_DELETE) && > - (cbi->type != IMMA_CALLBACK_OI_CCB_MODIFY)) { > - LOG_ER("Inconsistency in callback type:%u", cbi->type); > - abort(); > - } > - > - osafassert(immsv_om_augment_ccb_get_admo_name); > - rc = immsv_om_augment_ccb_get_admo_name(privateOmHandle, > &(cbi->name), admoNameOut); > - if(rc == SA_AIS_ERR_LIBRARY) { > - LOG_ER("Missmatch on attribute %s for delete or modify", > admoNameAttr); > - } > + /* modify or delete => fetch admo attribute for object from server. */ > + if((cbi->type != IMMA_CALLBACK_OI_CCB_DELETE) && > + (cbi->type != IMMA_CALLBACK_OI_CCB_MODIFY) && > + (cbi->type != IMMA_CALLBACK_OI_CCB_COMPLETED)) { > + LOG_ER("Inconsistency in callback type:%u", cbi->type); > + abort(); > + } > + > + osafassert(immsv_om_augment_ccb_get_admo_name); > + rc = immsv_om_augment_ccb_get_admo_name(privateOmHandle, &(cbi->name), > admoNameOut); > + if(rc == SA_AIS_ERR_LIBRARY) { > + LOG_ER("Missmatch on attribute %s for delete or modify", > admoNameAttr); > } > > /* attrVal found either in create callback, or fetched from server. */ > @@ -3734,6 +3713,7 @@ SaAisErrorT saImmOiAugmentCcbInitialize( > goto done; > } > > + struct imma_oi_ccb_record *ccb_oi_record = > imma_oi_ccb_record_find(cl_node, ccbId); > cbi = imma_oi_ccb_record_ok_augment(cl_node, ccbId, &privateOmHandle, > &privateAoHandle); > if(!cbi) { > TRACE_2("ERR_BAD_OPERATION: Ccb %u, is not in a state that " > @@ -3848,11 +3828,17 @@ SaAisErrorT saImmOiAugmentCcbInitialize( > > if(!privateAoHandle) { > TRACE("AugCcbinit: Admo has ReleaseOnFinalize FALSE " > - "=> init separate admo => must fetch admo-name > first"); > + "=> init separate admo => must fetch > admo-name first"); > osafassert(locked == false); > SaNameT admName;/* Used to get admo string name copied > to stack.*/ > > - rc = getAdmoName(privateOmHandle, cbi, &admName); > + if((cbi->type == IMMA_CALLBACK_OI_CCB_CREATE)|| > ((cbi->type == IMMA_CALLBACK_OI_CCB_COMPLETED) && > + > !osaf_is_extended_name_empty(&(ccb_oi_record->adminOwnerName)))){ > + admName = ccb_oi_record->adminOwnerName; > + } else { > + rc = getAdmoName(privateOmHandle, cbi, > &(ccb_oi_record->objectName),&admName); > + } > + > > [Hung] In case we already have a CcbCreate callback before the CcbMod/Del > callback (a ccb can have many operations), > we already got the admo name, we don't have to use the objectName to retrieve > the admo name from the server. > > So I think the if statement should be > > if (!osaf_is_extended_name_empty(&(ccb_oi_record->adminOwnerName))) { > admName = ccb_oi_record->adminOwnerName; > } else { > rc = getAdmoName(...); > } > yes, and similar logic can be applied in the code above for imma_oi_ccb_record_note_callback. Thanks, Neel. > if(rc != SA_AIS_OK) { > TRACE("ERR_TRY_AGAIN: failed to obtain > SaImmAttrAdminOwnerName %u", rc); > if(rc != SA_AIS_ERR_TRY_AGAIN) { > diff --git a/osaf/libs/agents/saf/imma/imma_proc.c > b/osaf/libs/agents/saf/imma/imma_proc.c > --- a/osaf/libs/agents/saf/imma/imma_proc.c > +++ b/osaf/libs/agents/saf/imma/imma_proc.c > @@ -2285,7 +2285,6 @@ static bool imma_process_callback_info(I > IMMSV_EVT ccbObjCrRpl; > bool locked = false; > SaImmAttrValuesT_2 **attr = NULL; > - size_t attrDataSize = 0; > int i = 0; > /* No need to check for > o.iCallbkA2f.saImmOiCcbObjectCreateCallback > * "o" is union and > o.iCallbk.saImmOiCcbObjectCreateCallback is same > @@ -2308,62 +2307,11 @@ static bool imma_process_callback_info(I > > const SaImmClassNameT className = > callback->className; /*0 */ > callback->className = NULL; > - int noOfAttributes = 0; > - > - /* NOTE: The code below is practically > a copy of the code > - in immOm searchNext, for serving the > attrValues structure. > - This code should be factored out > into some common function. > - */ > - IMMSV_ATTR_VALUES_LIST *p = > callback->attrValues; > - while (p) { > - ++noOfAttributes; > - p = p->next; > - } > - > - p = callback->attrValues; > - > - if(cl_node->isImmA2fCbk) { > - if(noOfAttributes) { > - p = p->next; // Skip > RDN. RDN is the first attribute > - noOfAttributes--; > - } else { > - /* There must be at > least one attribute value */ > - localEr = > SA_AIS_ERR_BAD_OPERATION; > - clientCapable = false; > - } > - } > - > - if(localEr == SA_AIS_OK) { > - attrDataSize = > sizeof(SaImmAttrValuesT_2 *) * (noOfAttributes + 1); > - attr = calloc(1, attrDataSize); > /*alloc-1 */ > - for (; i < noOfAttributes && p; > i++, p = p->next) { > - IMMSV_ATTR_VALUES *q = > &(p->n); > - attr[i] = calloc(1, > sizeof(SaImmAttrValuesT_2)); /*alloc-2 */ > - attr[i]->attrName = > malloc(q->attrName.size + 1); /*alloc-3 */ > - > strncpy(attr[i]->attrName, (const char *)q->attrName.buf, q->attrName.size + > 1); > - > attr[i]->attrName[q->attrName.size] = 0; /*redundant. */ > - > attr[i]->attrValuesNumber = q->attrValuesNumber; > - attr[i]->attrValueType > = (SaImmValueTypeT)q->attrValueType; > - if > (q->attrValuesNumber) { > - > attr[i]->attrValues = > - > calloc(q->attrValuesNumber, sizeof(SaImmAttrValueT)); /*alloc-4 */ > - /*alloc-5 */ > - > attr[i]->attrValues[0] = > - > imma_copyAttrValue3(q->attrValueType, &(q->attrValue)); > - > - if > (q->attrValuesNumber > 1) { > - int ix; > - > IMMSV_EDU_ATTR_VAL_LIST *r = q->attrMoreValues; > - for (ix > = 1; ix < q->attrValuesNumber; ++ix) { > - > osafassert(r); > - > attr[i]->attrValues[ix] = /*alloc-5 */ > - > imma_copyAttrValue3(q->attrValueType, &(r->n)); > - > r = r->next; > - }//for > - }//if > - }//if > - }//for > - attr[i] = NULL; /*redundant */ > + > + if(cl_node->isImmA2fCbk && > !callback->attrValsForCreateUc) { > + /* There must be at least one > attribute value */ > + localEr = > SA_AIS_ERR_BAD_OPERATION; > + clientCapable = false; > } > > if(localEr == SA_AIS_OK && > cl_node->isImmA2fCbk) { > @@ -2388,7 +2336,6 @@ static bool imma_process_callback_info(I > We cant use callback->attrValues > because it is partially stolen > from (caused to be incomplete) in > the loop above (see imma_copyAttrValue3). > */ > - callback->attrValsForCreateUc = (const > SaImmAttrValuesT_2 **)attr; > > if (localEr == SA_AIS_OK > && > !osaf_is_extended_names_enabled() > @@ -2457,6 +2404,7 @@ static bool imma_process_callback_info(I > > free(className); /*free-0 */ > free(objectName); /*free-6 */ > + attr = (SaImmAttrValuesT_2 **) > callback->attrValsForCreateUc; > for (i = 0; attr[i]; ++i) { > free(attr[i]->attrName); > /*free-3 */ > attr[i]->attrName = 0; > ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel