Here are my review comments for #49 NO_DANGLING First of all its a NACK in the current state. Both due to minor things and major things.
I think you have done a great job in creating what is "probably" a working solution for the problem. The most major comment I have is that we need to re-organize some of the no-dangling code. In particula, try to demarcate it more clearly and in some cases try to break parts of it out into separate functions. This is particularly needed for ImmModel::ccbObjectModify() which is now way way way too complex. You have also done some cleanup in my old code. Thats ok for trivial things like removing white space at end of lines. More complex changes should be done in a separate patch if it is not directly related to this enhancement. ------------------- Another general comment I have, or a lets say a general thought, about this enhancement is that it increases the emphasis on usage of the SaNameT type. We can then expect many applications increasing their use of the SaNameT type when they get this new eagerly awaited feature. This is a bit unfortunate, as at the same time we have the well known basic problems with the SaNameT type. That being addressed by the prototype that Anders Widell is working on. The current prototype AndersW is working on is in line with a strategy of moving *away* from SaNameT towards using SaStringT as the physical type for DNs (not just RDNs). One way of getting arround that problem, in the *next* OpensAF release, is to add a new attribute flag SA_IMM_ATTR_DN, to be used when SaStringT is used as the physical type for storing a reference. If an SaStringT attribute definition has this new flag set, it would mean that the attribute is to be interpreted as holding a DN, and all checks for DN validity would be applied, including the NO_DANGLING checks. So all of this means that if/when we push the enhancement for #191 (the SaNameT fix) then in the same OpenSAF release we should add this new SA_IMM_ATTR_DN flag and adjust the NO_DANGLING logic so that it works with that new flag. Currently it does not look like #191 will be poushed in OpenSAF 4.4. Ideally though the order between the #40 (NO_DANGLING) and #191 (long DN) enhacements should have been the reverse. First fix the long DN problem and then add new ways of using DNs in the IMM. -------------------------------------------------------------------------------------------------------- 2) The coment describing the IMM_NO_DANGLING_FLAG says it is part of the SaImmCcbFlagsT space, But actually it is part of the mObjeFlags space in ObjectInfo. ------------------------------------------------------------------------------------------------------- 3) The comment explaining sReverseRefObjectMMap should be placed after the declaration. Actually the first comment line could start on the same line as the declaration. If you look at the other static declarations and their commets, that is the pattern. So best follow the same pattern otherwise one could think the comment refers to sObjectMap. ---------------------------------------------------------------------------------------------------- 4) The function ImmModel::checkReferences() needs a clearer comment decribing what it does. Possibly this function should be called validateNoDangling, as it is only invoked in ccbApply? Currently the function only has this comment: // Add missing object to sReverseRefObjectMMap without any check // The main reference check will be done below in checkModificationReferences // to avoid double checks First I guess that should be plural (add missing objects) since you are iterating? Also the referral to checkModificationReferences is confusing because no such function exists. My best guess is that you mean checkReferencesModify? In general, functions dedicated to no-danbgling should have a clear name making it obvious that They are dealing with no-dangling. This goes for variables also. We need to come up with a short Prefix that clearly signifies no-dangling. --------------------------------------------------------------------------------------------------- 5) Current logic does not allow RDN attributes to have NO_DANGLING flag set. I think it should be allowed to have the NO_DANGLING flag set for RDN attributes if the type of the RDN Attribute is SaNameT (or in the future if SA_IMM_ATTR_DN flag is also set with SaStringT). If the RDN attribute is the DN of another object, then it means that the object is a so called association object. Most association objects I would expect are designed with the expectation that not only the parrent exists, buit also that the object referred to by the RDN attribute should exist. Which is just saying that association objects should probably set the NO_DANGLING flag from now on. Of course existing association objects dont have the NO_DANGLING flag set, which is ok since the IMM also does not check that the RDN points to a valid object. Thus existing AMF association objects will NOT be monitored for NO_DANGLING unless these AMF classes are upgraded to have The flag set. ---------------------------------------------------------------------------------------- 6) Method ImmModel::removeObjectReferences() Needs a short comment explaining what the function does and the meaning of the three input arguments. It should say that it Is only concerned with NO_DANGLING references. Probably the function could be called something like removeReverseNdReferences ? ----------------------------------------------------------------------------------- 7) General comment: There are sometimes comments like "Remove all references" which are not as helpfull/clear as they could be. I think they typically mean "remove all reverse references associated with this operation". --------------------------------------------------------------------------------------- 8) Method ImmModel::addObjectReferences() Needs a short comment. It should say that it Is only concerned with NO_DANGLING references. Probably the function could be called addReverseNdReferences() ? --------------------------------------------------------------------------------------- 9) There is a big chunk of code inserted at the end of the function ImmModel::verifySchemaChange All of this functionality deals with the two cases of the NO_DANGLING flag being added or not. But if you look at the structure of the existing code, cases of dealing with adding or removing any particular flag are handled in the function ImmModel::notCompatibleAtt(). I think the NO_DANGLING flag case should appear in the notCompatibleAtt() function in a way similar to that of NO_DUPLICATES. You turn on a flag checkNoDangling and probably checkCcb. The big chunk of code does ccb-interference check. I think it typically is more efficient to iterate through all CCBs (of which there are usually quite few active) and check each objecty in each such CCB (of which there is typically one); rather than iterate through the whole class extent and check each object if it is part of a ccb ... So this ccb interference check (with schema change) should be able to reuse the exising ccbs iteration inside notCompatibleAtt(). Besies this reorganisation, much of the logic of the existing patch could perhaps be reused. But perhaps the already coarse interference check done in notCompatibleATt is sufficent. Basically it aborts the CCB if it is non critical and touches any instance of the class, Or aborts the schema change if the CCB is critical and touches any instance of the class. Note that if notCompatibleAtt returns true, then that becomes verifyFailed==true and the schema change is aborted. ------------------------------------------------------------------------------------------- 10) Please use brackets to enclose body of if/else/for/while statements even if it is only one statement. E.g avoid: if(newAttr->mValueType != SA_IMM_ATTR_SANAMET) verifyFailed = true; // Collect attributes with added NO_DANGLING flags for(ObjectSet::iterator osi=oldClassInfo->mExtent.begin(); The code becomes both harder to read and is easier to break if someone "only" inserts say an unlucky TRACE statement in that if statemeent. I know we may have "tools påroblem" here, but anynoes/everynoes favorite tool Can not define the coding style. And in this case its not just stale, it is safer To bracket even one line code bodies. ---------------------------------------------------------------------------------------- 11) In ImmModel::checkReferencesModify() Dont understand the comment: /* Empty attribute */ ----------------------------------------------------------------------------------------- 12) In ImmModel::checkReferencesModify() This function *also* checks for ccb interference. But the function is only invoked from ccbApply where ccb-interference should NOT be an issue. Ccb interference should be checked as part of the modify operation. If there is interference, the modify is rejected (typically nonfataly for the ccb). If there is no interference when the operation is invoked, the afim is created etc and it is the duty of any *ohter* ccb trying to add an opertion in that other ccb to check for interference with this ccb. Thus if this ccb encounters no other problems, then the only thing to check for in apply should be *validity*, i.e. That any missing creates or missing deletes or missing modifications in this ccb Have been added so that the complete set of changes poposed by *this* ccb is complete (do not violate NO_DANGLING). This imm level validation must be done before sending any completed callbacks to Ois (which should be the case already here). ---------------------------------------------------------------------------------------- 13) In ImmModel::ccbObjectCreate() + /* Check object references */ Should be + /* Check for CCB interference on NO_DANGLING references */ --------------------------------------------------------------------------------------- 14) In ImmModel::ccbObjectCreate() This log message: + LOG_NO("ERR_BAD_OPERATION: NO_DANGLING attribute %s " + "refers to object that will be deleted (Ccb %u)", + i4->first.c_str(), ccbId); Should me adjusted to: + LOG_NO("ERR_BAD_OPERATION: NO_DANGLING attribute %s " + "refers to object '%s' scheduled for delete in same Ccb: %u", + i4->first.c_str(), omi->first.c_str(), ccbId); That is print out also the DN for the object to be deleted in same ccb. Better to say "scheduled" since it is not totally certain that this ccb will commit. ------------------------------------------------------------------------------- 15) In ImmModel::ccbObjectCreate() This log message: + LOG_IN("ERR_BUSY: NO_DANGLING attribute %s refers to " + "flagged object for deletion (Ccb %u)", + i4->first.c_str(), ccbId); + err = SA_AIS_ERR_BUSY; Shoud be adjusted to: + LOG_IN("ERR_BUSY: NO_DANGLING attribute %s refers to " + "object '%s' scheduled for deletion in other Ccb %u", + i4->first.c_str(), omi->first.c_str(), ccbId); + err = SA_AIS_ERR_BUSY; ------------------------------------------------------------------------------------------ 16)In ImmModel::ccbObjectCreate() Similarly: + if(omi->second->mObjFlags & IMM_CREATE_LOCK) { + if(omi->second->mCcbId != ccbId) { + LOG_IN("ERR_BUSY: NO_DANGLING attribute %s refers to " + "flagged object for creation (Ccb %u)", + i4->first.c_str(), ccbId); Should be: + if(omi->second->mObjFlags & IMM_CREATE_LOCK) { + if(omi->second->mCcbId != ccbId) { + LOG_IN("ERR_BUSY: NO_DANGLING attribute %s refers to " + "object '%s' scheduled for creation in different Ccb %u", + i4->first.c_str(), omi->first.c_str(), ccbId); ------------------------------------------------------------------------------------------ 17) In ImmModel::ccbObjectCreate() I dont quite understand why you are using: ObjectMap refObjectMap; It should be sufficient with just an ObjectSet. You dont need to duplicate the object DN here. Because you never do lookup using DN on this Map and never pick up the x->first member. The object map is just popuplated here: refObjectMap[omi->first] = omi->second; And used here: for(i5 = refObjectMap.begin(); i5!= refObjectMap.end(); ++i5) sReverseRefObjectMMap.insert(std::pair<ObjectInfo *, ObjectInfo *>(i5->second, object)); So please use ObjectSet which is already typedef'ed. And *please* use brackets arround single statement bodies for if/else/for/while/do-while .. Its ok to put the brackets on the same line, as the single code statement. -------------------------------------------------------------------------------------------- 18) ImmModel::collectObjectReferences I suggest this function be renamed to something ImmModel::collectRefsNoDangling. A header comment could state that it is for one object, if that is not obvious. --------------------------------------------------------------------------------------------- 19) ImmModel::ccbObjectModify Suggest rename of: + bool hasNoDanglingRefs = false; The term: has-no-dangling-refs is ambiguous, it could either mean that something DOES NOT have dangling references or that something DOES have references declared as no-dangling. But it seems to mean neither of these. I suggest: bool modifiesNoDanglingAttr = false; ??? Since it seems to be used for capturing that a no dangling attribute is being modified. Similarly: + bool hasNoDanglingAttr = false; suggest bool hasAttrNoDangling = false; --------------------- 20)ImmModel::ccbObjectModify + // Count missing references Lets stick with "dangling references" and not also use the term "missing references". Or is there some subtle difference here ? Suggest: // Adjust dangling references counters And + std::map<std::string, int>::iterator mrci; // Missing Reference Counter iterator Suggest: std::map<std::string, int>::iterator mrci; // Dangling References Counter iterator But I realize here that mrci is "missing reference counter iterator" and I fear that renaming *everything* Will risk that you get lost in the code to the same extent that I am now in this review ! What I am trying to say is that it usually pays long term to get as crisp and clear and unambiguous names as possible. Particularly in complex code like this. But I realize here that we need to do more Than just get clearer names and better comments. We need to break out the no-dangling parts, at least For the place where they add entire sections of code to ccbModify. I fail to convince myself that the code is correct in ccbModify. I think we need to go through this code together interactively. Where I get lost is with the function local variable: std::map<std::string, int> missingRefCounter; It is not populated or initilalized relative to the ccb in any way. Yet it is used in the modification-operation iteration directly. This is the first usage: if(momi != ccb->mMissingRefObjects.end()) { mrci = missingRefCounter.find(av->getValueC_str()); if(mrci != missingRefCounter.end()) { mrci->second--; } else { missingRefCounter[av->getValueC_str()] = -1; } } This means you can get a negative missingRefCounter. The counter is a signed int so at least it is consistent with the type to get a negative value. But what does a negative missing reference count mean here ? I guess it means it will cancel *if* there is a dangling reference added, but the count could then become zero, which seems strange if it *is* dangling. I think we need to clearly de-marcate the no-dangling code and break out some of the no-dangling logic into function calls. ----------------------------------------------------------------------------------- I will skip trying to review the rest of the server side patch. /AndersBJ -----Original Message----- From: Zoran Milinkovic [mailto:zoran.milinko...@ericsson.com] Sent: den 22 november 2013 13:47 To: reddy.neelaka...@oracle.com Cc: opensaf-devel@lists.sourceforge.net Subject: [devel] [PATCH 0 of 3] Review Request for IMM: Implementation of NO_DANGLING flag [#49] Summary: IMM: Implementation of NO_DANGLING flag [#49] Review request for Trac Ticket(s): 49 Peer Reviewer(s): Neelakanta, Anders Pull request to: Zoran Affected branch(es): default(4.4) Development branch: default(4.4) -------------------------------- Impacted area Impact y/n -------------------------------- Docs n Build system n RPM/packaging n Configuration files n Startup scripts n SAF services y OpenSAF services n Core libraries n Samples n Tests n Other n Comments (indicate scope for each "y" above): --------------------------------------------- changeset fdcd5898fc78be44582f741fe4c177ff3dba1c1f Author: Zoran Milinkovic <zoran.milinko...@ericsson.com> Date: Fri, 22 Nov 2013 13:23:03 +0100 IMM: API support for NO_DANGLING flag [#49] IMM API support for reference integrity (NO_DANGLING flag) changeset e3545d58fec23b0634acb045fcb0ecdf9fb471a3 Author: Zoran Milinkovic <zoran.milinko...@ericsson.com> Date: Fri, 22 Nov 2013 13:28:23 +0100 IMM: IMM service implementation of NO_DANGLING flag [#49] Implementation of reference integrity (NO_DANGLING flag) to IMMSV changeset 4f1ffe45aeb8facce248e40c26f4e42bf64f7bcb Author: Zoran Milinkovic <zoran.milinko...@ericsson.com> Date: Fri, 22 Nov 2013 13:34:11 +0100 IMMTOOLS: Add support of NO_DANGLING flag to IMM tools [#49] Support reference integrity (NO_DANGLING flag) to IMM tools Added Files: ------------ osaf/libs/saf/include/saImmOm_A_2_13.h osaf/services/saf/immsv/schema/SAI-AIS-IMM-XSD-A.02.13.xsd Complete diffstat: ------------------ osaf/libs/agents/saf/imma/imma_om_api.c | 36 +++- osaf/libs/common/immsv/include/immpbe_dump.hh | 2 +- osaf/libs/saf/include/Makefile.am | 3 +- osaf/libs/saf/include/saImmOm_A_2_12.h | 2 + osaf/libs/saf/include/saImmOm_A_2_13.h | 60 +++++++ osaf/libs/saf/libSaImm/Makefile.am | 2 +- osaf/services/saf/immsv/immloadd/imm_loader.cc | 8 +- osaf/services/saf/immsv/immloadd/imm_pbe_load.cc | 2 +- osaf/services/saf/immsv/immnd/ImmAttrValue.cc | 6 + osaf/services/saf/immsv/immnd/ImmAttrValue.hh | 2 + osaf/services/saf/immsv/immnd/ImmModel.cc | 1196 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------- osaf/services/saf/immsv/immnd/ImmModel.hh | 32 +++- osaf/services/saf/immsv/schema/SAI-AIS-IMM-XSD-A.02.13.xsd | 180 +++++++++++++++++++++ osaf/tools/safimm/immcfg/imm_import.cc | 2 + osaf/tools/safimm/immdump/imm_xmlw_dump.cc | 9 + osaf/tools/safimm/immlist/imm_list.c | 3 + 16 files changed, 1461 insertions(+), 84 deletions(-) Testing Commands: ----------------- Testing, Expected Results: -------------------------- The patches need to support: - immload support for NO_DANGLING flag - immdump support for NO_DANGLING flag - immcfg support for NO_DANGLING flag (immcfg -f) - immlist support for NO_DANGLING flag (NO_DANGLING flag is listed) - IMM library support for NO_DANGLING (classCreate - basic checks) - schema update (adding and removing NO_DANGLING flag) - default values - create object (NO_DANGLING attributes with and without values) - modify object (add, replace and delete) - delete object - CCB apply - CCB abort/terminate - support for missing objects, created later within the same CCB Conditions of Submission: ------------------------- Ack from Neelakanta and Anders 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. ------------------------------------------------------------------------------ Shape the Mobile Experience: Free Subscription Software experts and developers: Be at the forefront of tech innovation. Intel(R) Software Adrenaline delivers strategic insight and game-changing conversations that shape the rapidly evolving mobile landscape. Sign up now. http://pubads.g.doubleclick.net/gampad/clk?id=63431311&iu=/4140/ostg.clktrk _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel ------------------------------------------------------------------------------ Rapidly troubleshoot problems before they affect your business. Most IT organizations don't have a clear picture of how application performance affects their revenue. With AppDynamics, you get 100% visibility into your Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro! http://pubads.g.doubleclick.net/gampad/clk?id=84349351&iu=/4140/ostg.clktrk _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel