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

BIND 10 Development do-not-reply at isc.org
Fri Jul 6 00:59:59 UTC 2012


#1976: use meta-or-container-of data source in b10-auth
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120717
  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

 The following are my review comments. I have read the rest and have not
 found anything could be an issue. However, as I'm looking at a lot of this
 code for the first time as well, I suggest that someone else like Jinmei
 also review this branch. I have reviewed up to commit
 `af4e7787d37b951f9a48e992f0e2c6f4e4a28c22` ([1976] Add missing test file).

 * There's a spelling mistake here:

 {{{
 +++ b/src/bin/auth/datasrc_configurator.h
    +    /// is unkown and it would be questionable at least). It is called
 }}}

 * Can you rename `DataSourceConfiguratorGeneric::deinit()` to `cleanup()`?

 * Can only one AuthSrv instance use `DataSourceConfigurator`, if `init()`
 is called with it?

 * Why is `DataSourceConfiguratorGeneric::reconfigure()` public?

 * Can `doInInit()` be renamed to something else so it reads clearly, such
 as `initializeList()`?

 * This comment is incorrect in `DatasrcConfiguratorTest::multiple()`:
 {{{
 +    // This one does not set
 +    EXPECT_EQ("get CH\nset CH xxx\nget IN\nset IN yyy\n", log_);
 }}}

 * Similarly in `DatasrcConfiguratorTest::updateAdd()` too

 * It's probably good to have `lists_.size()` check in
 `DatasrcConfiguratorTest::createList()` and
 `DatasrcConfiguratorTest::modifyList()` too

 * You should add `lists_.size()` check to
 DatasrcConfiguratorTest::updateDelete()

 * Can you change this to read "Can't reconfigure when not initialized with
 init()" in `reconfigure()`:
 {{{
 +        if (server_ == NULL) {
 +            isc_throw(isc::InvalidOperation,
 +                      "Can't reconfigure while not inited");
 +        }
 }}}

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


More information about the bind10-tickets mailing list