BIND 10 #1627: Error: Server configuration failed: incomplete textual name

[BIND 10 Development]

#1627: Error: Server configuration failed: incomplete textual name
-------------------------------------+-------------------------------------
                   Reporter:  jreed  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:         |  Sprint-20120403
  medium                             |            Resolution:
                  Component:  bind-  |             Sensitive:  0
  ctl                                |           Sub-Project:  Core
                   Keywords:         |  Estimated Difficulty:  2
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Replying to [comment:12 jinmei]:
 > > I agree on avoiding inner try-catch blocks, but am not sure if
 maintaining a context would be less work than prepending the context and
 rethrowing, when something is thrown.
 >
 > I have a feeling that keeping a context will be useful so we can
 > produce useful exception what() (which in this context will be used as
 > a response message to the operator) in a consistent manner, even if we
 > keep the micro try-catch.  But at the moment that would probably be
 > over generalization.  So, I'm okay with leaving the block for now, but
 > I'd suggest adding some comments like:
 >
 > {{{#!c++
 >         // Note: we don't want to have such small try-catch block for
 each
 >         // specific error.  We may eventually want to introduce some
 unified
 >         // error handling framework as we have more configuration
 parameters.
 >         // See #1627 for the relevant discussion.
 >         InMemoryZoneFinder* imzf = NULL;
 > }}}
 >
 > so we can revisit the point later.

 This comment has been added.


 > > > - I think it a good idea to introduce some intermediate class
 > > >   hierarchy in for exception classes in libdns++ (in fact there
 should
 > > >   be a ticket for that, I think), but I'd like to do it based on
 some
 > > >   consistent design consideration, rather than by introducing an
 > > >   intermediate class in ad hoc manner as we see the need for it.
 > > >   e.g., isc::Exception => isc::dns::Exception => isc::dns::NameError
 > > >         => isc::dns::NameParserError => isc::dns::EmptyLabel
 > >
 > > This is perhaps good for another ticket too. I had a reason to add the
 NameParserException parent for this bug. :) We can change it however we
 want.
 >
 > I'll create a separate ticket for the redesign work.  On thinking how
 > it would probably go, I now think `NameParserError` is okay.

 Cool :)


 > Comments on the revised code:
 >
 > - I made a couple of more style fixes.
 > - one test now fails:
 > {{{
 > config_unittest.cc:335: Failure
 > Expected: parser->build(Element::fromJSON( "[{\"type\": \"memory\"," "
 \"zones\": [{\"origin\": \"example..com\"," "               \"file\":
 \"example.zone\"}]}]")) throws an exception of type EmptyLabel.
 >   Actual: it throws a different type.
 > }}}

 Oh yeah.. a different kind of exception now is thrown (as the
 NameParserException is now caught). I must have missed running make check.
 Fixed. :)

 > - also, I think we should add test cases for empty/missing zone or
 >   file name.

 Added.

 > - this could be simplified:
 > {{{#!c++
 >             imzf = new InMemoryZoneFinder(rrclass_,
 >                                           Name(origin->stringValue()));
 > }}}
 >   to:
 > {{{#!c++
 >             imzf = new InMemoryZoneFinder(rrclass_, Name(origin_txt));
 > }}}

 Done for both origin and file.

 Returning for re-review.

-- 
Ticket URL: <http://bind10.isc.org/ticket/1627#comment:14>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development