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>; 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,

comments inline.

On 2016/11/30 06:11 PM, Zoran Milinkovic wrote:
> 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]
> Sent: den 30 november 2016 06:39
> To: Zoran Milinkovic <zoran.milinko...@ericsson.com>; Hung Duc Nguyen 
> <hung.d.ngu...@dektech.com.au>
> Cc: 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

Reply via email to