Hi Vu,
I have not reviewed the code but have looked at the ticket and it seems to be
invalid. To me it seems as if the use case described in the ticket is not
really valid even if the IMM behavior is a bit inconsistent (value=<empty> is
replaced by default at restart but other values are not). If I have understood
it correctly you say that an attribute where an empty value has a significant
meaning (from ticket: MaxAge=<empty> means that a password never expires). This
is in itself not an invalid use case but there is already a way to avoid this
behavior and that is to not have a default value in the class specification. If
all values including <empty> is a valid value then setting a default value to
be handled by IMM is incorrect. Instead, the software that "owns/is dependent
on" this attribute should check if the value is valid (if for example the
attribute has a max value so values larger than that max are invalid) and if
the value is invalid it could be replaced by a default value.
Thanks
Lennart
-----Original Message-----
From: Vu Minh Nguyen <vu.m.ngu...@dektech.com.au>
Sent: den 10 januari 2019 03:56
To: Hans Nordebäck <hans.nordeb...@ericsson.com>; Lennart Lund
<lennart.l...@ericsson.com>; Gary Lee <gary....@dektech.com.au>
Cc: opensaf-devel@lists.sourceforge.net
Subject: RE: [PATCH 1/1] imm: allow empty-value attribute with default-tag
persisted [#2985]
Hi,
Any comment from you? I will push this ticket B/next week if no comment.
Regards, Vu
> -----Original Message-----
> From: Vu Minh Nguyen <vu.m.ngu...@dektech.com.au>
> Sent: Wednesday, December 19, 2018 6:11 PM
> To: hans.nordeb...@ericsson.com; lennart.l...@ericsson.com;
> gary....@dektech.com.au
> Cc: opensaf-devel@lists.sourceforge.net; Vu Minh Nguyen
> <vu.m.ngu...@dektech.com.au>
> Subject: [PATCH 1/1] imm: allow empty-value attribute with default-tag
> persisted [#2985]
>
> During runtime, when replacing value of an attribute which has
default-value
> tag with NULL, this NULL value is not persistent after cluster is
> rebooted
-
> NULL value will be automatically replaced with its default value by IMM.
>
> This behavior causes several unexpected results. Below is a use case.
>
> User defines an attribute with name `maxAge`. It is used to shows how
> many days user passwords will be expired, default value is 30 days; if
> replacing
with
> a NULL/empty, it means the passwords will never get expired.
>
> During runtime, user changes the existing value with NULL - expect the
> passwords never get expired. Later on, then cluster is rebooted, that
> value is
silently
> replaced with the default without notice of user.
>
> This patch makes some changes in immdump/immloader/imm om
> library/immnd to make NULL value in such case persisted even after
> cluster is rebooted.
> ---
> src/imm/README | 10 +---
> src/imm/agent/imma_om_api.cc | 7 ++-
> src/imm/immloadd/imm_loader.cc | 23 +++++---
> src/imm/immloadd/imm_pbe_load.cc | 16 ++++--
> src/imm/immnd/ImmModel.cc | 9 ++-
> src/imm/tools/imm_xmlw_dump.cc | 96 ++++++++++++++++++++------------
> 6 files changed, 100 insertions(+), 61 deletions(-)
>
> diff --git a/src/imm/README b/src/imm/README index
> 7ace34741..132ee0ac0 100644
> --- a/src/imm/README
> +++ b/src/imm/README
> @@ -351,14 +351,8 @@ as part of an attribute definition.
> (i) A default declaration is only allowed for single valued
> attributes
(no
> concept of a multivalued default exists).
>
> -(ii) Default values are assigned at object creation. Default values
> are
NOT
> -assigned if an attribute is set to the empty/null value by a
modification.
> -
> -(iii) Default values are assigned at cluster restart for any
> attributes
that
> -are null/empty and that have a default. This is a special case of (i)
because
> -imm loading actually uses the regular imm API to recreate the imm
contents.
> -In particular, saImmOmCcbObjectCreate is used to recreate all objects
from
> -the file-system image.
> +(ii) Default values are assigned at object creation, except the
> +creation
> comes
> +from IMM loader.
>
> Common missunderstandings about "system attributes" of an imm object.
> ---------------------------------------------------------------------
> diff --git a/src/imm/agent/imma_om_api.cc
> b/src/imm/agent/imma_om_api.cc index 7155799d9..0d24b2335 100644
> --- a/src/imm/agent/imma_om_api.cc
> +++ b/src/imm/agent/imma_om_api.cc
> @@ -2037,7 +2037,7 @@ static SaAisErrorT ccb_object_create_common(
> TRACE_2("ERR_INVALID_PARAM: Not allowed to set attribute %s",
> sysaImplName);
> goto mds_send_fail;
> - } else if (attr->attrValuesNumber == 0) {
> + } else if (attr->attrValuesNumber == 0 && !immOmIsLoader) {
> TRACE("CcbObjectCreate ignoring attribute %s with no values",
> attr->attrName);
> continue;
> @@ -2065,7 +2065,9 @@ static SaAisErrorT ccb_object_create_common(
>
> const SaImmAttrValueT *avarr = attr->attrValues;
> /*alloc-5 */
> - imma_copyAttrValue(&(p->n.attrValue), attr->attrValueType,
avarr[0]);
> + if (attr->attrValuesNumber > 0) {
> + imma_copyAttrValue(&(p->n.attrValue), attr->attrValueType,
avarr[0]);
> + }
>
> if (attr->attrValuesNumber > 1) {
> unsigned int numAdded = attr->attrValuesNumber - 1; @@
> -2087,6 +2089,7 @@ static SaAisErrorT ccb_object_create_common(
> }
> }
>
> +
> rc = imma_evt_fake_evs(cb, &evt, &out_evt, cl_node->syncr_timeout,
> cl_node->handle, &locked, false);
> cl_node = NULL;
> diff --git a/src/imm/immloadd/imm_loader.cc
> b/src/imm/immloadd/imm_loader.cc index e9a985c22..3bd3e2b2e 100644
> --- a/src/imm/immloadd/imm_loader.cc
> +++ b/src/imm/immloadd/imm_loader.cc
> @@ -117,6 +117,7 @@ typedef struct ParserStateStruct {
> SaImmAttrFlagsT attrFlags;
> SaUint32T attrNtfId;
> char *attrDefaultValueBuffer;
> + bool is_null_value;
>
> int attrValueTypeSet;
> int attrNtfIdSet;
> @@ -925,6 +926,13 @@ static void startElementHandler(void *userData,
> const xmlChar *name,
> state->state[state->depth] = VALUE;
> state->valueContinue = 0;
> state->isBase64Encoded = isBase64Encoded(attrs);
> + state->is_null_value = false;
> + if (attrs) {
> + char* null_value = getAttributeValue("xsi:nil", attrs);
> + if (null_value && std::string{null_value} == "true") {
> + state->is_null_value = true;
> + }
> + }
> /* <category> */
> } else if (strcmp((const char *)name, "category") == 0) {
> state->state[state->depth] = CATEGORY; @@ -982,7 +990,7 @@ static
> void endElementHandler(void *userData, const xmlChar *name) {
>
> /* </value> */
> if (strcmp((const char *)name, "value") == 0) {
> - if (state->attrValueBuffers.empty()) {
> + if (state->attrValueBuffers.empty() && !state->is_null_value) {
> char *str = (char *)malloc(1);
>
> str[0] = '\0';
> @@ -1759,14 +1767,13 @@ void addObjectAttributeDefinition(
> SaImmAttrValuesT_2 attrValues;
> int i;
> size_t len;
> + bool null_value = attrValueBuffers->empty();
> +
> TRACE_ENTER2("attrValueBuffers size:%u",
> (unsigned int)attrValueBuffers->size());
> /* The attrName must be set */
> assert(attrName);
>
> - /* The value array can not be empty */
> - assert(!attrValueBuffers->empty());
> -
> /* The object class must be set */
> assert(objectClass);
>
> @@ -1778,15 +1785,15 @@ void addObjectAttributeDefinition(
>
> /* For each value, convert from char* to SaImmAttrValuesT_2 and
> store an array pointing to all in attrValues */
> - attrValues.attrValuesNumber = attrValueBuffers->size();
> - attrValues.attrValues = (SaImmAttrValueT *)malloc(
> + attrValues.attrValuesNumber = null_value ? 0 :
attrValueBuffers->size();
> + attrValues.attrValues = null_value ? nullptr : (SaImmAttrValueT
*)malloc(
> sizeof(SaImmAttrValuesT_2) * attrValues.attrValuesNumber + 1);
>
> - attrValues.attrValues[attrValues.attrValuesNumber] = NULL;
> + if (!null_value) attrValues.attrValues[attrValues.attrValuesNumber]
> + =
> NULL;
>
> it = attrValueBuffers->begin();
> i = 0;
> - while (it != attrValueBuffers->end()) {
> + while (!null_value && it != attrValueBuffers->end()) {
> charsToValueHelper(&attrValues.attrValues[i],
attrValues.attrValueType,
> *it);
> i++;
> diff --git a/src/imm/immloadd/imm_pbe_load.cc
> b/src/imm/immloadd/imm_pbe_load.cc
> index d9e1c4d83..72b926383 100644
> --- a/src/imm/immloadd/imm_pbe_load.cc
> +++ b/src/imm/immloadd/imm_pbe_load.cc
> @@ -524,7 +524,8 @@ bool loadObjectFromPbe(void *pbeHandle,
> SaImmHandleT immHandle,
> const unsigned char *res;
> for (c = 0; c < ncols; ++c) {
> res = sqlite3_column_text(stmt, c);
> - if (res) {
> + bool is_null = false;
> + if (true) {
> SaImmValueTypeT attrType = (SaImmValueTypeT)0;
> const char *colname = sqlite3_column_name(stmt, c);
> assert(colname != NULL);
> @@ -544,7 +545,7 @@ bool loadObjectFromPbe(void *pbeHandle,
> SaImmHandleT immHandle,
> assert(it != class_info->attrInfoVector.end());
>
> char *val;
> - if (attrType == SA_IMM_ATTR_SADOUBLET) {
> + if (res && attrType == SA_IMM_ATTR_SADOUBLET) {
> double dbl = sqlite3_column_double(stmt, c);
>
> val = (char *)malloc(30);
> @@ -555,12 +556,16 @@ bool loadObjectFromPbe(void *pbeHandle,
> SaImmHandleT immHandle,
> snprintf(val, size, "%.17g", dbl);
> }
> } else {
> - val = strdup((const char *)res);
> + if (res) {
> + val = strdup((const char *)res);
> + }
> }
>
> + is_null = (res == nullptr);
> std::list<char *> attrValueBuffers;
> - attrValueBuffers.push_front(val);
> -
> + if (!is_null) {
> + attrValueBuffers.push_front(val);
> + }
> addObjectAttributeDefinition((char *)class_info->className.c_str(),
> (SaImmAttrNameT)colname,
&attrValueBuffers,
> attrType, &attrValuesList); @@
> -669,7 +674,6 @@ bool loadObjectFromPbe(void *pbeHandle, SaImmHandleT
> immHandle,
> (char *)(*it)->attrName.c_str(),
> &attrValueBuffers,
(*it)->attrValueType,
> &attrValuesList);
> -
> sqlite3_reset(stmt);
> } /*if(attr_is_multi && !attr_is_pure_rt)*/
> ++it;
> diff --git a/src/imm/immnd/ImmModel.cc b/src/imm/immnd/ImmModel.cc
> index 8e3f338dc..747b406c6 100644
> --- a/src/imm/immnd/ImmModel.cc
> +++ b/src/imm/immnd/ImmModel.cc
> @@ -8393,8 +8393,13 @@ SaAisErrorT ImmModel::ccbObjectCreate(
>
> ImmAttrValue* attrValue = i6->second;
> IMMSV_OCTET_STRING tmpos; // temporary octet string
> - eduAtValToOs(&tmpos, &(p->n.attrValue),
> - (SaImmValueTypeT)p->n.attrValueType);
> + memset(&tmpos, 0, sizeof(tmpos));
> + // Attribute value will be assigned null value or <empty>
> + // if having `attrValuesNumber=0` goes with it.
> + if (p->n.attrValuesNumber) {
> + eduAtValToOs(&tmpos, &(p->n.attrValue),
> + (SaImmValueTypeT)p->n.attrValueType);
> + }
> attrValue->setValue(tmpos);
> if (p->n.attrValuesNumber > 1) {
> /*
> diff --git a/src/imm/tools/imm_xmlw_dump.cc
> b/src/imm/tools/imm_xmlw_dump.cc index 2e7fcda86..e6fdd279f 100644
> --- a/src/imm/tools/imm_xmlw_dump.cc
> +++ b/src/imm/tools/imm_xmlw_dump.cc
> @@ -419,11 +419,6 @@ void objectToXMLw(std::string objectNameString,
> SaImmAttrValuesT_2** attrs,
> exit(1);
> }
> for (SaImmAttrValuesT_2** p = attrs; *p != NULL; p++) {
> - /* Skip attributes with attrValues = NULL */
> - if ((*p)->attrValues == NULL) {
> - continue;
> - }
> -
> if (classRDNMap.find(classNameString) != classRDNMap.end() &&
> classRDNMap[classNameString] == std::string((*p)->attrName)) {
> continue;
> @@ -469,10 +464,6 @@ static void StoreObject(const std::string&
> objectName,
>
> /* Add attributes to list */
> for (SaImmAttrValuesT_2** p = attrs; *p != NULL; p++) {
> - /* Skip attributes with attrValues = NULL */
> - if ((*p)->attrValues == NULL) {
> - continue;
> - }
> /* Skip RDN */
> if (classRDNMap.find(obj.className) != classRDNMap.end() &&
> classRDNMap[obj.className] == std::string((*p)->attrName)) {
> @@ -544,42 +535,62 @@ static void ObjectSetToXMLw(std::set<Object,
> ObjectComp>& objectSet,
> }
>
> /* Write attribute values */
> - for (std::list<std::string>::const_iterator value_it =
> - attr_it->second.begin();
> - value_it != attr_it->second.end(); ++value_it) {
> + std::list<std::string> values = attr_it->second;
> + if (values.empty()) {
> if (xmlTextWriterStartElement(writer, (xmlChar*)"value") < 0) {
> std::cout << "Error at xmlTextWriterStartElement (value)"
> << std::endl;
> exit(1);
> }
> - if (osaf_is_valid_xml_utf8((*value_it).c_str())) {
> - if (xmlTextWriterWriteString(writer,
(xmlChar*)(*value_it).c_str()) <
> - 0) {
> - std::cout << "Error at xmlTextWriterWriteString (value)"
> - << std::endl;
> - exit(1);
> - }
> - } else {
> - if (xmlTextWriterWriteAttribute(writer, (xmlChar*)"xsi:type",
> - (xmlChar*)"xs:base64Binary") <
0) {
> - std::cout << "Error at xmlTextWriterWriteAttribute (value)"
> - << std::endl;
> - exit(1);
> - }
> - if (xmlTextWriterWriteBase64(writer, (*value_it).c_str(), 0,
> - (*value_it).size()) < 0) {
> - std::cout << "Error at xmlTextWriterWriteBase64 (value)"
> - << std::endl;
> - exit(1);
> - }
> + if (xmlTextWriterWriteAttribute(writer, (xmlChar*)"xsi:nil",
> + (xmlChar*)"true") < 0) {
> + std::cout << "Error at xmlTextWriterWriteAttribute (value)"
> + << std::endl;
> + exit(1);
> }
> if (xmlTextWriterEndElement(writer) < 0) {
> std::cout << "Error at xmlTextWriterWriteEndElement (value)"
> << std::endl;
> exit(1);
> }
> - } /* Attribute values loop */
>
> + } else {
> + for (std::list<std::string>::const_iterator value_it =
> + attr_it->second.begin();
> + value_it != attr_it->second.end(); ++value_it) {
> + if (xmlTextWriterStartElement(writer, (xmlChar*)"value") < 0) {
> + std::cout << "Error at xmlTextWriterStartElement (value)"
> + << std::endl;
> + exit(1);
> + }
> + if (osaf_is_valid_xml_utf8((*value_it).c_str())) {
> + if (xmlTextWriterWriteString(writer,
(xmlChar*)(*value_it).c_str()) <
> + 0) {
> + std::cout << "Error at xmlTextWriterWriteString (value)"
> + << std::endl;
> + exit(1);
> + }
> + } else {
> + if (xmlTextWriterWriteAttribute(writer, (xmlChar*)"xsi:type",
> +
> + (xmlChar*)"xs:base64Binary")
< 0) {
> + std::cout << "Error at xmlTextWriterWriteAttribute (value)"
> + << std::endl;
> + exit(1);
> + }
> + if (xmlTextWriterWriteBase64(writer, (*value_it).c_str(), 0,
> + (*value_it).size()) < 0) {
> + std::cout << "Error at xmlTextWriterWriteBase64 (value)"
> + << std::endl;
> + exit(1);
> + }
> + }
> + if (xmlTextWriterEndElement(writer) < 0) {
> + std::cout << "Error at xmlTextWriterWriteEndElement (value)"
> + << std::endl;
> + exit(1);
> + }
> + } /* Attribute values loop */
> + }
> if (xmlTextWriterEndElement(writer) < 0) {
> std::cout << "Error at xmlTextWriterEndElement (attr-object)"
> << std::endl;
> @@ -597,8 +608,23 @@ static void ObjectSetToXMLw(std::set<Object,
> ObjectComp>& objectSet,
> }
>
> void valuesToXMLw(SaImmAttrValuesT_2* p, xmlTextWriterPtr writer) {
> - if (!p->attrValues) {
> - // std::cout << "No values!" << std::endl;
> + if (p->attrValuesNumber == 0) {
> + if (xmlTextWriterStartElement(writer, (xmlChar*)"value") < 0) {
> + std::cout << "Error at xmlTextWriterStartElement (value)"
> + << std::endl;
> + exit(1);
> + }
> + if (xmlTextWriterWriteAttribute(writer, (xmlChar*)"xsi:nil",
> + (xmlChar*)"true") < 0) {
> + std::cout << "Error at xmlTextWriterWriteAttribute (value)"
> + << std::endl;
> + exit(1);
> + }
> + if (xmlTextWriterEndElement(writer) < 0) {
> + std::cout << "Error at xmlTextWriterWriteEndElement (value)"
> + << std::endl;
> + exit(1);
> + }
> return;
> }
>
> --
> 2.19.2
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
