BIND 10 #3086: Develop NameChangeTransaction base class for b10-dhcp-ddns
BIND 10 Development
do-not-reply at isc.org
Tue Sep 3 17:54:00 UTC 2013
#3086: Develop NameChangeTransaction base class for b10-dhcp-ddns
-------------------------------------+-------------------------------------
Reporter: tmark | Owner: tomek
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 tmark):
* owner: tmark => tomek
Comment:
Replying to [comment:8 tomek]:
> General comments:
> 1. I like the size of this ticket. It's a welcome change after multi-day
> reviews.
>
I do try to please ;)
> 2. Many get/set methods do not check if the passed smart-pointer is
> not NULL.
ON getters or setters which reference ncr_, the !NameChangeTransaction
constructor guarantees ncr_ is not null, and since it retains a copy of
the
smart pointer, it cannot actually be deleted while the transaction exists.
I have removed setDnsUpdateResponse(), as I do not believe its needed.
>
> 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.
I have actually implemented this in work on a subsequent ticket,
trac3087 which is creating the first derivation, !NameAddTransaction. I
have
identified other few pieces, as anticipated in my submission comments for
this
ticket.
>
> 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.
Should I start this a separate ticket?
>
> 5. There is no ChangeLog entry.
>
Oops. I could have sworn I proposed one. How's this:
6xx. [func] tmark
Added update transaction base class, NameChangeTransaction,
to b10-dhcp-ddns. This class provides the common structure
and methods to implement the state models described in the
DHCP_DDNS design, plus integration with DNSClient and its
callback mechanism for asynchronous IO with the DNS servers.
> '''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.
>
Sometimes I get ahead of myself. I have removed it.
>
> '''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.
>
NCR's default constructor permits it, so technically it could be. The
default constructor is used to start create a blank NCR that gets filled
in as you un-marshall values from the JSON form (see NCR::fromJSON). NCR
does have a validateContent() method which is used after all values are
populated, and it tests for this condition. The other NCR constructor
calls validateContent() directly.
So it is technically possible for an NCR to have neither set but you would
have to do so deliberately and you would have to bypass a good deal of the
system to get it into a transaction.
I can test for it again in !NameChangeTransaction constructor if you like
but I think it is unnecessary.
> setState(): There should be a sanity check on the state passed.
>
Currently, states are integers so there isn't much of a way to sanity
check, especially when both the base class and the derived class may
define states. "sanity" checking is really left to whether or not a state
value maps to a state handler. This is checked and caught via
runStateModel() when it invokes getStateHandler().
> initServerSelection(): domain pointer could be passed as const.
> What would happen if I passed NULL pointer? :)
Well that would be bad. Why would you do that??? ;) I added an exception
toss.
>
> '''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 disagree:
"To sum it up: The C++ standard considers struct x; to be a declaration
and struct x {}; a definition. (In other words, "forward declaration" is
something of a misnomer, since there are no other forms of class
declarations in C++.) "
Besdies all my other .h files say "defines" therefore I am right ;)
>
> 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?
>
While they are used by D2, they are also used by dhcp4, dhcp6, etc. It is
conceivable that they could be used by customers for other things as well.
Unless you feel strongly, I would prefer to leave it. (Besides you
reviewed
this already when I created the library, no fair taking a second
helping!)
> Class comment: " the transaction's work is complete it is destroyed".
> It seems that a conjunction is missing in this sentence.
Quite right. Fixed it.
>
> 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 have created, ticket #3156
"Create separate base-classes for the Finite State Machine logic in
!NameChangeTransaction class".
I may do this work, immediately following the conclusion of this ticket.
It would make the derivations from !NameChangeTransaction cleaner, I
think.
>
> 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 :)
I was just testing your attention to detail. I have corrected this.
>
> !NameChangeTransaction desctructor should be documented.
Oops.
>
> 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.
I have removed it.
>
> 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!
>
Fixed the param name. Thank you! It is pretty nifty. I am just starting
to monkey about with it.
> setState(): Does this method do any sanity checking? It should check
> if the state is allowed. Otherise setState(123456); will mess things
> up.
Answered above.
>
> 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().
A NameChangeRequest may call for both the forward and reverse updates be
made.
These flags are intended as "post-mortem" flags, so that if transaction
fails,
it is possible to tell which portion(s) worked, if any.
>
> initServerSelection() domain parameter should be const.
Done.
>
> 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?
As we discussed earlier, the NameChangeRequest should contain all of the
lease
information, including the DHCID regardless of whether it calls for
forward
update, reverse update or both.
>
> 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()
I changed the comment to indicate the pointer returned will be empty if
the request does not call for a change in that direction.
>
> getForwardChangeCompleted() and getReverseChangeCompleted(): see
> comment about setForwardChangeCompleted() and
> setReverseChangeCompleted().
See above response to above comment ;).
>
> '''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)
NCR construction already employs a validateContent() method when
constructing them from the "wire", unless you deliberately circumvent a
good deal of code you can't really get one in here that isn't sane unless
memory has been stomped and you are crashing anyway.
>
> 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.
>
Quite right.
> !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.
I'm not sure if your issue is with the test structure or the class
implementation itself. What I have done is to add a method to the base
class, verifyStateHandlerMap, that is called after initStateHandlerMap. It
is up to the derivation to implement the actual verification.
This gives the derivation a chance to validate the map prior to executing
the state model. Without a way to know what the list of valid states
are, there is good way for the base class to implement the verification.
I think this is a reasonable compromise. Assuming the FSM implementation
continues to mature, it is something that could be revisited.
>
> The test only checks if getStateHandler does not throw. The method
> can return complete garbage (nulls, random data) and the test would
> pass.
>
You will find that I have revised the tests quite a bit. This particular
issue is addressed in the test, basicStateMapping. It retrieves and
attempts to execute a handler, whose outcome can be verified.
> !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.
I really did drop the ball on documenting this test. You will find this
much
improved.
>
> There is not tests for operator(), setDnsUpdateStatus(),
> setDnsUpdateResponse(), setForwardChangeCompleted(),
> setReverseChangeCompleted(), getPrevState(), getDnsUpdateStatus(),
> getForwardChangeCompleted(), and getReverseChangeCompleted().
>
For your viewing pleasure, I have added a test, basicAccessors, which
should address your concerns.
--
Ticket URL: <http://bind10.isc.org/ticket/3086#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list