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

[BIND 10 Development]

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

 * owner:  muks => stephen


Comment:

 Hi Stephen

 Replying to [comment:10 stephen]:
 > Reviewed commit f6d053c16792e0a511af21ca758b14fcf26ae032
 >
 > '''src/lib/dns/master_loader.cc'''[[BR]
 > Copyright needs to be updated to 2012-2014.

 Done.

 > 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()).

 API doc has been written for all of `MasterLexerImpl` now.

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

 Fixed.

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

 Actually `i` was used to indicate any integer (as there's no other
 special meaning for every integer in the range in the `$GENERATE`
 directive). I've renamed it to `num`.

 > 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?

 This is now documented too.

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

 Now there is basic API doc and the implementation is also commented.

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

 Done.

 > 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".

 I don't see a mistake in the code. Did you misread the `%1[]` (digit
 `1`) as `%l[]` (alphabet `l`)?  The `%[doxXnN]` specifier matches a
 sequence of characters from the specified set. The maximum field width
 modifier (1)
 limits it to 1 character. It also needs space for the trailing 0, so a
 `char[2]` buffer is supplied.

 > 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".

 The variables have been renamed.

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

 Now there is basic API doc and the implementation is also commented.

 > 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.

 The implementation is fully commented now.

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

 Done.

 > 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.

 Done.

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

 Added.

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

 No it won't (to keep it compatible with BIND 9). A comment has been
 added to indicate this.

 > '''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?

 A query for the generated NS records was also added to the lettuce test.

 > 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.

 There is this line in lettuce:
 {{{
         And wait for bind10 stderr message CMDCTL_STARTED
 }}}

 Perhaps I have not understood what you mean above.

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

 Done.

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