Hi Neelakanta, Ack from me when the code is fixed.
Thanks, Zoran From: Neelakanta Reddy [mailto:reddy.neelaka...@oracle.com] Sent: den 1 december 2016 10:35 To: Zoran Milinkovic <zoran.milinko...@ericsson.com>; Hung Duc Nguyen <hung.d.ngu...@dektech.com.au> Cc: opensaf-devel@lists.sourceforge.net Subject: Re: [PATCH 1 of 1] imm:allow augumentCcbInit with ROF as false in completed callback[#1956] V2 Hi zoran, Yes, the code needs to be corrected for IMMA_CALLBACK_OI_CCB_COMPLETED. Thanks, Neel. On 2016/12/01 02:42 PM, Zoran Milinkovic wrote: Hi Hung, Then the code needs to be fixed, and objectName variable can remain in the code. Otherwise I don't see the purpose of the variable. Thanks, Zoran From: Hung Nguyen [mailto:hung.d.ngu...@dektech.com.au] Sent: den 1 december 2016 10:04 To: Zoran Milinkovic <zoran.milinko...@ericsson.com><mailto:zoran.milinko...@ericsson.com>; Neelakanta Reddy <reddy.neelaka...@oracle.com><mailto:reddy.neelaka...@oracle.com> Cc: opensaf-devel@lists.sourceforge.net<mailto:opensaf-devel@lists.sourceforge.net> Subject: Re: [PATCH 1 of 1] imm:allow augumentCcbInit with ROF as false in completed callback[#1956] V2 Hi Zoran, It's still needed, Neel just forgot to pass the 'object' param to immsv_om_augment_ccb_get_admo_name() when it's IMMA_CALLBACK_OI_CCB_COMPLETED. 'cbi->name' is not set (not usable) when it's a CcbCompletedCallback and we have to use 'object' param instead. BR, Hung Nguyen - DEK Technologies -------------------------------------------------------------------------------- From: Zoran Milinkovic zoran.milinko...@ericsson.com<mailto:zoran.milinko...@ericsson.com> Sent: Thursday, December 01, 2016 3:53PM To: Neelakanta Reddy, Hung Nguyen reddy.neelaka...@oracle.com<mailto:reddy.neelaka...@oracle.com>, hung.d.ngu...@dektech.com.au<mailto:hung.d.ngu...@dektech.com.au> Cc: Opensaf-devel opensaf-devel@lists.sourceforge.net<mailto:opensaf-devel@lists.sourceforge.net> Subject: RE: [PATCH 1 of 1] imm:allow augumentCcbInit with ROF as false in completed callback[#1956] V2 Hi Neelakanta, I still don't see where you use the new objectName variable. The only place where the variable is used is: + 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); + } But, if you look at getAdmoName() function, you will see that the third parameter is not used in the function at all, and it implies that objectName is not needed as well. Thanks, Zoran -----Original Message----- From: Neelakanta Reddy [mailto:reddy.neelaka...@oracle.com] Sent: den 1 december 2016 05:56 To: Zoran Milinkovic <zoran.milinko...@ericsson.com><mailto:zoran.milinko...@ericsson.com>; Hung Duc Nguyen <hung.d.ngu...@dektech.com.au><mailto:hung.d.ngu...@dektech.com.au> Cc: opensaf-devel@lists.sourceforge.net<mailto:opensaf-devel@lists.sourceforge.net> Subject: Re: [PATCH 1 of 1] imm:allow augumentCcbInit with ROF as false in completed callback[#1956] V2 Hi Zoran, comments inline. On 2016/11/30 06:11 PM, Zoran Milinkovic wrote: -------------------------------------------------------------------------------- From: Zoran Milinkovic zoran.milinko...@ericsson.com<mailto:zoran.milinko...@ericsson.com> Sent: Thursday, December 01, 2016 3:53PM To: Neelakanta Reddy, Hung Nguyen reddy.neelaka...@oracle.com<mailto:reddy.neelaka...@oracle.com>, hung.d.ngu...@dektech.com.au<mailto:hung.d.ngu...@dektech.com.au> Cc: Opensaf-devel opensaf-devel@lists.sourceforge.net<mailto:opensaf-devel@lists.sourceforge.net> Subject: RE: [PATCH 1 of 1] imm:allow augumentCcbInit with ROF as false in completed callback[#1956] V2 Hi Neelakanta, The new objectName variable is not used in the code. I think here you are referring to variable name. The variable name can be changed to object. I would rather like to see adminOwnerName as some sort of string type than using SaNameT. This is not an issue in the patch, only the suggestion. The reason for this is to try to avoid using SaNameT type as much as possible, and replace it with SaImmAdminOwnerNameT (or other sting type). SaStringT will be used for both object and adminOwnerName Find other comments inline -----Original Message----- From: reddy.neelaka...@oracle.com<mailto:reddy.neelaka...@oracle.com> [mailto:reddy.neelaka...@oracle.com] Sent: den 30 november 2016 06:39 To: Zoran Milinkovic <zoran.milinko...@ericsson.com><mailto:zoran.milinko...@ericsson.com>; Hung Duc Nguyen <hung.d.ngu...@dektech.com.au><mailto:hung.d.ngu...@dektech.com.au> Cc: opensaf-devel@lists.sourceforge.net<mailto: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*/ [Zoran] objectName is not used in the code. It can be removed from the patch The ObjectName will be changed to "SastingT Object". The object variable is required to store the object name in the case of modify/delete ccb operation +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){ + [Zoran] Remove extra lines +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; [Zoran] Move the above line in the IF statement for IMMA_CALLBACK_OI_CCB_CREATE. 'attrValsForCreateUc' is only used for CCB object create callback. will move the code accordingly + +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)); [Zoran] The whole IF block can be removed. 'objectName' is not used. No, we require variable to store the object for CCB modify/delete operation. so, the object will be there. Thanks, Neel. +} +} + 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) [Zoran] 'object' is not used in the function. It can be removed from the function parameter list. Object will be copied from CCB modify/delete operation and the variable is required. { 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; [Zoran] Remove spaces in the line above and align the line with the correct indent Thanks, Zoran +} else { +rc = getAdmoName(privateOmHandle, cbi, &(ccb_oi_record->objectName),&admName); +} + 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
