BIND 10 #1206: Create datasource factory

[BIND 10 Development]

#1206: Create datasource factory
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  stephen
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20111011
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  4
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => stephen


Comment:

 >
 > '''src/lib/datasrc/Makefile.am'''[[BR]]
 > Just a comment on the file names - the names of the memory data source
 files in the separate DLL are of the form memory_datasrc.*, but those for
 the SQLite3 data source are sqlite3_accessor.*.  On the other hand,
 sqlite3_datasrc - and static_datasrc are included in the main library.
 This is confusing.
 >

 Yes. I also noticed some weird stuff while working on 1179, and I think we
 should move things around and change some names, but wasn't sure if this
 was
 the place to do it...

 I'll create a new ticket for this

 > '''src/lib/datasrc/client.h'''[[BR]]
 > Good description of !DataSourceClientContainer, thanks.  It really helps
 the understanding of the code.
 >

 thank you :)

 > The description of the return value in getSym() should indicate that
 NULL will be returned if the symbols does not exist.
 >

 actually, on error, or if the symbol does not exist, an exception is
 raised. If
 getSym() returns NULL, it means the symbol *does* exist, but has the value
 NULL
 itself. I've added that to the description.


 > '''src/lib/datasrc/factory.h'''[[BR]]
 > There are a few #includes commented out.
 >

 oops, removed two of them. I realized last night that the two new classes
 should actually be noncopyable, so I left that one in and actually use it.

 > I assume that the constructor takes a std::string because this is
 something read from the configuration, whereas getSym takes a char*
 because the names of the symbols retrieved are literal character strings
 in the code?  (If so, a note in the getSym() header to this effect would
 be useful.)
 >

 added

 > Perhaps want to add another exception - !DataSourceLibrarySymbolError -
 to distinguish between a failure to load a library and a failure to access
 a symbol in the library?
 >

 good idea, done

 > '''src/lib/datasrc/factory.cc'''[[BR]]
 > !DataSourceClientContainer constructor: Suggest using reinterpret_cast
 instead of the C-style casts when casting the returned symbol to the
 correct type.
 >

 ohyeah, meant to do that, i knew i forgot something. It does remind me
 that i
 do not actually know if every compiler accepts this... anyway changed for
 now.

 > Question: in what directory is the DLL located?  The reason for this
 question is that I would see that there are two locations that would be in
 use.  The first is the library directory of the BIND 10 installation
 (which may be a directory specified on the "configure" command line).  The
 second is a site-specific directory in which local data sources are
 stored.  If the location of the latter were stored in the configuration
 database somewhere, and that were preserved across BIND 10 upgrades, it
 would be possible to upgrade BIND 10 without affecting the local data
 source.
 >

 hmm. Interesting idea. I do think we can do something to that effect as a
 separate enhancement, and for now leave them in the 'main system' standard
 lib
 path.

 > '''src/lib/datasrc/memory_datasrc.cc'''[[BR]]
 > addError(): by the time the code gets here, "errors" should be non-null.
 (Especially as this is a function in the anonymous namespace and visible
 only to functions declared below it - and all paths through those
 functions create the error list.)  Under these circumstances, can't
 "errors->add()" be called directly from within the code?
 >

 see below

 > '''src/lib/datasrc/sqlite3_accessor.cc'''[[BR]]
 > checkConfig() can also be placed in the anonymous namespace (as it is in
 memory_datasrc.cc) - there is no reason for it to be visible outside the
 module.

 The answer to both is, 'ack, but for now'. I was actually considering
 making
 checkConfig() an externally visible option, where errors may be null (if
 you
 don't care about the errors but only need to know whether it's good).

 Since i wasn't sure we needed it (we may know once we start turning it
 on), it
 now lives halfway between the two options.

 So for now here's my proposal; I do indeed move checkConfig to anonymous
 namespace, but I leave the check in addError (it's not harmful, it's not
 on a
 critical code path, and it will probably prevent a problem of
 forgetfullnes
 should we decide to export the symbols)

 Done in separate commit, you may respectfully disagree ;)

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