BIND 10 #2430: support $GENERATE in dns::MasterLoader

[BIND 10 Development]

#2430: support $GENERATE in dns::MasterLoader
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:  muks
                Type:  task          |                       Status:
            Priority:  high          |  reviewing
           Component:  libdns++      |                    Milestone:
            Keywords:                |  bind10-1.2-release-freeze
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DNS           |                 CVSS Scoring:
Estimated Difficulty:  5             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => muks


Comment:

 Reviewed commit f6d053c16792e0a511af21ca758b14fcf26ae032

 '''src/lib/dns/master_loader.cc'''[[BR]
 Copyright needs to be updated to 2012-2014.

 I wonder how this file got through its initial review.  It defines the
 internal !MasterLoaderImpl class but gives no documentation for it.

 Could we have header comments (at least brief comments) for the new
 functions (generateForIter(), doGenerate() and the standalone function
 genNibbles()).

 ''genNibbles()''
 s/seperator/separator/

 "i" is usually used as a loop index, not as an argument.

 Without knowing exactly what this function is supposed to do, it's not
 possible to check it for correctness.  In particular, should a separator
 be added if i is zero but "width" is non-zero?  Should it return a value
 if "width" is zero and i non-zero?


 ''MasterLoaderImpl::generateForIter()''
 No header means that it is not easy to check what the method is supposed
 to do.

 Comments explaining the cases would be helpful.  It took me some time to
 work out what each branch is for.

 The "sscanf" format string is "{%d,%u,%l[doxXnN]}".  The variables to
 receive the values are "int", "unsigned int" and "char[2]".  According to
 the sscanf documentation, the format strings "%ld", "%lo" etc. require
 that the receiving variable is a "long int".

 As a note, the BIND 9 Administrator's Reference Manual refers to the
 modifiers of the "$" substitution variable as "offset", "width" and
 "base".  The code uses "delta", "width" and "mode".

 ''MasterLoaderImpl::doGenerate()''
 No header means that it is not easy to check what the method is supposed
 to do.

 Comments inside the function as to what is being parsed and why would
 help.  I could decode it, but (for example) had to locate and read what
 parseRRParams() did before I could understand the part of the code where
 it was used.



 '''src/lib/dns/tests/master_loader_unittest.cc'''[[BR]]
 Copyright needs to be updated to 2012-2014.

 Add a comment in the "generate" test to the effect that the test not only
 checks that $GENERATE creates the correct records, the checks for the
 "before" and "after" RRs also check that it doesn't generate too many
 records.

 There is a "generateInFront" tests and a "generateInMiddle" test.  A
 logical extension would be a "generateAtEnd" test.

 In the generateWithModifiers test, does the junk type generate an error
 and, if so, will this show up in the error count?

 '''tests/lettuce/features/master_loader.feature'''[[BR]]
 As the generate.zone file also generated the NS records for the
 "example.org." zone, is it possible to query for the NS records of the
 zone?

 In the test of processes that should/should not be running, b10-cmdctl
 should be running (according to the configuration), yet no check  is made
 on whether it is running.

 '''tests/lettuce/features/terrain/terrain.py'''[[BR]]
 Copyright needs to be updated to 2011-2014.

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