BIND 10 #1067: support zone iterator in the new data source API

[BIND 10 Development]

#1067: support zone iterator in the new data source API
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110816
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  4.0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => vorner


Comment:

 I have taken the liberty to commit a few style-fixes.

 I also have some naming suggestions: first is getIterator (as discussed, i
 want
 to modify searchForRecords to return an iterator as well; i think we
 should
 consistently name them; either we make them something like
 getRecords(name) and
 getAllRecords(), or getRecordIterator() and getZoneIterator())

 And even though is it a local class, i suggest the name of Iterator is
 changed
 to DatabaseIterator (so it matches the naming structure of MemoryIterator)


 sqlite3_accessor.cc:
 getNext(); I don't think sqlite3_step() should only check for SQLITE_ROW
 as a
 result value; it can signal the 'end' of the data directly with
 SQLITE_DONE, and
 otherwise there is an error we should probably throw an exception (that
 includes
 sqlite3_errmsg()).

 as discussed on jabber, instead of getstr() we can use the
 convertToPlainChar()
 that is in master now (it also does a bit of extra error checking), though
 i
 would not object to renaming it :)

 also, we should use an enum for the columns instead of hardcoded numbers
 for the
 different fields. I was going to suggest reusing the same we use in
 getNextRecord() (the RecordColumns enum), but it's actually different
 values
 that are returned here, so we can't simply do that.

 I originally intended to suggest discussing either making it 4 direct
 arguments
 again, but if we want one interface for both (and perhaps more) iterator
 types,
 we can't do that either. Nor can we use a trick like getNext(std::string
 (&data)[4]) to enforce the row size (We might change the number of columns
 in
 the other call to 3).

 So I would suggest to add a second argument to getNext(), the size of the
 array.
 Checking this number (and passing the right one) will be a contract-level
 issue,
 but it should, at least in theory, reduce the change of out-of-bounds
 writing to
 the array.

 iterator.h: getNextRRset(); we should probably add to the documentation
 that if
 a (datasource)error is raised, the iterator cannot safely be used anymore
 (data
 might get lost, and with at least the current implementation, if it is a
 different-TTL problem, you'll get a looping error if you happen to catch
 the
 exception inside the loop that calls getNextRRset())

 memory_datasrc.h: 'friend' statements tend to set off alarm bells, so i
 suggest
 we add some comments as to why we use/need them.

 database.cc: note; it could be open for discussion, but the 1062 version
 'fixes'
 differing TTL values (by modifying the ttl to the lowest one it finds, and
 logging an INFO message that the data should be fixed), instead of raising
 an
 error.

 And speaking of logging, perhaps we can use a few log messages here as
 well
 (though if we take the ones i added in 1062 as a measure it would be a
 couple at
 most, esp. since two or three in there will disappear once we use RAII
 through
 an iterator like this)

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