Hi Nguyen The main issue that I saw is that you have added some checking of <_empy_> tag in the ccb handler (in imm_modify_config). This is not allowed. The ccb handler is supposed to be generic and completely independent of SMF. For now it is housed within the SMF src directories but it could be made a library and be used by other services. Let's not destroy that possibility. The ccb handler is also already used in other places that are not related to smfImmOperation (exec control object handling and node groups for lock operations).
Instead add a check <_empty_> before adding a value to an attribute descriptor.
If the value is <_empty_>, don't add it. The legacy attribute handling classes
still have the original attribute vectors and there it is ok to add <_empty_>
as before. I have written some comments in the code as well to clarify.
See attached patch (can be applied on your review branch)
Thanks
Lennart
> -----Original Message-----
> From: Vu Minh Nguyen <vu.m.ngu...@dektech.com.au>
> Sent: den 19 juni 2018 10:50
> To: Lennart Lund <lennart.l...@ericsson.com>; Hans Nordebäck
> <hans.nordeb...@ericsson.com>; Gary Lee <gary....@dektech.com.au>
> Cc: opensaf-devel@lists.sourceforge.net; Vu Minh Nguyen
> <vu.m.ngu...@dektech.com.au>
> Subject: [PATCH 1/1] imm: fix failure to import file containing existing long
> dn
> object [#2874]
>
> The original object name `state->objectName` was copied using the function
> osaf_extended_name_lend(state->objectName, &objectName).
>
> In case of long dn object, the `objectName` contained a reference to the
> string
> which was stored in `state->objectName`, any change to `state-
> >objectName`,
> in this case it was the code line `state->objectName[DNlen] = '\0',
> would impact the backup `objectName`.
>
> As the `objectName` had been changed unintentionally to non-existing
> object dn,
> `saImmOmAccessorGet_2` returned `SA_AIS_ERR_NOT_EXIST`
> unexpectedly.
>
> With this fix, using C++ string to copy the `state->objectName` value.
> Extracting parent DN, and object RDN are operating on C++ string, don't
> change the original value.
> ---
> src/imm/tools/imm_import.cc | 39 +++++++++++++++++++--------------------
> 1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/src/imm/tools/imm_import.cc b/src/imm/tools/imm_import.cc
> index 5397c67..e2bdcba 100644
> --- a/src/imm/tools/imm_import.cc
> +++ b/src/imm/tools/imm_import.cc
> @@ -182,6 +182,7 @@ static SaImmValueTypeT
> getClassAttrValueType(ParserState *, const char *,
> const char *);
> static void saveRDNAttribute(ParserState *parserState);
> static void getDNForClass(ParserState *, const SaImmClassNameT,
> + const std::string&,
> SaImmAttrValuesT_2 *);
> static int charsToValueHelper(SaImmAttrValueT *, SaImmValueTypeT, const
> char *,
> bool strictParse);
> @@ -633,7 +634,8 @@ static void createImmObject(ParserState *state) {
> SaAisErrorT errorCode = SA_AIS_OK;
> int i = 0;
> size_t DNlen;
> - SaNameT objectName;
> + const char *parent = nullptr;
> + std::string rdn;
> std::list<SaImmAttrValuesT_2>::iterator it;
>
> TRACE_8("CREATE IMM OBJECT %s, %s", state->objectClass, state-
> >objectName);
> @@ -645,23 +647,21 @@ static void createImmObject(ParserState *state) {
>
> /* Set the parent name */
> osaf_extended_name_clear(&parentName);
> - if (state->objectName != NULL) {
> - char *parent;
> - osaf_extended_name_lend(state->objectName, &objectName);
> -
> + if (state->objectName != nullptr) {
> + rdn = std::string{state->objectName};
> /* ',' is the delimeter */
> /* but '\' is the escape character, used for association objects */
> - parent = state->objectName;
> + parent = rdn.c_str();
> do {
> parent = strchr(parent, ',');
> TRACE_8("PARENT: %s", parent);
> } while (parent && (*((++parent) - 2)) == '\\');
>
> if (parent && strlen(parent) <= 1) {
> - parent = NULL;
> + parent = nullptr;
> }
>
> - if (parent != NULL) osaf_extended_name_lend(parent, &parentName);
> + if (parent != nullptr) osaf_extended_name_lend(parent, &parentName);
> } else {
> LOG_ER("Empty DN for object");
> stopParser(state);
> @@ -675,19 +675,13 @@ static void createImmObject(ParserState *state) {
>
> if (state->ctxt->instate == XML_PARSER_EOF) return;
>
> -#ifdef TRACE_8
> /* Get the length of the DN and truncate state->objectName */
> if (!osaf_is_extended_name_empty(&parentName)) {
> DNlen = strlen(state->objectName) -
> (strlen(osaf_extended_name_borrow(&parentName)) + 1);
> - } else {
> - DNlen = strlen(state->objectName);
> + rdn[DNlen] = '\0';
> }
>
> - state->objectName[DNlen] = '\0';
> - TRACE_8("OBJECT NAME: %s", state->objectName);
> -#endif
> -
> if (!state->validation) {
> /* Set the attribute values array, add space for the rdn attribute
> * and a NULL terminator */
> @@ -717,17 +711,19 @@ static void createImmObject(ParserState *state) {
>
> /* Do the actual creation */
> attrValues[i] = (SaImmAttrValuesT_2 *)calloc(1,
> sizeof(SaImmAttrValuesT_2));
> - getDNForClass(state, className, attrValues[i]);
> + getDNForClass(state, className, rdn, attrValues[i]);
>
> if (state->ctxt->instate != XML_PARSER_EOF) {
> errorCode = immutil_saImmOmCcbObjectCreate_2(
> - state->ccbHandle, className, &parentName,
> + state->ccbHandle, className,
> + (parent == nullptr) ? nullptr : &parentName,
> (const SaImmAttrValuesT_2 **)attrValues);
> } else {
> goto done;
> }
> - } else
> + } else {
> attrValues = NULL;
> + }
>
> if (SA_AIS_OK != errorCode) {
> if ((errorCode == SA_AIS_ERR_NOT_EXIST) && imm_import_ccb_safe) {
> @@ -746,6 +742,7 @@ static void createImmObject(ParserState *state) {
> SaImmAttrValuesT_2 **existing_attributes;
> SaImmAttrValuesT_2 *attr;
> const char *existing_className;
> + SaNameT dn;
>
> LOG_IN("OBJECT %s already exist, verifying...", state->objectName);
>
> @@ -759,7 +756,8 @@ static void createImmObject(ParserState *state) {
> goto done;
> }
>
> - errorCode = immutil_saImmOmAccessorGet_2(accessorHandle,
> &objectName,
> + osaf_extended_name_lend(state->objectName, &dn);
> + errorCode = immutil_saImmOmAccessorGet_2(accessorHandle, &dn,
> NULL, // get all attributes
> &existing_attributes);
> if (SA_AIS_OK != errorCode) {
> @@ -1149,6 +1147,7 @@ static void createImmClass(ParserState *state) {
> * Returns an SaImmAttrValueT struct representing the DN for an object
> */
> static void getDNForClass(ParserState *state, const SaImmClassNameT
> className,
> + const std::string& rdn,
> SaImmAttrValuesT_2 *values) {
> std::string classNameString;
>
> @@ -1176,7 +1175,7 @@ static void getDNForClass(ParserState *state, const
> SaImmClassNameT className,
> values->attrValuesNumber = 1;
>
> if (charsToValueHelper(values->attrValues, values->attrValueType,
> - state->objectName, true)) {
> + rdn.c_str(), true)) {
> free(values->attrValues);
> values->attrValues = NULL;
>
> --
> 1.9.1
nguyen-fix-lennart-comments.diff
Description: nguyen-fix-lennart-comments.diff
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
