Please find comments inline Thanks, Minh On 7/4/2014 9:48 PM, Zoran Milinkovic wrote: > Hi, > > Find my comments inline started with [Zoran]. > > > -----Original Message----- > From: Minh Hon Chau [mailto:minh.c...@dektech.com.au] > Sent: den 2 juli 2014 07:50 > To: Anders Widell; mathi.naic...@oracle.com; Hans Feldt; > praveen.malv...@oracle.com > Cc: opensaf-devel@lists.sourceforge.net > Subject: [devel] [PATCH 2 of 5] NTF: Adapt NTF common library to support long > DNs [#873] > > osaf/libs/common/ntfsv/Makefile.am | 1 + > osaf/libs/common/ntfsv/include/ntfsv_mem.h | 7 + > osaf/libs/common/ntfsv/ntfsv_enc_dec.c | 37 ++++-- > osaf/libs/common/ntfsv/ntfsv_mem.c | 167 > +++++++++++++++++++++++++++- > 4 files changed, 191 insertions(+), 21 deletions(-) > > > diff --git a/osaf/libs/common/ntfsv/Makefile.am > b/osaf/libs/common/ntfsv/Makefile.am > --- a/osaf/libs/common/ntfsv/Makefile.am > +++ b/osaf/libs/common/ntfsv/Makefile.am > @@ -23,6 +23,7 @@ SUBDIRS = include > noinst_LTLIBRARIES = libntfsv_common.la > > libntfsv_common_la_CPPFLAGS = \ > + -DSA_EXTENDED_NAME_SOURCE \ > $(AM_CPPFLAGS) \ > -I$(top_srcdir)/osaf/libs/common/ntfsv/include > > diff --git a/osaf/libs/common/ntfsv/include/ntfsv_mem.h > b/osaf/libs/common/ntfsv/include/ntfsv_mem.h > --- a/osaf/libs/common/ntfsv/include/ntfsv_mem.h > +++ b/osaf/libs/common/ntfsv/include/ntfsv_mem.h > @@ -97,6 +97,13 @@ extern "C" { > void > ntfsv_filter_obj_cr_del_free(SaNtfObjectCreateDeleteNotificationFilterT *f); > void ntfsv_filter_attr_ch_free(SaNtfAttributeChangeNotificationFilterT > *f); > > + SaAisErrorT ntfsv_sanamet_copy(SaNameT* pDes, SaNameT* pSrc); > + bool ntfsv_sanamet_is_valid(const SaNameT* pName); > + void ntfsv_sanamet_clone_strptr(SaNameT* pName); > + SaStringT ntfs_sanamet_strdup(SaNameT* pName); > + size_t ntfs_sanamet_length(const SaNameT* pName); > + void ntfs_sanamet_steal(SaStringT value, size_t length, SaNameT* pName); > + void ntfs_sanamet_alloc(SaConstStringT value, size_t length, SaNameT* > pName); > #ifdef __cplusplus > } > #endif > diff --git a/osaf/libs/common/ntfsv/ntfsv_enc_dec.c > b/osaf/libs/common/ntfsv/ntfsv_enc_dec.c > --- a/osaf/libs/common/ntfsv/ntfsv_enc_dec.c > +++ b/osaf/libs/common/ntfsv/ntfsv_enc_dec.c > @@ -18,6 +18,8 @@ > #include <ncsencdec_pub.h> > #include "ntfsv_enc_dec.h" > #include "ntfsv_mem.h" > +#include "osaf_extended_name.h" > +#include "saAis.h" > > typedef union { > uint32_t uint32_val; > @@ -334,19 +336,22 @@ static uint32_t decodeSaNtfAttribute(NCS > static uint32_t encodeSaNameT(NCS_UBAID *uba, uint8_t *p8, SaNameT *name) > { > uint32_t rv; > - > p8 = ncs_enc_reserve_space(uba, 2); > if (!p8) { > TRACE("ncs_enc_reserve_space failed"); > return NCSCC_RC_OUT_OF_MEM; > } > - if (name->length > SA_MAX_NAME_LENGTH) { > - LOG_ER("SaNameT length too long %hd", name->length); > - osafassert(0); > - } > - ncs_encode_16bit(&p8, name->length); > - ncs_enc_claim_space(uba, 2); > - rv = ncs_encode_n_octets_in_uba(uba, name->value, > (uint32_t)name->length); > + > + if (!ntfsv_sanamet_is_valid(name)) { > + LOG_ER("SaNameT is invalid"); > + osafassert(0); > + } > + > + SaConstStringT value = osaf_extended_name_borrow(name); > + size_t length = ntfs_sanamet_length(name); > + ncs_encode_16bit(&p8, length); > + ncs_enc_claim_space(uba, 2); > + rv = ncs_encode_n_octets_in_uba(uba, (uint8_t*) value, (uint32_t) > length); > return rv; > } > > @@ -355,14 +360,22 @@ static uint32_t decodeSaNameT(NCS_UBAID > uint8_t local_data[2]; > uint32_t rv; > p8 = ncs_dec_flatten_space(uba, local_data, 2); > - name->length = ncs_decode_16bit(&p8); > - if (name->length > SA_MAX_NAME_LENGTH) { > - LOG_ER("SaNameT length too long: %hd", name->length); > + size_t length = ncs_decode_16bit(&p8); > + if (length > kMaxDnLength) { > + LOG_ER("SaNameT length too long: %zu", length); > /* this should not happen */ > osafassert(0); > } > ncs_dec_skip_space(uba, 2); > - rv = ncs_decode_n_octets_from_uba(uba, name->value, > (uint32_t)name->length); > + char* value = (char*) malloc(length + 1); > + if (value == NULL) { > + LOG_ER("Out of memory"); > + /* this should not happen */ > + osafassert(0); > + } > + rv = ncs_decode_n_octets_from_uba(uba, (uint8_t*) value, (uint32_t) > length); > + value[length] = '\0'; > + ntfs_sanamet_steal(value, length, name); > return rv; > } > > diff --git a/osaf/libs/common/ntfsv/ntfsv_mem.c > b/osaf/libs/common/ntfsv/ntfsv_mem.c > --- a/osaf/libs/common/ntfsv/ntfsv_mem.c > +++ b/osaf/libs/common/ntfsv/ntfsv_mem.c > @@ -18,6 +18,8 @@ > #include <saNtf.h> > #include <stdlib.h> > #include "ntfsv_mem.h" > +#include "osaf_extended_name.h" > +#include "saAis.h" > #include <logtrace.h> > > void ntfsv_free_header(const SaNtfNotificationHeaderT *notificationHeader) > @@ -27,10 +29,14 @@ void ntfsv_free_header(const SaNtfNotifi > free(notificationHeader->eventType); > if (notificationHeader->eventTime != NULL) > free(notificationHeader->eventTime); > - if (notificationHeader->notificationObject != NULL) > + if (notificationHeader->notificationObject != NULL) { > + osaf_extended_name_free(notificationHeader->notificationObject); > free(notificationHeader->notificationObject); > - if (notificationHeader->notifyingObject != NULL) > + } > + if (notificationHeader->notifyingObject != NULL) { > + osaf_extended_name_free(notificationHeader->notifyingObject); > free(notificationHeader->notifyingObject); > + } > if (notificationHeader->notificationClassId != NULL) > free(notificationHeader->notificationClassId); > if (notificationHeader->notificationId != NULL) > @@ -486,10 +492,10 @@ void ntfsv_copy_ntf_header(SaNtfNotifica > *(dest->eventTime) = *(src->eventTime); > > /* Set Notification Object */ > - *(dest->notificationObject) = *(src->notificationObject); > + ntfsv_sanamet_copy(dest->notificationObject, src->notificationObject); > > /* Set Notifying Object */ > - *(dest->notifyingObject) = *(src->notifyingObject); > + ntfsv_sanamet_copy(dest->notifyingObject, src->notifyingObject); > > /* set Notification Class Identifier */ > dest->notificationClassId->vendorId = > src->notificationClassId->vendorId; > @@ -954,7 +960,7 @@ SaAisErrorT ntfsv_filter_header_alloc(Sa > /* Notification objects */ > if (numNotificationObjects != 0) { > > - header->notificationObjects = (SaNameT > *)malloc(numNotificationObjects * sizeof(SaNameT)); > + header->notificationObjects = (SaNameT > *)calloc(numNotificationObjects, sizeof(SaNameT)); > if (header->notificationObjects == NULL) { > TRACE_1("Out of memory notificationObjects"); > rc = SA_AIS_ERR_NO_MEMORY; > @@ -964,7 +970,7 @@ SaAisErrorT ntfsv_filter_header_alloc(Sa > > /* Notifying objects */ > if (numNotifyingObjects != 0) { > - header->notifyingObjects = (SaNameT > *)malloc(numNotifyingObjects * sizeof(SaNameT)); > + header->notifyingObjects = (SaNameT > *)calloc(numNotifyingObjects, sizeof(SaNameT)); > if (header->notifyingObjects == NULL) { > TRACE_1("Out of memory notifyingObjects"); > rc = SA_AIS_ERR_NO_MEMORY; > @@ -1174,10 +1180,18 @@ error_free: > > void ntfsv_filter_header_free(SaNtfNotificationFilterHeaderT *header) > { > + size_t i; > + > + for (i = 0; i < header->numNotificationObjects; ++i) > + osaf_extended_name_free(header->notificationObjects + i); > + > + for (i = 0; i < header->numNotifyingObjects; ++i) > + osaf_extended_name_free(header->notifyingObjects + i); > + > free(header->eventTypes); > - free(header->notificationObjects); > - free(header->notifyingObjects); > - free(header->notificationClassIds); > + free(header->notificationObjects); > + free(header->notifyingObjects); > + free(header->notificationClassIds); > } > > void ntfsv_filter_sec_alarm_free(SaNtfSecurityAlarmNotificationFilterT > *s_filter) > @@ -1226,3 +1240,138 @@ void ntfsv_filter_attr_ch_free(SaNtfAttr > } > } > > +/** > + * @Brief: Copy SaNameT pointed by pSrc to pDes > + * > + */ > +SaAisErrorT ntfsv_sanamet_copy(SaNameT* pDes, SaNameT* pSrc) > +{ > + SaAisErrorT rc = SA_AIS_OK; > + if (osaf_is_an_extended_name(pDes)) { > + TRACE("Can not modify the existing longDn object"); > + return SA_AIS_ERR_EXIST; > + } > + if (osaf_is_an_extended_name(pSrc)) { > + osaf_extended_name_alloc(osaf_extended_name_borrow(pSrc), pDes); > + } else { > + /* pSrc is old SaNameT format, @.length could be counted with or > + * without null-termination, we need to preserve the @.length, > + * just a memcpy here > + */ > + memcpy(pDes, pSrc, sizeof(SaNameT)); > + } > + return rc; > > [Zoran] Minor comment. "rc" is not necessary in the function. > "return rc;" can be replaced with "return SA_AIS_OK;", and skip using "rc" [Minh]: Will remove rc > +} > + > +/** > + * @Brief: Check SaNameT is a valid formation > + * > + */ > +bool ntfsv_sanamet_is_valid(const SaNameT* pName) > +{ > + if (!osaf_is_extended_name_valid(pName)) { > + LOG_ER("Environment variable SA_ENABLE_EXTENDED_NAMES " > + "is not set, or not using extended name api"); > + return false; > + } > + if (osaf_extended_name_length(pName) > kMaxDnLength) { > + LOG_ER("Exceeding maximum of extended name length(%u)" > + ,kMaxDnLength); > + return false; > + } > + return true; > +} > + > +/** > + * @Brief: Clone the string pointer in extended name > + * > + */ > +void ntfsv_sanamet_clone_strptr(SaNameT* pName) > +{ > + if (osaf_is_an_extended_name(pName)) > + osaf_extended_name_alloc(osaf_extended_name_borrow(pName), > pName); > > [Zoran] Please check how this function is used. Seems like that this code > leads to a memory leak. Pointer in the extended name is overwritten. > I'll give you an example of using this function in another email. [Minh]: I will explain in another email. > +} > + > +/** > + * @Brief: Return the length of string specified in SaNameT type. > + * > + */ > +size_t ntfs_sanamet_length(const SaNameT* pName) > +{ > + if (osaf_is_an_extended_name(pName)) > + return osaf_extended_name_length(pName); > + > + /* In case of unextended name, the length returned by > + * osaf_extended_name_length sometimes is greater than @.length since if > + * @.value is not a null-terminated string, and it reads over the > + * @.length character(s). Here we need the length specified in @.length > + */ > > [Zoran] This text is not true. osaf_extended_name_length() takes care of > non-null-terminated string. > If it's not extended name, then osaf_extended_name_length() will return max > @.length. > Taken from osaf_extended_name_length the code: > length = strnlen((const char*) (name->_opaque + 1), length); [Minh]: The above text is wrong I agree, and I have sent another email to correct it. I should be: /* In case of unextended name, sometimes @.length includes the count on * null-termination, and osaf_extended_name_length returns the length * excluding the null-termination which less 1 than the @.length. Here * we need the length specified in @.length. This case mainly applies * in encodeSaNameT/decodeSaNameT in order to preserve the original * @.length value */ And that's the reason I need to retrieve 2 first bytes of SaNameT rather than returned by osaf_extended_name_length Otherwise the ntftest fails because the notificationObject/notifyingObject returned in callback having @.length less 1 than the expectation. And I don't think that's the issue in ntftest, since the length in old SaNameT is not strictly required with/or without a null-termination. > + size_t length = 0; > + memcpy(&length, (char*)pName, 2); > > [Zoran] This code will not work correct on big endian system. The size of > size_t is greater than 16 bits. > This is a minor comment, since ntfs_sanamet_length can be replaced > osaf_extended_name_length. [Minh]: will fix > + osafassert(length < SA_MAX_UNEXTENDED_NAME_LENGTH); > + return length; > +} > + > +/** > + * @Brief: Allocate the new null-terminated string in SaNameT type, and > return > + * the pointer of this new string. > + * > + */ > +SaStringT ntfs_sanamet_strdup(SaNameT* pName) > +{ > + SaStringT pRtr; > + SaConstStringT pStr; > + > + size_t length = 0; > + if (osaf_is_an_extended_name(pName)) { > + pStr = osaf_extended_name_borrow(pName); > + length = strlen(pStr) + 1; > + } else { > + length = ntfs_sanamet_length(pName); > + osafassert(length < SA_MAX_UNEXTENDED_NAME_LENGTH); > + pStr = (char*)(pName) + 2; > + /* for the case of non null-terminated in @.value */ > + if (pStr[length - 1] != '\0') > + length++; > + > + } > > [Zoran] As I mentioned in the comment before, osaf_extended_name_length() > works fine with both extended and non-extended names. > There is no need for making double work and making distinguish between > extended and non-extended name. osaf_extended_name_* functions should take > care of that. > The code above can be safely replaced with: > length = osaf_extended_name_length(pName) + 1; > pStr = osaf_extended_name_borrow(pName); > > + pRtr = (char*) malloc(length); > + if (pRtr == NULL) { > + LOG_ER("out of memory"); > + return NULL; > + } > + memcpy(pRtr, pStr, length); > > [Zoran] memcpy(pRtr, pStr, length - 1); [Minh]: The ntfs_sanamet_strdup above is not neat since I use my own ntfs_sanamet_length, and It can be simply replaced by your code, but I hope it would be made as osaf_extended_name_strdup, because osaf_extended_name_borrow does not guarantee a null-terminated string. Everytime we want to compare 2 SaNameT, we have to get length and put a NULL at the correct position in borrowed string. > Best regards, > Zoran > > + pRtr[length - 1] = '\0'; > + return pRtr; > +} > + > +/** > + * @Brief: Create SaNameT input by string @value and @length, > + * @value is not freed then > + */ > +void ntfs_sanamet_alloc(SaConstStringT value, size_t length, SaNameT* pName) > +{ > + osaf_extended_name_alloc(value, pName); > + /* Accept the old SaNameT which's @.length counting the null > termination */ > + if (!osaf_is_an_extended_name(pName) > + && ((ntfs_sanamet_length(pName)+1) == length)) { > + memcpy(pName, &length, 2); > + } > +} > + > +/** > + * @Brief: Create SaNameT input by string @value and @length, > + * @value is freed then > + */ > +void ntfs_sanamet_steal(SaStringT value, size_t length, SaNameT* pName) > +{ > + /* create pName by string @value */ > + osaf_extended_name_free(pName); > + osaf_extended_name_steal(value, pName); > + > + /* Accept the old SaNameT which's @.length counting the null > termination */ > + if (!osaf_is_an_extended_name(pName) > + && ((ntfs_sanamet_length(pName)+1) == length)) { > + memcpy(pName, &length, 2); > + } > +} > > ------------------------------------------------------------------------------ > Open source business process management suite built on Java and Eclipse > Turn processes into business applications with Bonita BPM Community Edition > Quickly connect people, data, and systems into organized workflows > Winner of BOSSIE, CODIE, OW2 and Gartner awards > http://p.sf.net/sfu/Bonitasoft > _______________________________________________ > Opensaf-devel mailing list > Opensaf-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/opensaf-devel >
------------------------------------------------------------------------------ Open source business process management suite built on Java and Eclipse Turn processes into business applications with Bonita BPM Community Edition Quickly connect people, data, and systems into organized workflows Winner of BOSSIE, CODIE, OW2 and Gartner awards http://p.sf.net/sfu/Bonitasoft _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
