BIND 10 #3075: Implement Main Event Loop in D2Process
BIND 10 Development
do-not-reply at isc.org
Tue Aug 27 08:13:14 UTC 2013
#3075: Implement Main Event Loop in D2Process
-------------------------------------+-------------------------------------
Reporter: tmark | Owner: tmark
Type: enhancement | Status:
Priority: medium | reviewing
Component: dhcp-ddns | Milestone:
Keywords: Task# 6.2.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 marcin):
* owner: marcin => tmark
Comment:
Replying to [comment:7 tmark]:
> Replying to [comment:6 marcin]:
> > Reviewed commit c39eb9bbe30285a2b19fea86473b63ddb758506b
> >
> > The code has a very good quality and it is very well documented.
Thanks for that.
> >
>
> Thank you, kindly.
The unit tests failed for me:
{{{
d2_process_unittests.cc: In member function ‘virtual void
{anonymous}::D2ProcessTest_canShutdown_Test::TestBody()’:
d2_process_unittests.cc:480:167: error: converting ‘false’ to pointer type
for argument 1 of ‘char
testing::internal::IsNullLiteralHelper(testing::internal::Secret*)’
[-Werror=conversion-null]
d2_process_unittests.cc:481:167: error: converting ‘false’ to pointer type
for argument 1 of ‘char
testing::internal::IsNullLiteralHelper(testing::internal::Secret*)’
[-Werror=conversion-null]
d2_process_unittests.cc:482:167: error: converting ‘false’ to pointer type
for argument 1 of ‘char
testing::internal::IsNullLiteralHelper(testing::internal::Secret*)’
[-Werror=conversion-null]
d2_process_unittests.cc:489:167: error: converting ‘false’ to pointer type
for argument 1 of ‘char
testing::internal::IsNullLiteralHelper(testing::internal::Secret*)’
[-Werror=conversion-null]
d2_process_unittests.cc:490:167: error: converting ‘false’ to pointer type
for argument 1 of ‘char
testing::internal::IsNullLiteralHelper(testing::internal::Secret*)’
[-Werror=conversion-null]
d2_process_unittests.cc:500:167: error: converting ‘false’ to pointer type
for argument 1 of ‘char
testing::internal::IsNullLiteralHelper(testing::internal::Secret*)’
[-Werror=conversion-null]
d2_process_unittests.cc:501:167: error: converting ‘false’ to pointer type
for argument 1 of ‘char
testing::internal::IsNullLiteralHelper(testing::internal::Secret*)’
[-Werror=conversion-null]
d2_process_unittests.cc:537:167: error: converting ‘false’ to pointer type
for argument 1 of ‘char
testing::internal::IsNullLiteralHelper(testing::internal::Secret*)’
[-Werror=conversion-null]
d2_process_unittests.cc:550:167: error: converting ‘false’ to pointer type
for argument 1 of ‘char
testing::internal::IsNullLiteralHelper(testing::internal::Secret*)’
[-Werror=conversion-null]
d2_process_unittests.cc:551:167: error: converting ‘false’ to pointer type
for argument 1 of ‘char
testing::internal::IsNullLiteralHelper(testing::internal::Secret*)’
[-Werror=conversion-null]
cc1plus: all warnings being treated as errors
make: *** [d2_unittests-d2_process_unittests.o] Error 1
}}}
Part of the problem is that you use this construct:
{{{
EXPECT_EQ(false, checkCanShutdown(SD_NORMAL));
}}}
instead of
{{{
EXPECT_FALSE(checkCanShutdown(SD_NORMAL));
}}}
Although, I admit I don't know why EXPECT_EQ doesn't work.
>
> > I think it requires a !Changelog. I am hoping I haven't simply missed
it in the ticket again.
> >
>
> Good point, I did not offer a !ChangeLog entry. How about something like
this?
>
> 6xx. [func] tmark
> Added main process event loop to D2Process which is the primary
> application object in b10-dchp-ddns. This allows DHCP-DDNS
> to queue requests received from clients for processing while
> listening for cmd control events.
>
Suggest that !''cmd!'' is replaced with !''command!''.
Also, please remember to add the commit number when you commit the new
entry.
I pushed a couple of trivial fixes, which were easier for me to make
rather than explain what should be fixed.
>
> > Other comments:
> >
> > '''src/bin/d2/d2_config.cc'''
> > Invalid alignment of the second line in the constructor definition:
> > {{{
> > TSIGKeyInfoListParser::TSIGKeyInfoListParser(const std::string&
list_name,
> > TSIGKeyInfoMapPtr keys)
> > }}}
> >
> > build and commit: The concept of the separation of build and commit
methods is that the error prone part should be done in build() and the
commit() should be exception free (see description of abstract methods:
commit and build in src/lib/dhcpsrv/dhcp_config_parsers.h). The commit()
could only do:
> > {{{
> > *keys_ = *local_keys_;
> > }}}
> > Everything else should be moved to the build(). In fact, similar
changes should be done for !TSIGKeyInfoParser.
> >
>
> I understand what you are describing and don't disagree with you. I did
not pick up on that particular seperation of function between build and
commit. If you look through d2_config.cc, you will see that I followed
the same pattern throughout. I am willing to rework them as part of a
separate ticket, specifically to address your issue. I would prefer not to
do a large scale restructure of the parsing within this ticket (3075).
That's fine. However, please submit a ticket (if there isn't one submitted
already), so as we don't forget about this issue.
>
>
> > TSIGKeyInfoParser::commit: typo ''Algorithme''
>
> Fixed.
>
In fact, it is not fixed.
> >
> > '''src/bin/d2/d2_config.h'''
> > Doxygen description of commit has a typo ''This causes each parser to
instantiate a TSIGKeyInfo from its internal data values and add '''that
that''' key instance to the local key storage area, local_keys_. ''. Also
spurious space after ''local_keys_''.
> >
>
> Fixed.
This hasn't been fixed too. Did you push your changes?
>
> > '''src/bin/d2/d2_messages.mes'''
> > Garbage text in the first line:
''/Users/tmark/ddns/build/new3075/bind10''.
> >
>
> Fixed. I had little script around the reorder_message_file.py that was
echoing out the directory.
Ok. Cool script BTW :-)
> >
> >
> > init: Wouldn't it make sense to call this method when the object is
instantiated? I know it is empty at the moment, but if you already call
it, I don't have to bother thinking where I should call it from.
Especially, that I am not an expert of D2Process class, because I haven't
created it. If method is not called, it doesn't make sense to define it.
> >
>
> This method is actually called by DController::initProcess. This method
is responsible for instantiating the process under its control and then
invoking this method. I kept it apart from the constructor thinking that
constructors are not necessarily the place to attempt opening IO or other
resources. Keep in mind the DController/DProcess were intended to be
general purpose.
Ok. Got it.
> >
> > '''src/bin/d2/d2_process.h'''
> >
> > Is there any reason why text labels for shutdown types are declared
'''public''' in the header file? Will they be used externally or they are
just helper constants for the internal use of the !D2Process class?
>
> They are just helpers but is there harm in them being public? You never
know when somebody might want them.
If you think that will be used in the future, it is fine to leave them.
However, in such case, instead of associating them with the Shutdown types
by their names, I would rather add a function which would translate the
string argument to the shutdown type. Note, that there is a function
which translates from the shutdown type to string. If you concur, please
add a function, if not, you're fine to leave it.
>
> >
> > canShutdown: could this function be const?
>
> Apparently yes, but I had to change DProcess::shouldShutdown to const to
do it. ;)
Well, it makes sense for shouldShutdown to be const anyway. :-)
>
> > getShutdownTypeStr: the argument could be const.
> >
>
> Done.
Do you pass the enum by reference to defend yourself against cppcheck? :-)
>
> >
> > '''src/bin/d2/d2_queue_mgr.cc'''
> > operator() is not guarded from exceptions, which may be potentially
thrown by the stopListening. Note that we expect this operator to be
exception free.
> >
>
> Actually, the only exception that can be thrown is a programmatic
exception, in which case you're toast anyway.
NameChangeListener->stopListening, intentionally does not throw.
>
> I have a added a try-catch around it which logs but swallows an
exception. There isn't much of way to do anything sensible as we
shouldn't be able to get here.
ok.
> >
> >
> > '''src/bin/d2/dhcp-ddns.spec'''
> > I am not certain, but it looks to me that ''Shuts down '''b10-dhcp-
ddns module''' '' would be more appropriate for the command_description.
Is DHCP_DDNS a server? in some sense it is, but it may be considered as a
part of the DHCPv6 server, so using the term DHCP_DDNS server is a little
confusing. How do you think?
> >
>
> Good catch, you're right, I changed it.
Ok. But I had to fix a typo.
> >
> > '''src/lib/dhcp_ddns/dhcp_ddns_messages.mes'''
> > Garbage before the copyright header:
''/Users/tmark/ddns/build/trac3075/bind10''
>
> got it.
But it was still there. I removed it along with other minor fixes.
--
Ticket URL: <http://bind10.isc.org/ticket/3075#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list