Hi Anders,
Currently in ntfimcnd, the SaNameT arrived from IMM has null-terminated
string, and not much code in ntfimcnd to deal with null or non-null
terminated string.
The problem is mostly between NTF and its other users, who send the
notification/notifying objects and expect exactly the same ones in the
callback, so as you suggested, it's dealt in ntf agent/library for now
Another issue that I have to *clone* the string pointer inside long dn
objects:
- As sending notification, app allocates notification, use saAisNameLend to
"package" the long dn object. At the end, app calls saNtfNotificationFree(),
as result the ntfsv_free_header() will free these long dn objects. At this
moment, ntfsv_free_header() does not have to worry the string pointer since
app will free it
- In case of callback, the ntf agent allocates a new notification and also
new long dn objects are allocated. At the end of receiving callback, the app
also calls saNtfNotificationFree() to free the received notification in
callback, ... ntfsv_free_header() now has to free the string pointer in
these long dn objects.
Both cases in ntfsv_free_header(), if I can distinguish notification
generated by app or ntf agent (in callback), I would not have to clone the
string pointer.
Thanks,
Minh
On 7/7/2014 6:22 PM, Anders Bjornerstedt wrote:
Anders Bjornerstedt wrote:
About null terminated or not strings in SaNameT.
The IMM handles this issue like this (not BEFORE the long DN support).
That should have been: (note BEFORE the long DN support).
/AndersBj
1) The actual max length of DNs supported by the IMM is 255 bytes, not
256.
This is to allow all SaNameT values to be null termianted which is a HUGE
simplification in
all internal code for trace/log etc.
2) An SaNameT that arrives over the IMMA API from a user can of course not
be assumed to be null terminated.
But all IMMA APIs that do receive an SaNameT from the user will check that
the length is at most 255 bytes and
then actually write a terminating null byte into the SaNameT struct.
3) Only the agent/library code should need to deal with the (quite frankly
stupid) SaNameT type.
Server code should not use SaNaameT.
So I really dont like to see the code in either library or server getting
ccomplicated just because
some SaNameT received from a user may not be null terminated. Plug that
problem at the source
close to the user API.
Given that this is done, you should be able to use the lend and borrow
functions and avoid any
unnecessary cloning/copying.
Ntf may have legacy code (messages etc) that use SaNameT in the server.
Even if you have that and thus use SaNameT more than is really needed, I
strongly suggest
you null terminate SaNameTs close to the source (the ntf agent/library
user), also for NTF.
/AndesBj
minhchau wrote:
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 [[1]mailto:[email protected]]
Sent: den 2 juli 2014 07:50
To: Anders Widell; [2][email protected]; Hans Feldt;
[3][email protected]
Cc: [4][email protected]
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);
}
voidntfsv_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
[5]http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Opensaf-devel mailing list
[6][email protected]
[7]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
[8]http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Opensaf-devel mailing list
[9][email protected]
[10]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
[11]http://p.sf.net/sfu/Bonitasoft
_______________________________________________
Opensaf-devel mailing list
[12][email protected]
[13]https://lists.sourceforge.net/lists/listinfo/opensaf-devel
References
1. mailto:[email protected]
2. mailto:[email protected]
3. mailto:[email protected]
4. mailto:[email protected]
5. http://p.sf.net/sfu/Bonitasoft
6. mailto:[email protected]
7. https://lists.sourceforge.net/lists/listinfo/opensaf-devel
8. http://p.sf.net/sfu/Bonitasoft
9. mailto:[email protected]
10. https://lists.sourceforge.net/lists/listinfo/opensaf-devel
11. http://p.sf.net/sfu/Bonitasoft
12. mailto:[email protected]
13. 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
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
