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:[email protected]]
Sent: den 22 november 2013 13:47
To: [email protected]
Cc: [email protected]
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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
[email protected]
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
