Opened 9 years ago

Closed 9 years ago

#423 closed task (fixed)

simple master file loader

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: y2 12 month milestone
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This is a task for the A team sprint, but is a relatively generic feature that can be used for other purposes.

It parses a "normalized" zone file, where "normalized" mean:

  • no directives ($xxx, @, etc)
  • TTLs are numbers
  • no multiline RR
  • no omission (owner name for RRs of the same owner name, TTL, RR Class)
  • no relative owner names
  • RRs of a single RRset are not interleaved with RRs of a different RRset. That is, the following sequence shouldn't happen:
    • example.com. 3600 IN A 192.0.2.1
    • example.com. 3600 IN AAAA 2001:db8::1
    • example.com. 3600 IN A 192.0.2.2
  • no mixed ordering of RR parameters; only accept the form of <owner name> <TTL> <RR class> <RR type> <Rdata (single line)>

note: BIND 9's named-compilezone (from text to text) can convert an arbitrary zone file in the "normalized" form:

 % named-compilezone -f text -F text -o example.com.norm example.com example.com.zone

This can be a simple free function (not a class), which takes a path to the zone file, the origin name, and RR class, and a callback functor. It calls the functor for every RRset built from the zone file. Some level of validation against origin/RR class should better be performed, but if it takes time it can be skipped at this time. In either case it should be documented.

Subtickets

Change History (8)

comment:1 Changed 9 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from new to reviewing

branches/trac423 is ready for review.

This is a relatively big among the tasks of this sprint, but I believe the main part of code is easy to understand.

The most tricky (and perhaps controversial) part would be the supplemental MasterLoadCallback class. I introduced this so that we can accept variuos kinds of callbacks (functors/functions) transparently, while still avoiding relying on boost.function. If this part of the code is difficult to understand or the reviewer doesn't agree with the idea of "in-house" version of boost.function, we could simply replace it with the following typedef:

typedef boost::function<void(RRsetPtr rrset)> MasterLoadCallback;

and move forward, while discussing whether to rely on boost.function in our public header files project-wide.

comment:2 Changed 9 years ago by jinmei

I guess we need a changelog entry for this ticket. This is the proposal:

  126.?	[func]		jinmei
	src/lib/dns: Added new functions masterLoad() for loading master
	zone files.  The initial implementation can only parse a limited
	form of master files, but BIND 9's named-compilezone can convert
	any valid zone file into the acceptable form.
	(Trac #423, svn rTBD)

comment:3 Changed 9 years ago by zzchen_pku

  • Owner changed from UnAssigned to zzchen_pku

comment:4 follow-up: Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to jinmei

Seems ok, some comments below:
master.h
-Exception name 'MasterError?' is ambiguous, since the exception is related to master file loading.
-We have discussed on boost dependency for a long time:-) In my personal opinion, because boost.function has been introduced into std::fr1, and it is a general interface, we don't have to add a supplemental class to replace it, which will reduce the readability of our code.
master.c
-It seems the variable 'prev_rrset' can be replaced by 'rrset'.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 9 years ago by jinmei

  • Owner changed from jinmei to zzchen_pku

Replying to zzchen_pku:

Seems ok, some comments below:
master.h
-Exception name 'MasterError?' is ambiguous, since the exception is related to master file loading.

Hmm maybe we should rename it MasterLoadError? In that case I'd also rename file names from master.xx to masterload.xx.

-We have discussed on boost dependency for a long time:-) In my personal opinion, because boost.function has been introduced into std::fr1, and it is a general interface, we don't have to add a supplemental class to replace it, which will reduce the readability of our code.

I think we have to agree to disagree here, but for this branch I adopted your preference and changed the definition using boost.function.

It's not about the spec is standardized or not. It's about maturity of implementation, especially in terms of binary compatibility. But this is a more general topic, and I guess we (all developers) should discuss this in f2f in January.

master.c
-It seems the variable 'prev_rrset' can be replaced by 'rrset'.

Ah, good catch. Fixed.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to jinmei

Replying to jinmei:

Hmm maybe we should rename it MasterLoadError? In that case I'd also rename file names from master.xx to masterload.xx.

That's fine, it make sense to me.

I think we have to agree to disagree here, but for this branch I adopted your preference and changed the definition using boost.function.
It's not about the spec is standardized or not. It's about maturity of implementation, especially in terms of binary compatibility. But this is a more general topic, and I guess we (all developers) should discuss this in f2f in January.

Agree. Currently both A and R teams are facing the problem, I think we should make it more clear:-)

All the others are ok, please merge.

comment:7 in reply to: ↑ 6 Changed 9 years ago by jinmei

Replying to zzchen_pku:

Replying to jinmei:

Hmm maybe we should rename it MasterLoadError? In that case I'd also rename file names from master.xx to masterload.xx.

That's fine, it make sense to me.

Okay, I've changed the branch accordingly.

I'm going to merge this latest branch to trunk. Thanks for review.

comment:8 Changed 9 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.