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      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:11 muks]:

 > > Ideally, the spec file should be more descriptive so it can be
 > > rejected at that level, but as far as I know we need to provide some
 > > default anyway (in this case, an empty string).  So, technically, we
 > > cannot distinguish intentionally specified empty string from
 > > unspecified config, but in practice if an empty string is passed it's
 > > most likely that the user forgets to fill in it.
 > >
 > > If you agree on this, we could more localize the fixes (we don't have
 > > to modify name.h, etc).  But I'll comment on the changes in the branch
 >
 > I agree, but please elaborate further.

 ??? I don't understand this.  By "But I'll comment on the changes in
 the branch anyway", I meant: I could have stopped there and confirmed
 whether or not we agree on where the issue was and made comments on
 the actual changes only if not (because if we agreed, the minimum
 necessary change could just be a few line of changes only within
 auth_config.cc); but I actually also made comments on the rest of the
 changes in the branch rather than waiting.  Do you need something
 more/else?

 > > - as convention we generally add '[ticket #]' in the beginning of each
 > >   commit log message.  So, in this case it should be '[1627]'.  If you
 > >   don't have a particular reason not to follow this convention, I'd
 > >   suggest you do the same for consistency.
 >
 > [number] is parsed by Trac as a link to a commit. #number is parsed by
 Trac
 > and linked to a bug. Instead of using #number, I used "bug #number"
 because
 > when editing a git commit log in an editor, lines that start with # are
 > assumed as comments and not included in the commit log message.

 Ah, okay.  In that case using '#' may make more sense.  In either case
 I think we should use a consistent style, so maybe we should discuss
 this at the mailing list.

 > 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.

 > > - 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.

 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.
 }}}
 - also, I think we should add test cases for empty/missing zone or
   file name.
 - this could be simplified:
 {{{#!c++
             imzf = new InMemoryZoneFinder(rrclass_,
                                           Name(origin->stringValue()));
 }}}
   to:
 {{{#!c++
             imzf = new InMemoryZoneFinder(rrclass_, Name(origin_txt));
 }}}

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