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-20110830
                  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:

 Replying to [comment:8 vorner]:

 I pushed one const fix that some compilers gonna want in
 sqlite3_accessor_unittest.cc


 >
 > I went for the getAllRecords(), but I'm waiting for the modification to
 return the iterator for renaming the second part.
 >

 ack. Let's defer the other change to the unification (or refactor-
 refactor-)ticket, which i'll create shortly

 > > 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 unified it by adding a fifth (well, in C++, fourth) column for the
 name. That one is unused in case of findRecords, but I think it doesn't
 matter much. Now we can reuse the iterator, if it is (in case of SQLite
 one) slightly modified in the constructor.
 >

 ok cool

 > > 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.
 >
 > I don't think it helps much, but anyway, I added it.
 >

 As discussed shortly on jabber, if we are going for a always-fixed-length
 array in which, depending on context, the calls update specific fields, we
 can actually enforce the array size compile-time (by using a ref-to-
 array), and we don't need the second argument anymore.

 But this too we can defer to the refactor-refactor-ticket

 > > 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.
 >
 > I'm not sure I like that way. The zone is clearly inconsistent and
 broken, so in that case we're serving bad data.
 >
 > And anyway, this should be at last WARN, I believe. What else should we
 warn about, if not about clearly bad data ;-). And we might create a tool
 that would scan whole zone/datasource and show suggestions/recommendations
 sometime in future.

 See jinmei's comment :)

 Other than that the changes look OK to me.

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