BIND 10 #3086: Develop NameChangeTransaction base class for b10-dhcp-ddns
BIND 10 Development
do-not-reply at isc.org
Fri Aug 30 15:00:22 UTC 2013
#3086: Develop NameChangeTransaction base class for b10-dhcp-ddns
-------------------------------------+-------------------------------------
Reporter: tmark | Owner: tmark
Type: enhancement | Status:
Priority: medium | reviewing
Component: dhcp-ddns | Milestone:
Keywords: Task# 4.1 | Sprint-DHCP-20130904
Sensitive: 0 | Resolution:
Sub-Project: DHCP | CVSS Scoring:
Estimated Difficulty: 0 | Defect Severity: N/A
Total Hours: 0 | Feature Depending on Ticket:
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by tomek):
* owner: tomek => tmark
Comment:
General comments:
1. I like the size of this ticket. It's a welcome change after multi-day
reviews.
2. Many get/set methods do not check if the passed smart-pointer is
not NULL.
3. Do you see a need to implement onEnter() and onExit()
methods? That's a honest question. I have implemented FSM a long time
ago for a simulation I did. I found them very useful, besides
onEvent(). If you're interested, there's even some documentation for
my my code: http://klub.com.pl/numbat/doxygen/d3/dde/features.html
The code is mostly a crap, though.
4. I like those diagrams. Please create src/bin/d2/d2.dox page and
add the diagrams there. Please update doc/devel/mainpage.dox to point
out to it. This new page doesn't have to be big. Couple sentences will
be sufficient. Please try to explain the big picture, rather than the
details. This doc should grow over time an the diagrams you made will
fit in there nicely.
5. There is no ChangeLog entry.
'''d2_update_mgr.cc'''
Why did you add a code that is disabled? If you want to keep it, please
add
a @todo comment that explains when it should be reenabled.
'''nc_trans.cc'''
!NameChangeTransaction constructor:
It is possible that !NameChangeRequest be neither forward nor reverse?
It can be, according to !NameChangeRequest constructor. I'm not familiar
with the NCR creation process, but I would think that such a
no-fwd/no-reverse update does not make sense, so it should throw in
NCR constructor. If that is not feasible for whatever reason, the
second best step would be to add a check to !NameChangeTransaction ctor
and throw there.
setState(): There should be a sanity check on the state passed.
initServerSelection(): domain pointer could be passed as const.
What would happen if I passed NULL pointer? :)
'''nc_trans.h'''
File comment. Strictly speaking, the header declares, not defines the
class. See
http://stackoverflow.com/questions/1410563/what-is-the-difference-
between-a-definition-and-a-declaration
for an interesting discussion.
I see that there's a separate namespace for dhcp_ddns and d2.
Perhaps it would be easier to use just one namespace for both,
just as isc::dhcp is shared by libdhcp, libdhcpsrv, dhcp4 and dhcp6?
Class comment: " the transaction's work is complete it is destroyed".
It seems that a conjunction is missing in this sentence.
I like this class a lot. It is essentially Finite State Machine.
However, it is too deeply tied to DNS Updates. It is fine to keep it
as it is, but I'll like it to be extracted to a separate class.
One notable example where it will be useful is DHCP failover (both
v4 and v6). There are states for end-points that could benefit
from this code. Anyway, there's not much to do here in this regard.
Please simply add a @todo comment and create separate ticket for
extracting state mechine routines into separate, generic class.
I'd like to have comments for some states to be clarified. For example:
{{{
/// @brief State in which forward DNS server selection is done.
static const int SELECTING_FWD_SERVER_ST = 2;
}}}
This can be either interpreted as a state where selection is
being done (selecting) or the state immediately after selection
has been done already (selected).
Comment for NameChangeTransaction constructor. The exception thrown
is NameChangeTransactionError. Most expects agree that it is
considered a bad practice if a class throws itself, especially in
constructor :)
!NameChangeTransaction desctructor should be documented.
cancelTranscation(): You may have doubts if the method should be
there, but if it's there, it must be commented. I wouldn't implement
this method if I were in your shoes, because cancelling an update
is useful for optimizations only. That's definitely not needed yet.
However, since you already took time to implement it, I would leave
it in. Not sure how much my advice is worth - I don't know much
about D2 and DDNS in general.
addToMap: comment explains state parameter, but it is called
idx. Also, nice trick with boost::bind(). I did not know that
before. Well played, sir!
setState(): Does this method do any sanity checking? It should check
if the state is allowed. Otherise setState(123456); will mess things
up.
setForwardChangeCompleted() and setReverseChangeCompleted(). How many
packets can one change generate? I thought only one - forward or
reverse. So those two methods could be merged into
setChangeCompleted().
initServerSelection() domain parameter should be const.
getTransactionKey() - If we want to do both AAAA and PTR, how
many transactions are there: 1 or 2? I don't recall the details, but
I think that DHCID is used only for forward updates, right?
So what would getTransactionKey() return if we had only reverse
update?
getNcrStatus(): "Once the transaction is reached it's conclusion"
=> "Once the transaction has reached its conclusion"
getForwardDomain(): What does it mean "meaningless" in this context?
This method should probably throw if getForwardDomain() is called on
a NCT that is reverse.
getReverseDomain(): similar to getForwardDomain()
getForwardChangeCompleted() and getReverseChangeCompleted(): see
comment about setForwardChangeCompleted() and
setReverseChangeCompleted().
'''nc_trans_unittests.cc'''
Please replace existing domain names with something that belongs to
example.com or other domains reserved in RFC2606 for documentation and
example purposes. I'm sure that our code is completely bug free and
firewall in our lab is completely packet tight, but in case it isn't,
we should not spam anyone on the Internet.
NameChangeTransaction.construction should also check if
passed ncr is sane (i.e. if at lease one of isForwardChange() or
isReverseChange() is true)
That comment in line 262 is invalid:
{{{
// Verify that an empty forward domain is allowed when the requests
does
// include a forward change.
}}}
It should be "does not include", right?
The same is true for the next comment (line 268) after it.
!NameChangeTransactionTest.stateModelTest. I sort of don't like
the apporach that we check if the state handler is there and
then we throw when it isn't. This could lead to cases where the state
machine throws in some obscure conditions. It would be better
if there was a kind of init and check function, similar to
initStateHandlerMap() that would check if for every allowed
state there is a state handler.
The test only checks if getStateHandler does not throw. The method
can return complete garbage (nulls, random data) and the test would
pass.
!NameChangeTransactionTest.serverSelectionTest test:
I have trouble understanding what's going on in this test. Couple
extra comments would be helpful. I managed to get it, but it took
me some time.
There is not tests for operator(), setDnsUpdateStatus(),
setDnsUpdateResponse(), setForwardChangeCompleted(),
setReverseChangeCompleted(), getPrevState(), getDnsUpdateStatus(),
getForwardChangeCompleted(), and getReverseChangeCompleted().
--
Ticket URL: <http://bind10.isc.org/ticket/3086#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list