From: Anders B. <and...@er...> - 2014-07-09 09:48:21
|
Summary: imm: Provide validation for config changes on imm service objects [#951] Review request for Trac Ticket(s): 951 Peer Reviewer(s): Neel; Zoran Pull request to: Affected branch(es): 4.3; 4.4; default(4.5) Development branch: -------------------------------- Impacted area Impact y/n -------------------------------- Docs n Build system n RPM/packaging n Configuration files n Startup scripts n SAF services n OpenSAF services n Core libraries n Samples n Tests n Other n Comments (indicate scope for each "y" above): --------------------------------------------- changeset 015ee3f5d468ff8ec89667554aef729b34a76cc1 Author: Anders Bjornerstedt <and...@er...> Date: Wed, 09 Jul 2014 10:27:19 +0200 imm: Provide validation for config changes on imm service objects [#951] The six validation cases described in the ticket are implemented by this changeset: For the object 'opensafImm=opensafImm,safApp=safImmService': 1) 0PBE reject delete of the object. 2) 0PBE reject creates using class 'OpensafImm' (All modifications to current config attributes are allowed after #934) (For 1PBE and 2PBE, the validation for this object is handled by the PBE-OI). For the object 'safRdn=immManagement,safApp=safImmService': 3) Reject delete of the object. 4) Reject create using class 'SaImmMngt' 5) Validate modifications to attribute 'saImmRepositoryInit' 6) Reject use/modification of 'saImmOiTimeout'(not supported). The patch sent for review is for default(4.5) and does not apply cleanly on 4.3 or 4.4. I have patches for 4.3 and 4.4 that can be provided on request. When adding validations which are always a form of restriction, there is always the issue of backwards compatibility. The only possible issue that I could arise is if some application/script is currently updating the 'saImmOiTimeout' value. That is a futile operation that has no effect currently since the imm is not honoring that config attribute (see ticket #16 for details on why). I tink this is unlikely and the application that is doing such modifications is probably well served by adjusting to the reality of the attribute not working. In any case, a failure to update this attribute should not cause any major problem unless the application is designed to be extremely brittle/fragile. Complete diffstat: ------------------ osaf/services/saf/immsv/immnd/ImmModel.cc | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 files changed, 86 insertions(+), 8 deletions(-) Testing Commands: ----------------- The above 6 validation cases should work. Testing, Expected Results: -------------------------- An attempt to violate any of the 6 cases should result in that ccb-operation being rejected (BAD_OPERATION). Conditions of Submission: ------------------------- Ack from Neel Arch Built Started Linux distro ------------------------------------------- mips n n mips64 n n x86 n n x86_64 n n powerpc n n powerpc64 n n Reviewer Checklist: ------------------- [Submitters: make sure that your review doesn't trigger any checkmarks!] Your checkin has not passed review because (see checked entries): ___ Your RR template is generally incomplete; it has too many blank entries that need proper data filled in. ___ You have failed to nominate the proper persons for review and push. ___ Your patches do not have proper short+long header ___ You have grammar/spelling in your header that is unacceptable. ___ You have exceeded a sensible line length in your headers/comments/text. ___ You have failed to put in a proper Trac Ticket # into your commits. ___ You have incorrectly put/left internal data in your comments/files (i.e. internal bug tracking tool IDs, product names etc) ___ You have not given any evidence of testing beyond basic build tests. Demonstrate some level of runtime or other sanity testing. ___ You have ^M present in some of your files. These have to be removed. ___ You have needlessly changed whitespace or added whitespace crimes like trailing spaces, or spaces before tabs. ___ You have mixed real technical changes with whitespace and other cosmetic code cleanup changes. These have to be separate commits. ___ You need to refactor your submission into logical chunks; there is too much content into a single commit. ___ You have extraneous garbage in your review (merge commits etc) ___ You have giant attachments which should never have been sent; Instead you should place your content in a public tree to be pulled. ___ You have too many commits attached to an e-mail; resend as threaded commits, or place in a public tree for a pull. ___ You have resent this content multiple times without a clear indication of what has changed between each re-send. ___ You have failed to adequately and individually address all of the comments and change requests that were proposed in the initial review. ___ You have a misconfigured ~/.hgrc file (i.e. username, email etc) ___ Your computer have a badly configured date and time; confusing the the threaded patch review. ___ Your changes affect IPC mechanism, and you don't present any results for in-service upgradability test. ___ Your changes affect user manual and documentation, your patch series do not contain the patch that updates the Doxygen manual. |
From: Anders B. <and...@er...> - 2014-07-09 09:48:24
|
osaf/services/saf/immsv/immnd/ImmModel.cc | 94 ++++++++++++++++++++++++++++-- 1 files changed, 86 insertions(+), 8 deletions(-) The six validation cases described in the ticket are implemented by this changeset: For the object 'opensafImm=opensafImm,safApp=safImmService': 1) 0PBE reject delete of the object. 2) 0PBE reject creates using class 'OpensafImm' (All modifications to current config attributes are allowed after #934) (For 1PBE and 2PBE, the validation for this object is handled by the PBE-OI. For the object 'safRdn=immManagement,safApp=safImmService': 3) Reject delete of the object. 4) Reject create using class 'SaImmMngt' 5) Validate modifications to attribute 'saImmRepositoryInit' 6) Reject use/modification of 'saImmOiTimeout'(not supported). diff --git a/osaf/services/saf/immsv/immnd/ImmModel.cc b/osaf/services/saf/immsv/immnd/ImmModel.cc --- a/osaf/services/saf/immsv/immnd/ImmModel.cc +++ b/osaf/services/saf/immsv/immnd/ImmModel.cc @@ -441,8 +441,10 @@ static const std::string immSyncBatchSiz static const std::string immPbeBSlaveName(OPENSAF_IMM_2PBE_APPL_NAME); static const std::string immLongDnsAllowed(OPENSAF_IMM_LONG_DNS_ALLOWED); +static const std::string immMngtClass("SaImmMngt"); static const std::string immManagementDn("safRdn=immManagement,safApp=safImmService"); static const std::string saImmRepositoryInit("saImmRepositoryInit"); +static const std::string saImmOiTimeout("saImmOiTimeout"); static SaImmRepositoryInitModeT immInitMode = SA_IMM_INIT_FROM_FILE; @@ -7126,19 +7128,45 @@ SaAisErrorT ImmModel::ccbObjectCreate(Im object->mImplementer->mImplementerName.c_str()); ccb->mWaitStartTime = time(NULL); osafassert(ccb->mWaitStartTime > ((time_t) 0)); + } else if(className == immMngtClass) { + if(sImmNodeState == IMM_NODE_LOADING) { + if(objectName != immManagementDn) { + /* Backwards compatibility for loading. */ + LOG_WA("Imm loading encountered bogus object '%s' of class '%s'", + objectName.c_str(), immMngtClass.c_str()); + } + } else { + setCcbErrorString(ccb, + "ERR_BAD_OPERATION: Imm not allowing creates of instances of class '%s'", + immMngtClass.c_str()); + err = SA_AIS_ERR_BAD_OPERATION; + } + } else if(className == immClassName) { + if(sImmNodeState == IMM_NODE_LOADING) { + if(objectName != immObjectDn) { + /* Backwards compatibility for loading. */ + LOG_WA("Imm loading encountered bogus object '%s' of class '%s'", + objectName.c_str(), immClassName.c_str()); + } + } else { + setCcbErrorString(ccb, + "ERR_BAD_OPERATION: Imm not allowing creates of instances of class '%s'", + immClassName.c_str()); + err = SA_AIS_ERR_BAD_OPERATION; + } } else if(ccb->mCcbFlags & SA_IMM_CCB_REGISTERED_OI) { if((object->mImplementer == NULL) && (ccb->mCcbFlags & SA_IMM_CCB_ALLOW_NULL_OI)) { TRACE_7("Null implementer, SA_IMM_CCB_REGISTERED_OI set, " "but SA_IMM_CCB_ALLOW_NULL_OI set => safe relaxation"); } else { - TRACE_7("ERR_NOT_EXIST: object '%s' does not have an " + TRACE_7("ERR_NOT_EXIST: class '%s' does not have an " "implementer and flag SA_IMM_CCB_REGISTERED_OI is set", - objectName.c_str()); + className.c_str()); setCcbErrorString(ccb, - "ERR_NOT_EXIST: object '%s' exist but " - "no implementer (which is required)", - objectName.c_str()); + "ERR_NOT_EXIST: class '%s' does not have an " + "implementer and flag SA_IMM_CCB_REGISTERED_OI is set", + className.c_str()); err = SA_AIS_ERR_NOT_EXIST; } } else { /* SA_IMM_CCB_REGISTERED_OI NOT set */ @@ -7152,7 +7180,7 @@ SaAisErrorT ImmModel::ccbObjectCreate(Im TRACE_7("Object '%s' has NULL implementer, flag SA_IMM_CCB_REGISTERED_OI " "is NOT set - moderately safe.", objectName.c_str()); } - } + } } bypass_impl: @@ -7384,6 +7412,7 @@ ImmModel::ccbObjectModify(const ImmsvOmC ObjectNameSet afimPreOpNDRefs; // Set of NO_DANGLING references from after image before CCB operation bool hasNoDanglingRefs = false; + bool modifiedImmMngt = false; /* true => modification of the SAF immManagement object. */ if(! (nameCheck(objectName)||nameToInternal(objectName)) ) { LOG_NO("ERR_INVALID_PARAM: Not a proper object name"); @@ -7445,6 +7474,8 @@ ImmModel::ccbObjectModify(const ImmsvOmC } object = oi->second; + + modifiedImmMngt = (objectName == immManagementDn); object->getAdminOwnerName(&objAdminOwnerName); if(objAdminOwnerName != adminOwner->mAdminOwnerName) @@ -7564,6 +7595,19 @@ ImmModel::ccbObjectModify(const ImmsvOmC sz = strnlen((char *) p->attrValue.attrName.buf, (size_t) p->attrValue.attrName.size); std::string attrName((const char *) p->attrValue.attrName.buf, sz); + bool modifiedRim = modifiedImmMngt && (attrName == saImmRepositoryInit); + bool modifiedOiTimeout = modifiedImmMngt && (attrName == saImmOiTimeout); + + if(modifiedOiTimeout) { + /* Currently the IMM does not support this attribute. */ + TRACE_7("ERR_BAD_OPERATION: attr '%s' in IMM object %s is not supported", + attrName.c_str(), objectName.c_str()); + setCcbErrorString(ccb, + "ERR_BAD_OPERATION: attr '%s' in IMM object %s is not supported", + attrName.c_str(), objectName.c_str()); + err = SA_AIS_ERR_BAD_OPERATION; + break; //out of for-loop + } i4 = classInfo->mAttrMap.find(attrName); if(i4==classInfo->mAttrMap.end()) { @@ -7709,7 +7753,22 @@ ImmModel::ccbObjectModify(const ImmsvOmC multiattr->setExtraValue(tmpos); } - + + if (modifiedRim) { + SaImmRepositoryInitModeT newRim = (SaImmRepositoryInitModeT) attrValue->getValue_int(); + if((newRim != SA_IMM_INIT_FROM_FILE) && (newRim != SA_IMM_KEEP_REPOSITORY)) { + TRACE_7("ERR_INVALID_PARAM: attr '%s' in IMM object %s can not have value %u", + attrName.c_str(), objectName.c_str(), newRim); + setCcbErrorString(ccb, + "ERR_BAD_OPERATION: attr '%s' in IMM object %s can not have value %u", + attrName.c_str(), objectName.c_str(), newRim); + err = SA_AIS_ERR_BAD_OPERATION; + break; + } + } + + + if(p->attrValue.attrValuesNumber > 1) { if(!(attr->mFlags & SA_IMM_ATTR_MULTI_VALUE)) { LOG_NO("ERR_INVALID_PARAM: attr '%s' is not multivalued, yet " @@ -7760,6 +7819,17 @@ ImmModel::ccbObjectModify(const ImmsvOmC break; } + if (modifiedRim) { + TRACE_7("ERR_BAD_OPERATION: attr '%s' in IMM object %s must not be empty", + attrName.c_str(), objectName.c_str()); + setCcbErrorString(ccb, + "ERR_BAD_OPERATION: attr '%s' in IMM object %s must not be empty", + attrName.c_str(), objectName.c_str()); + err = SA_AIS_ERR_BAD_OPERATION; + break; + } + + if(!attrValue->empty()) { eduAtValToOs(&tmpos, &(p->attrValue.attrValue), (SaImmValueTypeT) p->attrValue.attrValueType); @@ -7804,7 +7874,7 @@ ImmModel::ccbObjectModify(const ImmsvOmC p->attrModType); break; } - + if(err != SA_AIS_OK) { break; //out of for-loop } @@ -8388,6 +8458,14 @@ ImmModel::deleteObject(ObjectMap::iterat if(!doIt && !(oi->second->mImplementer && oi->second->mImplementer->mNodeId) && configObj) { /* Implementer is not present. */ + /* Prevent delete of imm service objects, even when there is no OI for them */ + if(oi->first == immManagementDn || oi->first == immObjectDn) { + setCcbErrorString(ccb, + "ERR_BAD_OPERATION: Imm not allowing delete of object '%s'", + oi->first.c_str()); + return SA_AIS_ERR_BAD_OPERATION; + } + if(ccb->mCcbFlags & SA_IMM_CCB_REGISTERED_OI){ if((oi->second->mImplementer == NULL) && (ccb->mCcbFlags & SA_IMM_CCB_ALLOW_NULL_OI)) { |
From: Neelakanta R. <red...@or...> - 2014-07-16 11:16:02
|
HI AndersBj, Reviewed and tested the patch. Ack. /Neel. On Wednesday 09 July 2014 03:18 PM, Anders Bjornerstedt wrote: > Summary: imm: Provide validation for config changes on imm service objects [#951] > Review request for Trac Ticket(s): 951 > Peer Reviewer(s): Neel; Zoran > Pull request to: > Affected branch(es): 4.3; 4.4; default(4.5) > Development branch: > > -------------------------------- > Impacted area Impact y/n > -------------------------------- > Docs n > Build system n > RPM/packaging n > Configuration files n > Startup scripts n > SAF services n > OpenSAF services n > Core libraries n > Samples n > Tests n > Other n > > > Comments (indicate scope for each "y" above): > --------------------------------------------- > > changeset 015ee3f5d468ff8ec89667554aef729b34a76cc1 > Author: Anders Bjornerstedt <and...@er...> > Date: Wed, 09 Jul 2014 10:27:19 +0200 > > imm: Provide validation for config changes on imm service objects [#951] > > The six validation cases described in the ticket are implemented by this > changeset: > > For the object 'opensafImm=opensafImm,safApp=safImmService': > > 1) 0PBE reject delete of the object. > 2) 0PBE reject creates using class 'OpensafImm' > (All modifications to current config attributes are allowed after #934) > (For 1PBE and 2PBE, the validation for this object is handled by the PBE-OI). > > For the object 'safRdn=immManagement,safApp=safImmService': > > 3) Reject delete of the object. > 4) Reject create using class 'SaImmMngt' > 5) Validate modifications to attribute 'saImmRepositoryInit' > 6) Reject use/modification of 'saImmOiTimeout'(not supported). > > > The patch sent for review is for default(4.5) and does not apply > cleanly on 4.3 or 4.4. I have patches for 4.3 and 4.4 that can > be provided on request. > > When adding validations which are always a form of restriction, there is always > the issue of backwards compatibility. The only possible issue that I could arise > is if some application/script is currently updating the 'saImmOiTimeout' value. > That is a futile operation that has no effect currently since the imm is not > honoring that config attribute (see ticket #16 for details on why). > I tink this is unlikely and the application that is doing such modifications is > probably well served by adjusting to the reality of the attribute not working. > In any case, a failure to update this attribute should not cause any major problem > unless the application is designed to be extremely brittle/fragile. > > > Complete diffstat: > ------------------ > osaf/services/saf/immsv/immnd/ImmModel.cc | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------- > 1 files changed, 86 insertions(+), 8 deletions(-) > > > Testing Commands: > ----------------- > The above 6 validation cases should work. > > > Testing, Expected Results: > -------------------------- > An attempt to violate any of the 6 cases should result in that ccb-operation > being rejected (BAD_OPERATION). > > > Conditions of Submission: > ------------------------- > Ack from Neel > > > Arch Built Started Linux distro > ------------------------------------------- > mips n n > mips64 n n > x86 n n > x86_64 n n > powerpc n n > powerpc64 n n > > > Reviewer Checklist: > ------------------- > [Submitters: make sure that your review doesn't trigger any checkmarks!] > > > Your checkin has not passed review because (see checked entries): > > ___ Your RR template is generally incomplete; it has too many blank entries > that need proper data filled in. > > ___ You have failed to nominate the proper persons for review and push. > > ___ Your patches do not have proper short+long header > > ___ You have grammar/spelling in your header that is unacceptable. > > ___ You have exceeded a sensible line length in your headers/comments/text. > > ___ You have failed to put in a proper Trac Ticket # into your commits. > > ___ You have incorrectly put/left internal data in your comments/files > (i.e. internal bug tracking tool IDs, product names etc) > > ___ You have not given any evidence of testing beyond basic build tests. > Demonstrate some level of runtime or other sanity testing. > > ___ You have ^M present in some of your files. These have to be removed. > > ___ You have needlessly changed whitespace or added whitespace crimes > like trailing spaces, or spaces before tabs. > > ___ You have mixed real technical changes with whitespace and other > cosmetic code cleanup changes. These have to be separate commits. > > ___ You need to refactor your submission into logical chunks; there is > too much content into a single commit. > > ___ You have extraneous garbage in your review (merge commits etc) > > ___ You have giant attachments which should never have been sent; > Instead you should place your content in a public tree to be pulled. > > ___ You have too many commits attached to an e-mail; resend as threaded > commits, or place in a public tree for a pull. > > ___ You have resent this content multiple times without a clear indication > of what has changed between each re-send. > > ___ You have failed to adequately and individually address all of the > comments and change requests that were proposed in the initial review. > > ___ You have a misconfigured ~/.hgrc file (i.e. username, email etc) > > ___ Your computer have a badly configured date and time; confusing the > the threaded patch review. > > ___ Your changes affect IPC mechanism, and you don't present any results > for in-service upgradability test. > > ___ Your changes affect user manual and documentation, your patch series > do not contain the patch that updates the Doxygen manual. > |