BIND 10 #1976: use meta-or-container-of data source in b10-auth

[BIND 10 Development]

#1976: use meta-or-container-of data source in b10-auth
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120731
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  b10-auth                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  8
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => vorner


Comment:

 Hi vorner

 Here are my review comments:

 * Have you addressed the test suggestions in the earlier review?

 * In `auth_srv.cc`, `90593cc3f05fbd272f11fc57c876487f9dbc3813` seems to
 leave a comment:
 {{{
      /// In-memory data source.  Currently class IN only for simplicity.
 }}}

 * Can the `cache-enable` key be renamed to simply `cache` as it's boolean?
 `cache-zones` can remain as-is.

 * In `ConfigurableClientList::findInternal()` which returns nothing now,
 the following comment is obsolete:
 {{{
     // Return the partial match we have. In case we didn't want a partial
     // match, this surely contains the original empty result.
 }}}

 If findInternal() does not touch candidate when there's no match,
 candidate isn't reset. Granted that this is a private function, I think a
 comment that a reset candidate should be passed to it would be good.

 * In `ConfigurableClientList::configure()`, what is the purpose of
 `allow_cache` argument? It cross checks with `dconf->get("cache-enable")`,
 but why can't it simply use that key instead?

 * `ConfigurableClientList::configure()` can throw `isc::Unexpected` too
 (should be added in the doxygen doc)

 * In `prepareCache()`, is the following comment always true?
 {{{
 +        // We leave the zone empty, so we can check it was reloaded.
 }}}

 * In `ListTest.reloadNoSuchZone`, is the comment correct?
 {{{
 +    list_->configure(config_elem_zones_, true);
 +    Name name("example.org");
 +    // We put the cache in even when not enabled. This won't confuse the
 thing.
 +    prepareCache(0, Name("example.com"));
 }}}

 * In `ListTest.reloadMasterFile`, currently the code modifies the zone in
 memory and reloads the master file to see if it was reset to the old data.
 Can you add a test that modifies the master file instead and checks if it
 shows up after the reload?

 * In `src/auth/command.cc`, the following comment for `validate()` should
 be modified:
 {{{
     // A helper private method to parse and validate command parameters.
     // On success, it sets 'old_zone_finder_' to the zone to be updated.
     // It returns true if everything is okay; and false if the command is
     // valid but there's no need for further process.
 }}}

 * You may simply want to fold `validate()` into `LoadZoneCommand::exec()`

 * Thank you for indenting the lettuce config files

 * I have pushed these patches:

 {{{
 * 65a9d49 [1976] Make some minor typo/grammar changes
 * c7462ec [1976] Remove unused member variable
 * 6adec61 [1976] Add some static casts to fix compile errors
 }}}

 System and lettuce tests pass.

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