Hi Thanh,
ACK with minor comments inline.
Best Regards,
ThuanTr
-----Original Message-----
From: Thanh Nguyen <thanh.ngu...@dektech.com.au>
Sent: Wednesday, September 2, 2020 10:49 AM
To: Thang Duc Nguyen <thang.d.ngu...@dektech.com.au>; Minh Hon Chau
<minh.c...@dektech.com.au>; Thuan Tran <thuan.t...@dektech.com.au>
Cc: opensaf-devel@lists.sourceforge.net; Thanh Nguyen
<thanh.ngu...@dektech.com.au>
Subject: [PATCH 1/1] imm: Enhance deep clone of Imm Attr values in immutil
[#3215]
1) Reuse existing functionality for deep clone of Imm Attr values.
2) Publish immutil internal memory management for external use
---
src/amf/amfd/imm.cc | 15 ++-
src/amf/amfd/imm.h | 8 +-
src/ntf/ntfimcnd/ntfimcn_imm.c | 50 +--------
src/ntf/ntfimcnd/ntfimcn_imm.h | 18 ----
src/osaf/immutil/immutil.c | 190 ++++++++++++---------------------
src/osaf/immutil/immutil.h | 48 ++++++---
6 files changed, 127 insertions(+), 202 deletions(-)
diff --git a/src/amf/amfd/imm.cc b/src/amf/amfd/imm.cc
index 826e90d41..b2a17a10c 100644
--- a/src/amf/amfd/imm.cc
+++ b/src/amf/amfd/imm.cc
@@ -206,9 +206,19 @@ done:
return res;
}
+const size_t ImmObjCreate::INIT_MEMREF_SIZE;
+
+ImmObjCreate::ImmObjCreate() :
+ className_(NULL),
+ parentName_(),
+ attrValues_(NULL),
+ memRef(NULL)
+{
+ memRef = immutil_getMem(INIT_MEMREF_SIZE);
+}
//
ImmObjCreate::~ImmObjCreate() {
- immutil_freeSaImmAttrValuesT(attrValues_);
+ immutil_freeMem(memRef);
delete[] className_;
}
@@ -1862,7 +1872,8 @@ void avd_saImmOiRtObjectCreate(const std::string
&className,
ajob->className_ = StrDup(className.c_str());
osafassert(ajob->className_ != nullptr);
ajob->parentName_ = parentName;
- ajob->attrValues_ = immutil_dupSaImmAttrValuesT(attrValues);
+ ajob->attrValues_ = immutil_dupSaImmAttrValuesT_array(ajob->memRef,
+ attrValues);
Fifo::queue(ajob);
TRACE_LEAVE();
diff --git a/src/amf/amfd/imm.h b/src/amf/amfd/imm.h
index 99d47d313..797d7ac82 100644
--- a/src/amf/amfd/imm.h
+++ b/src/amf/amfd/imm.h
@@ -84,12 +84,18 @@ class ImmObjCreate : public ImmJob {
SaImmClassNameT className_;
std::string parentName_;
SaImmAttrValuesT_2 **attrValues_;
+ // Memory used to deep clone SaImmAttrValuesT_2**
+ // memRef must be allocated by immutil_getMem() and free by immutil_freeMem()
+ void* memRef;
- ImmObjCreate() : ImmJob(){};
+ ImmObjCreate();
bool immobj_update_required();
AvdJobDequeueResultT exec(AVD_CL_CB *cb);
~ImmObjCreate();
+
+ private:
+ static const size_t INIT_MEMREF_SIZE = 512;
};
//
diff --git a/src/ntf/ntfimcnd/ntfimcn_imm.c b/src/ntf/ntfimcnd/ntfimcn_imm.c
index 2e515e22b..2d04a8e2b 100644
--- a/src/ntf/ntfimcnd/ntfimcn_imm.c
+++ b/src/ntf/ntfimcnd/ntfimcn_imm.c
@@ -37,7 +37,6 @@
#include <saImmOm.h>
#include <saImmOi.h>
-#include "osaf/immutil/immutil.h"
#include "ntfimcn_main.h"
#include "ntfimcn_notifier.h"
@@ -288,14 +287,6 @@ static void free_ccb_data(CcbUtilCcbData_t *ccb_data) {
if (ccb_data != NULL) {
if (ccb_data->userData != NULL) {
osaf_extended_name_free(ccb_data->userData);
- free(ccb_data->userData);
- }
- // Free userData in CcbUtilOperationData
- struct CcbUtilOperationData* oper_data =
- ccb_data->operationListHead;
- for (; oper_data!= NULL; oper_data = oper_data->next) {
- immutil_freeSaImmAttrValuesT((SaImmAttrValuesT_2**)
- oper_data->userData);
}
ccbutil_deleteCcbData(ccb_data);
}
@@ -578,7 +569,8 @@ saImmOiCcbObjectModifyCallback(SaImmOiHandleT immOiHandle,
SaImmOiCcbIdT ccbId,
struct CcbUtilOperationData *ccbOperData;
ccbOperData = ccbUtilCcbData->operationListTail;
SaImmAttrValuesT_2 **curAttr;
- rc = get_current_attrs(objectName, attrMods, &curAttr);
+ rc = immutil_get_currentAttributes(ccbUtilCcbData->memref,
+ objectName, attrMods, &curAttr);
if (SA_AIS_OK == rc) {
ccbOperData->userData = curAttr;
} else {
@@ -999,41 +991,3 @@ static void finalizeImmOmHandle(SaImmHandleT immOmHandle) {
saf_error(ais_rc));
}
}
-
-SaAisErrorT get_current_attrs(const SaNameT *objectName,
- const SaImmAttrModificationT_2 **attrMods,
- SaImmAttrValuesT_2 ***curAttr) {
- TRACE_ENTER();
- SaAisErrorT rc = SA_AIS_OK;
- // There is no new attribute modifications
- if (attrMods == NULL) {
- *curAttr = NULL;
- return SA_AIS_ERR_INVALID_PARAM;
- }
- int len;
- for (len = 0; attrMods[len] != NULL; ++len) ;
- SaImmAttrNameT *attrNames = calloc((len + 1), sizeof(SaImmAttrNameT));
- if (attrNames == NULL) {
- *curAttr = NULL;
- return SA_AIS_ERR_NO_MEMORY;
- }
- for (int i = 0; i < len; ++i) {
- attrNames[i] = attrMods[i]->modAttr.attrName;
- }
- attrNames[len] = NULL;
- // Get current attributes for the given attribute names
- SaImmAttrValuesT_2 **resAttr;
- rc = immutil_saImmOmAccessorGet_2(ntfimcn_cb.immAccessorHandle,
- objectName, attrNames, &resAttr);
- if (SA_AIS_OK == rc) {
- *curAttr = immutil_dupSaImmAttrValuesT(
- (const SaImmAttrValuesT_2**) resAttr);
- }
- else {
- TRACE("immutil_saImmOmAccessorGet_2 failed rc = %u", rc);
- *curAttr = NULL;
- }
- free(attrNames);
- TRACE_LEAVE2("rc = %u", rc);
- return rc;
-}
diff --git a/src/ntf/ntfimcnd/ntfimcn_imm.h b/src/ntf/ntfimcnd/ntfimcn_imm.h
index 8945d3096..1c6547d89 100644
--- a/src/ntf/ntfimcnd/ntfimcn_imm.h
+++ b/src/ntf/ntfimcnd/ntfimcn_imm.h
@@ -20,8 +20,6 @@
#define NTF_NTFIMCND_NTFIMCN_IMM_H_
#include "ntfimcn_main.h"
-#include <stdbool.h>
-
#ifdef __cplusplus
extern "C" {
@@ -38,22 +36,6 @@ extern "C" {
*/
int ntfimcn_imm_init(ntfimcn_cb_t *cb);
-/**
- * For a given array of IMM Attribute Modifications, fetch the
- * corresponding current IMM Attribute Values. The return data
- * curAttr is managed by IMM unless a copy is requested.
- * Deep clone method is used to copy the returned IMM Attribute Values
- *
- * @param *objectName[in]
- * @param **attrMods[in]
- * @param ***curAttr[out]
- * @param copy[in]
- * @return SaAisErrorT
- */
-SaAisErrorT get_current_attrs(const SaNameT *objectName,
- const SaImmAttrModificationT_2 **attrMods,
- SaImmAttrValuesT_2 ***curAttr);
-
#ifdef __cplusplus
}
#endif
diff --git a/src/osaf/immutil/immutil.c b/src/osaf/immutil/immutil.c
index d2c280ca0..310451ff3 100644
--- a/src/osaf/immutil/immutil.c
+++ b/src/osaf/immutil/immutil.c
@@ -1042,6 +1042,20 @@ static void *clistMalloc(struct Chunk *clist, size_t
size)
return chunk->data;
}
+/* ----------------------------------------
+ * Publishing IMM utility memory management
+ */
+
+void* immutil_getMem(size_t size)
+{
+ return newChunk(NULL, size);
+}
+
+void immutil_freeMem(void* mem)
+{
+ deleteClist((struct Chunk*) mem);
+}
+
/* ----------------------------------------------------------------------
* IMM call wrappers; This wrapper interface offloads the burden to handle
* return values and retries for each and every IMM-call. It makes the code
@@ -2622,128 +2636,12 @@ size_t immutil_valueSize(SaImmValueTypeT type)
return valueSize;
}
-static void _copySaImmAttrValuesT(SaImmAttrValuesT_2 **dest,
- const SaImmAttrValuesT_2 *src)
-{
- SaImmAttrValuesT_2 *tdest = (SaImmAttrValuesT_2*) calloc(1,
- sizeof(SaImmAttrValuesT_2));
- tdest->attrName = strdup(src->attrName);
-
- tdest->attrValueType = src->attrValueType;
- tdest->attrValuesNumber = src->attrValuesNumber;
- SaUint32T count = src->attrValuesNumber;
- size_t valSize = immutil_valueSize(src->attrValueType);
- if (src->attrValues == NULL){
- *dest = tdest;
- return;
- }
- tdest->attrValues = (SaImmAttrValueT*) malloc(
- sizeof(SaImmAttrValueT) * count);
-
- for (unsigned int i = 0; i < count; ++i){
- if (src->attrValues[i] == NULL) {
- tdest->attrValues[i] = NULL;
- continue;
- }
- tdest->attrValues[i] = malloc(valSize);
- switch (src->attrValueType) {
- case SA_IMM_ATTR_SASTRINGT: {
- *((SaStringT*) tdest->attrValues[i]) = strdup(
- *((SaStringT*) src->attrValues[i]));
- break;
- }
- case SA_IMM_ATTR_SANAMET: {
- SaNameT *nameSrc = (SaNameT*) src->attrValues[i];
- SaNameT *nameDest = (SaNameT*) tdest->attrValues[i];
- osaf_extended_name_alloc(
- osaf_extended_name_borrow(nameSrc), nameDest);
- break;
- }
- case SA_IMM_ATTR_SAANYT: {
- SaAnyT *anySrc = (SaAnyT*) src->attrValues[i];
- SaAnyT *anyDest = (SaAnyT*) tdest->attrValues[i];
- anyDest->bufferSize = anySrc->bufferSize;
- if (anyDest->bufferSize) {
- anyDest->bufferAddr =
- (SaUint8T*) malloc(anyDest->bufferSize);
- memcpy(anyDest->bufferAddr,
- anySrc->bufferAddr, anyDest->bufferSize);
- } else {
- anyDest->bufferAddr = NULL;
- }
- break;
- }
- default:
- memcpy(tdest->attrValues[i],
- src->attrValues[i], valSize);
- break;
- }
- }
- *dest = tdest;
-}
-
-SaImmAttrValuesT_2** immutil_dupSaImmAttrValuesT(
+SaImmAttrValuesT_2** immutil_dupSaImmAttrValuesT_array(void* memRef,
const SaImmAttrValuesT_2 **src)
{
- // Return if there is no source
- if (src == NULL) {
- TRACE("src is null");
- return NULL;
- }
- // Return if the source is just an array of NULL
- int len = 0;
- while (src[len] != NULL)
- ++len;
- if (!len) {
- TRACE("src length is null");
- return NULL;
- }
- SaImmAttrValuesT_2 **attr =
- (SaImmAttrValuesT_2 **)malloc(
- (len + 1) * sizeof(SaImmAttrValuesT_2*));
- if (attr == NULL) {
- TRACE("Failed to allocate memory");
- return NULL;
- }
- // Deep clone the source to destination
- for (int i = 0; i < len; ++i) {
- _copySaImmAttrValuesT(&attr[i], src[i]);
- }
- attr[len] = NULL;
- return attr;
-}
-
-void immutil_freeSaImmAttrValuesT(SaImmAttrValuesT_2 **attrs)
-{
- if (attrs == NULL)
- return;
- SaImmAttrValuesT_2 *attr;
- SaImmAttrValueT value;
- for (int i = 0; (attr = attrs[i]) != NULL; ++i) {
- free(attr->attrName);
- if (attr->attrValues == NULL) {
- continue;
- }
- for (unsigned int j = 0; j < attr->attrValuesNumber; ++j) {
- value = attr->attrValues[j];
- if (value == NULL) {
- continue;
- }
- if (attr->attrValueType == SA_IMM_ATTR_SASTRINGT) {
- free(*((SaStringT*) value));
- }
- else if (attr->attrValueType == SA_IMM_ATTR_SANAMET) {
- osaf_extended_name_free((SaNameT*) value);
- }
- else if (attr->attrValueType == SA_IMM_ATTR_SAANYT) {
- free(((SaAnyT*) value)->bufferAddr);
- }
- free(value);
- }
- free(attr->attrValues);
- free(attr);
- }
- free(attrs);
+ return (SaImmAttrValuesT_2**) dupSaImmAttrValuesT_array(
+ (struct Chunk*) memRef,
+ src);
}
const SaImmAttrValuesT_2* immutil_findAttrByName(
@@ -2762,3 +2660,55 @@ const SaImmAttrValuesT_2* immutil_findAttrByName(
}
return result;
}
+
+SaAisErrorT immutil_get_currentAttributes(
[Thuan] should rename function like immutil_getCurrentAttrs(...) consistent
with other immutil_xxx functions
+ void* memRef,
+ const SaNameT *objectName,
+ const SaImmAttrModificationT_2 **attrMods,
+ SaImmAttrValuesT_2 ***curAttr)
+{
+ SaAisErrorT rc = SA_AIS_OK;
+ *curAttr = NULL;
+ SaImmAccessorHandleT accessorHandle;
+ if (attrMods == NULL) {
+ return SA_AIS_ERR_INVALID_PARAM;
+ }
+ SaImmHandleT omHandle;
+ SaVersionT localVer = immVersion;
+ if ((rc = immutil_saImmOmInitialize(&omHandle, NULL, &localVer)) !=
+ SA_AIS_OK) {
+ return rc;
+ }
+ if ((rc = immutil_saImmOmAccessorInitialize(omHandle, &accessorHandle))
+ != SA_AIS_OK) {
+ (void)immutil_saImmOmFinalize(omHandle);
+ return rc;
+ }
+ int len;
+ for (len = 0; attrMods[len] != NULL; ++len) ;
+ SaImmAttrNameT *attrNames = clistMalloc(memRef,
+ (len + 1) * sizeof(SaImmAttrNameT));
+ if (attrNames == NULL) {
+ rc = SA_AIS_ERR_NO_MEMORY;
+ goto done;
+ }
+ for (int i = 0; i < len; ++i) {
+ attrNames[i] = attrMods[i]->modAttr.attrName;
+ }
+ attrNames[len] = NULL;
+ SaImmAttrValuesT_2 **resAttr;
+ rc = immutil_saImmOmAccessorGet_2(accessorHandle,
+ objectName, attrNames, &resAttr);
+ if (SA_AIS_OK == rc) {
+ *curAttr = (SaImmAttrValuesT_2**)
+ immutil_dupSaImmAttrValuesT_array(memRef,
+ (const SaImmAttrValuesT_2**) resAttr);
+ }
+ else {
+ *curAttr = NULL;
+ }
+done:
+ (void)immutil_saImmOmAccessorFinalize(accessorHandle);
+ (void)immutil_saImmOmFinalize(omHandle);
+ return rc;
+}
diff --git a/src/osaf/immutil/immutil.h b/src/osaf/immutil/immutil.h
index 7528d8203..0c430b801 100644
--- a/src/osaf/immutil/immutil.h
+++ b/src/osaf/immutil/immutil.h
@@ -190,7 +190,22 @@ EXTERN_C CcbUtilOperationData_t
*ccbutil_getCcbOpDataByDN(SaImmOiCcbIdT id,
* exported in a common library.
*/
/*@{*/
-
+/**
+ * Allocate memory managed by ImmHelpUtils
+ *
+ * @param size[in]
+ *
+ * @return void*
+ */
+EXTERN_C void* immutil_getMem(size_t size);
+/**
+ * Deallocate memory managed by ImmHelpUtils
+ *
+ * @param memRef[in]
+ *
+ * @return void*
+ */
+EXTERN_C void immutil_freeMem(void* memRef);
/**
* Get IMM Attribute Value size base on type
*
@@ -198,23 +213,32 @@ EXTERN_C CcbUtilOperationData_t
*ccbutil_getCcbOpDataByDN(SaImmOiCcbIdT id,
* @return size_t
*/
EXTERN_C size_t immutil_valueSize(SaImmValueTypeT type);
-
/**
- * Deep clone IMM Attribute Values array
+ * For a given array of IMM Attribute Modifications, fetch the
+ * corresponding current IMM Attribute Values. The return data
+ * Deep clone method is used to copy the returned IMM Attribute Values
*
- * @param **src[in]
- * @return SaImmAttrValuesT_2**
+ * @param *memRef[in]
+ * @param *objectName[in]
+ * @param **attrMods[in]
+ * @param ***curAttr[out]
+ * @param copy[in]
+ * @return SaAisErrorT
*/
-EXTERN_C SaImmAttrValuesT_2** immutil_dupSaImmAttrValuesT(
- const SaImmAttrValuesT_2 **src);
+EXTERN_C SaAisErrorT immutil_get_currentAttributes(void* memRef,
+ const SaNameT *objectName, const SaImmAttrModificationT_2 **attrMods,
+ SaImmAttrValuesT_2 ***curAttr);
/**
- * Deallocate memory used for deep cloning IMM Attribute Values
+ * Deep clone IMM Attribute Values array. The memory reference memRef must be
+ * obtained by immutil_getMem()
*
- * @param **attrs[in]
- * @return void
+ * @param *memRef[in]
+ * @param **src[in]
+ * @return SaImmAttrValuesT_2**
*/
-EXTERN_C void immutil_freeSaImmAttrValuesT(SaImmAttrValuesT_2 **attrs);
+EXTERN_C SaImmAttrValuesT_2** immutil_dupSaImmAttrValuesT_array(void* memRef,
+ const SaImmAttrValuesT_2 **src);
/**
* Find attribute values from the given attribute name
@@ -825,6 +849,4 @@ EXTERN_C SaAisErrorT immutil_saImmOmClassDescriptionGet_2(
EXTERN_C SaAisErrorT immutil_saImmOmClassDescriptionMemoryFree_2(
SaImmHandleT immHandle, SaImmAttrDefinitionT_2 **attrDef);
-/*@}*/
-
[Thuan] Should keep this to match with Line /*@{*/
#endif // OSAF_IMMUTIL_IMMUTIL_H_
--
2.25.0
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
