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>; Neelakanta Reddy 
<reddy.neelaka...@oracle.com>
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,



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

Reply via email to