BIND 10 #3075: Implement Main Event Loop in D2Process
BIND 10 Development
do-not-reply at isc.org
Tue Aug 27 12:20:51 UTC 2013
#3075: Implement Main Event Loop in D2Process
-------------------------------------+-------------------------------------
Reporter: tmark | Owner:
Type: enhancement | marcin
Priority: medium | Status:
Component: dhcp-ddns | reviewing
Keywords: Task# 6.2.1 | Milestone:
Sensitive: 0 | Sprint-DHCP-20130904
Sub-Project: DHCP | Resolution:
Estimated Difficulty: 0 | CVSS Scoring:
Total Hours: 0 | Defect Severity: N/A
| Feature Depending on Ticket:
| Add Hours to Ticket: 0
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by tmark):
* owner: tmark => marcin
Comment:
Replying to [comment:8 marcin]:
> 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.
This is actually a bug in gtest but they don't plan to fix it. I have
changed the tests to use EXPECT_FALSE. What's funny is that it only fails
for false.
>
> >
> > > 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.
Got it.
>
>
> 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.
I created #3121 to cover this.
>
> >
> >
> > > 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?
>
I had corrected both of the above and then began to correct the parser
structuring. Part way through I decided the restructure was too large for
this ticket so I rolled back those changes and unfortunately rolled back
the above two changes as well.
> >
> > > '''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.
>
Moved the string constants into getShutdownTypeStr().
> >
> > >
> > > 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?
:-)
Why yes I did.
>
> >
> > >
> > > '''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:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list