BIND 10 #374: Writable datasources: Basic support for writables in C++ datasrc library
BIND 10 Development
do-not-reply at isc.org
Fri Nov 12 11:12:54 UTC 2010
#374: Writable datasources: Basic support for writables in C++ datasrc library
------------------------------+---------------------------------------------
Reporter: jelte | Owner: UnAssigned
Type: enhancement | Status: reviewing
Priority: major | Milestone:
Component: data source | Resolution:
Keywords: | Sensitive: 0
Estimatedhours: 0.0 | Hours: 0
Billable: 1 | Totalhours: 0
Internal: 0 |
------------------------------+---------------------------------------------
Comment(by jinmei):
continuing review...this is the last chunk of comments.
'''sqlite3_unittest.cc'''
- I made some trivial editorial changes to the branch (r3509)
- general comments
- there are mixture of ASSERT_EQ() and EXPECT_EQ(), and some
of them don't seem to have to be ASSERT. did you intentionally use
ASSERT_EQ() where it's used?
- according to lcov there are some parts that are not tested, including:
- some of the "higher level" methods are not tested directly.
- some FORMERR cases in updateCheckPrerequisite()
- some error cases in updateProcessUpdate()
- same for doIXFR()
- overall, the newly added tests rather seem to belong to the higher
level layer. I didn't see much (if any) part in the tests that's
specific to sqlite3. also, if we test them at the higher level
using some mock data source, it will be easier to test cases with
DB errors.
- SQLITE_DBFILE_WRITE could (should) be of ConstElementPtr.
- zone_name could (should) be of const Name.
- zone_name is doubly defined (in the scope of Sqlite3DataSourceTest.)
is that intentional?
- do we need to copy the writable data source file to the build dir
in the test? ... ah, I see we need to do that to "reset the
DB". actually, in that case, I'd introduce a separate class, always
rest the DB in each test's initialization phase (constructor or
SetUp()). see also the next bullet and the relevant comment on the
delRRset test below.
- install_writable_database() should be named installWritableDatabase?
- many of the new tests first switch to the writable data source.
maybe we should introduce a separate test class
- Transactions test
- I don't understand this comment:
{{{
// need rrsetit for doIXFR (but doIXFR should probably be moved up to
datasource itself)
}}}
(btw it's too long). maybe it's relevant to rrset_it defined above
(note that it's not "rrsetit"), which is unused in the test.
- delRRset test
- I'm afraid it's not a good idea to rely on a side effect
of another test. for example, we cannot selectively run this test:
{{{
% ./run_unittests --gtest_filter=Sqlite3DataSourceTest.delRRset
(this fails)
}}}
- these should be "delete it", and "add it", resp:
{{{
// add it, but roll back
...
// add it, and commit
}}}
- getRRsetCallback_empty() should be named getRRsetCallbackEmpty?
- replaceZone_callback_empty test
- a very minor nit, but considering the test name is a function
name, this should probably be replaceZoneCallbackEmpty? same for
other test names.
- replaceZone_new_zone test
- 'rrset' could (should) be a vector of ConstRRsetPtr.
- initialization of soa_rrset could be a bit simplified:
{{{
RRsetPtr soa_rrset(new RRset(Name("new.example.com"),
RRClass::IN(),
RRType::SOA(),
RRTTL(3600)));
}}}
same comment applies to some other similar cases, too.
- namespace can be omitted for createRdata(). this will help
prevent the parameter lines from being too long (damaging readability).
- general comments about helper functions
- I think it should be defined somewhere else (possibly with some
generalization) so that we can share it in other test cases. we
might also want to integrate this feature into
lib/dns/tests/testdata/gen-wiredata.py
- from this point of view, it's not very clear how to populate test
data other than by reading short comments about rrsetsFromFile()
and source code + test examples. we'll need more friendly
description about how to use it. (admittedly, gen-wiredata.py is
bad on this point, too, and dns/unittest_util is not very good
either)
- questionFromFile()
- I believe the return value could/should be of ConstQuestionPtr.
- error cases after operator>> or getline() are ignored (this
comment applies to other helper functions). Understanding this is
just a helper function in tests, I could live with it.
Personally, however, I'd still to catch error cases, because the
input is handmade data and we sometimes make errors. If we still
ignore it, please at least leave a comment explaining that's
intentional (for simplicity, etc).
- a specific related point. this code effectively works as EOF
determination:
{{{
string s;
file >> s;
if (s == "") {
return (QuestionPtr());
}
}}}
but IMO it's fragile because it's implicit and some specific value
in s.
- with this parsing code we'd accept multi-line input like this
{{{
example.com IN
SOA
}}}
Is that intentional? If so, is it okay to accept the following
example?
{{{
example.com IN
SOA another.example IN A 192.0.2.1
}}}
("another.example..." will simply be ignored)
- if multi-line input isn't expected, I'd suggest the following
revise:
- call readline() at the outer loop in rrsetsFromFile() and
updateFromFile(), and pass the extracted to string a string
version of rrsetFromFile() (say, rrsetFromString()):
{{{
string line;
while (getline(ifs, line), !ifs.eof()) {
if (ifs.bad() || ifs.fail()) {
//error handling
}
ConstRRsetPtr rrset = rrsetFromString(line);
}}}
- in rrsetFromString() use istringstream to parse that line:
{{{
rrsetFromFile(const string& line) {
istringstream ss(line);
string s;
ss >> s;
...
}
}}}
- the call to getline() at the end of this function will result in
ignoring erroneous input as a question like "example IN A
192.0.2.1" (which is valid for other sections). shouldn't we rather
catch and reject that?
- rrsetFromFile()
- most of the comments on questionFromFile() apply here, too
- initialization of rrset could be simplified:
{{{
RRsetPtr rrset(new RRset(n, rrclass, rrtype, ttl));
}}}
- I'd use stringbuf to get the "rest of the line" (skipping spaces):
{{{
stringbuf sbuf;
is >> &sbuf;
if (ss.fail()) {
// no additional input
} else if (!ss.bad()) {
rrset->addRdata(createRdata(rrtype, rrclass, sbuf.str()));
}
}}}
- the call to addRdata() could be simplified as shown in the above
example (without using a temporary variable 'rdata')
- rrsetsFromFile() (msg version)
- the first block of comment seems to be out of sync. it doesn't
set flags/codes; it doesn't only handle the answer section.
- the code logic inside the while loop is difficult to understand to
me due to multiple nested conditions with mutable variables. At
least there are some obviously redundant code:
- this test is unnecessary
{{{
if (rrset) {
}}}
because at this point rrset must be non NULL.
- updating prev_rrset can be unified:
{{{
prev_rrset = rrset;
} else {
prev_rrset = rrset;
}
}}}
thereby eliminating an unnecessary else clause.
But, frankly, I'd be still not sure if the code is sufficiently
readable. I'd feel a bit more comfortable with the following code:
{{{
while (! file.eof() ) {
const RRsetPtr rrset = rrsetFromFile(file);
const bool same_type =
(rrset && prev_rrset &&
prev_rrset->getName() == rrset->getName() &&
prev_rrset->getType() == rrset->getType() &&
prev_rrset->getType() != RRType::SOA() &&
prev_rrset->getClass() == rrset->getClass());
// if this is the end of the section or we've got a new type of
RRset,
// add the RRset we've constructed so far, if any.
if (prev_rrset && (!rrset || !same_type)) {
msg.addRRset(section, prev_rrset);
}
// if this is the end of the section we are done.
if (!rrset) {
return (1);
}
// update prev_rrset: if this is a new RR of the same type, merge
it.
// otherwise replace it.
if (same_type) {
RdataIteratorPtr it = rrset->getRdataIterator();
for (it->first(); !it->isLast(); it->next()) {
prev_rrset->addRdata(it->getCurrent());
}
} else {
prev_rrset = rrset;
}
}
}}}
The point is to unify the point of doing addRRset(), and to make
rrset a const variable (note: not ConstRRsetPtr) so that prev_rrset
is the only mutable variable. But I must admit this code is not
obviously clearer than the original, so if you don't agree, please
ignore the suggestion and move forward with the trivial cleanup by
removing the redundancy.
(for that matter, we may need a "merge mode" for the RRset class or
introduce a notion of "RR" and provide a primitive of "addRR" for
usage like this. but that's a different topic)
- as already noted, file.eof() never happens.
- the intent of special case for SOA isn't so obvious. maybe we
need some comment here.
- rrsetsFromFile (ifstream + vector version)
- most of the code is a duplicate of the other version. I see the
need for refactoring. we could use a template with some helper
class, or unify the main code in the vector version and have the
msg version use it.
- updateFromFile()
- this code is not very readable to me, due to the tricky usage with
stage (and the magic numbers for 'stage'). it looks like if we
unified the interface of questionFromFile() with that of
rrsetsFromFile(), the code would become more straightforward:
{{{
while (! myfile.eof() ) {
questionFromFile(myfile, m);
rrsetsFromFile(myfile, m, Section::ANSWER());
rrsetsFromFile(myfile, m, Section::AUTHORITY());
rrsetsFromFile(myfile, m, Section::ADDITIONAL());
}
}}}
(and I guess we could even share the code for questionFromFile and
rrsetsFromFile, e.g. using a template).
- checkSingleRRset()
- text comparison is not really good (fragile to minor implementation
changes). ideally, for example, we'd pass a vector of RDATA
strings with name/type/class, then in checkSingleRRset() we'd
construct another vector of RDATA from the retrieved RRset, sort
both vectors and then compare them. We may also include such
tests in a shared place such as unittest_util. But, perhaps we
should live with the current tests within this ticket - it's
already too big with too many review comments.
- what's the purpose of commenting out a test?
{{{
//EXPECT_EQ(DataSrc::SUCCESS, find_flags);
}}}
if it's intentionally commented-out, please explain the intent;
otherwise, please simply remove it.
- rrsetsFromFile(): container could/should be a vector of ConstRRsetPtr.
- dynamic_update_bad_class test
- the comment says "no/bad question section", but doesn't it only
test the case for "no question section"? or perhaps you intended
to set a question section with the IN class?
- dynamic_update_prereq_fails test
- for "update4.packet", if we consider each RR (not RRset) a
prerequisite, it's fifth prereq that fails. same comment applies
to the subsequent updateX.packet. (but this is quite a minor point)
--
Ticket URL: <http://bind10.isc.org/ticket/374#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list