Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#48 closed task (fixed)

review Rdata class and some supplemental routines

Reported by: jinmei Owned by: jinmei
Priority: very high Milestone: 02. Running, functional authoritative-only server
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

Please review the jinmei-dnsrdata2 branch.

While it includes many changes, I'd specifically ask for review about
the following part of the changes (everything is under src/lib/dns/cpp):

  • rdata.cc, rdata.h
  • rrparamregistry-placeholder.cc, rrparamregistry.h: these have been partially reviewed, and the following part doesn't have to be reviewed: from bool CICharEqual() to RRParamRegistry::codeToClassText()
  • gen-rdatacode.py: this could be separated if the above two are already heavy.

Subtickets

Change History (9)

comment:1 Changed 9 years ago by jinmei

  • Priority changed from major to blocker

comment:2 Changed 9 years ago by jinmei

And relevant test code should also be reviewed:
src/lib/dns/cpp/tests/rdata_unittest.cc
src/lib/dns/cpp/tests/rrparamregistry_unittest.cc

comment:3 Changed 9 years ago by shane

  • Owner jinmei deleted
  • Status changed from new to assigned

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

As always, your test coverage seems very complete and your documentation is stellar.

(1) The generated files have two copyrights, one from the generation script and one from the source file.

(2) It looks like all this generation script does is add a very small number of lines to three files: one for well-known classes, one for well-known types, and another for well-known types to the registry. While the generate script is perhaps handy, it is also rather large itself, and I think it would have been easier and quite likely safer to document how to properly add a new class/type to these three files. The generate script requires a specific template for the new rdata to have that will require instruction to use the template anyway. We might as well document "also add it here." I think the generation script adds complexity for very little gain.

(3) rdata.cc has a method called compareNames(). I think this should be part of the name class. In any case, something that compares one name to another with DNSSEC ordering should probably not be in rdata.cc.

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

Replying to mgraff:

As always, your test coverage seems very complete and your
documentation is stellar.

Thanks for the review.

I'm responding to the review comments, but overall none of the points
is a blocking issue, it seems. So, unless I'm told otherwise, I'll
merge this branch to trunk soon, and create separate tickets for
remaining open issues (if any).

(1) The generated files have two copyrights, one from the

generation script and one from the source file.

There are several auto-generated files, and I'm assuming you're
specifically talking about rdataclass.{h,cc}. (Other auto-generated
files don't have this issue unless I miss something)

Yes, that's odd. I generally followed BIND9's approach here: it
hardcodes the top copyright in lib/dns/gen.c, and effectively
importing various (and slightly different) copyright notices from
implementation files by including them.

IMO the top copyright was redundant and actually inappropriate
(because it's not really associated with the copyrighted content).
So, if you're simply suggesting removing the head copyright from the
script, I'm happy with that.

(2) It looks like all this generation script does is add a very
small number of lines to three files: one for well-known classes,
one for well-known types, and another for well-known types to the
registry. While the generate script is perhaps handy, it is also
rather large itself, and I think it would have been easier and quite
likely safer to document how to properly add a new class/type to
these three files. The generate script requires a specific template
for the new rdata to have that will require instruction to use the
template anyway. We might as well document "also add it here." I
think the generation script adds complexity for very little gain.

I'm going to explain why I chose the script approach below, but first,
it's not a strong opinion. If you still think the additional
complexity doesn't outweigh the gain after reading the explanation, I
don't mind changing the approach (but that would be a post-year-1 task
I guess?). Also, opinion from Evan might be helpful. He has added
some new RR types using this scheme, and it's nice to know how useful
or not-so-useful he felt it was.

Like point (1) above, I followed the BIND9 approach here, and
regarding this point, since I thought it made sense. I thought it's
advantageous that the only thing a developer should do to support a
new RR type is to create the related header and implementation files
(e.g. rdata/generic/mx_15.{h,cc}): everything else is automatically
generated by the tool, especially a mapping between 15 and "MX", and
if it's the first RR type implemented for a specific RR class, mapping
between "CLASSnnn" and nnn.

One wouldn't be able to miss implementing mx_15.{h,cc} (though they
may be buggy or even empty) because the result wouldn't compile.
But even if the text-code mapping of an RR type is missing, the code
would still compile, and if it's not caught via unit tests, we'll only
notice that via a bug report on a release version. In theory, we're
supposed to be able to know the missing stuff via the thorough test
phase, but I don't trust humans so much:-)

I've also been planning, thought it's not yet implemented, to
auto-generate some test cases that are common for all RR types (like
the text-code mappings). Such a test would normally pass, but we may
still find something unexpected from such even most trivial tests
(e.g. there may be unexpected corner cases regarding the same RR types
of different classes like CH/A and IN/A), and since such tests look so
trivial and boring I'm afraid humans tend to overlook or ignore them
to add.

Another point is that this python script auto-generates common member
functions for derived RDATA class, such as this:

BEGIN_COMMON_MEMBERS

explicit A(const std::string& type_str);
explicit A(InputBuffer?& buffer, size_t rdata_len);
A(const A& other);
virtual std::string toText() const;
virtual void toWire(OutputBuffer?& buffer) const;
virtual void toWire(MessageRenderer?& renderer) const;
virtual int compare(const Rdata& other) const;

END_COMMON_MEMBERS

So the developer only needs to copy the template with the correct
class name

class MyType? : public Rdata {
public:

BEGIN_COMMON_MEMBERS
END_COMMON_MEMBERS

private:

RR-type specific members are here.

};

and a minimal number of type-specific members.

This is a solution to your concern that we'd have to repeat many
similar things to support new RR type. (I considered a C++ template
approach as you suggested, but soon realized it's not possible.
Templates generally require all the classes have exactly the same set
of members, but Rdata derived classes will definitely need specialized
members).

(3) rdata.cc has a method called compareNames(). I think this
should be part of the name class. In any case, something that
compares one name to another with DNSSEC ordering should probably
not be in rdata.cc.

I don't mind moving it to the name module. I actually thought about
that, but decided to place it in rdata. I (perhaps implicitly)
documented why I did so: it's actually almost a private helper
function, and the only expected usage is to implement a compare method
of an rdata derived class (and as documented BIND9 proved that). IMO
such a specialized interface is better to be hidden from a more
visible place to avoid making more fundamental base class (in this
case name) a monolithic kitchen sink.

Having said that, I also recognize this is probably a matter of
opinion. If you still disagree with the rationale, and especially if
someone else wants to have it in the name module also, I'm happy to
move it there.
(but I guess this is also a post-year1 thing)

comment:6 Changed 9 years ago by jinmei

  • Owner set to mgraff

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

  • Owner changed from mgraff to jinmei

Replying to jinmei:

I'm responding to the review comments, but overall none of the points
is a blocking issue, it seems. So, unless I'm told otherwise, I'll
merge this branch to trunk soon, and create separate tickets for
remaining open issues (if any).

Please do. :)

IMO the top copyright was redundant and actually inappropriate
(because it's not really associated with the copyrighted content).
So, if you're simply suggesting removing the head copyright from the
script, I'm happy with that.

That's what I'd do. Put the copyright in the input file(s) and not in the generated files(s) through the script. Makes it easy to maintain the right date too.

Another point is that this python script auto-generates common member
functions for derived RDATA class, such as this:

BEGIN_COMMON_MEMBERS

Ahh, that part I missed.

Perhaps a README in the rdata directory talking about what to do to make a new rdata type live, and specifically not to remove or alter those comments, as they are magic.

Ignore my point (2), I like the script now. :)

About (3), it is a matter of style, but I also think it's a matter of API cleanup. I don't expect to see a compare(Name, Name) function in rdata, but rather in name. If it becomes isc::dns::Name::compare() or isc::dns::name_compare() I don't really care much. Shane may argue for the latter as it does not TECHNICALLY need access to name internals, but I would make it a class (and instance) method:

Name::compare(n1, n2)

n1.compare(n2)

The latter could be implemented in terms of the former of course.

If this occurs before or after y1 I don't really have an opinion on. Part of me wants to see it move now, but it's not blocking, so it can clearly wait.

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

  • Resolution set to fixed
  • Status changed from assigned to closed

Replying to mgraff:

Please do. :)

Okay, thanks. Done.

IMO the top copyright was redundant and actually inappropriate
(because it's not really associated with the copyrighted content).
So, if you're simply suggesting removing the head copyright from the
script, I'm happy with that.

That's what I'd do. Put the copyright in the input file(s) and not
in the generated files(s) through the script. Makes it easy to
maintain the right date too.

I've updated the script so that it won't copy the common COPYING to
auto-generated files. It still copies copyright notices from the
original sources. in particular rdataclass.{h,cc} have many copied
notices. would you also like the script to remove it?

it's easy to modify the script to something like
this part is generated from rdata/generic/rrsig_46.h
for example, but since these source-of-source files won't be installed
(or will they?), I'm afraid such a reference can be a dangling link.

Another point is that this python script auto-generates common member
functions for derived RDATA class, such as this:

BEGIN_COMMON_MEMBERS

Ahh, that part I missed.

Perhaps a README in the rdata directory talking about what to do to
make a new rdata type live, and specifically not to remove or
alter those comments, as they are magic.

Ignore my point (2), I like the script now. :)

okay, good to know that. but I have to admit this feature can be
separated from auto-generating well-known RRtype/class parameters, so
you may still want to request limiting the task of the auto generation
script. if so, please file a separate ticket.

If this occurs before or after y1 I don't really have an opinion on.
Part of me wants to see it move now, but it's not blocking, so it
can clearly wait.

Okay, let's treat it as a post year1 fodder. I've created a separate
ticket for that. (#56)

I'm going to close this ticket.

comment:9 in reply to: ↑ 5 Changed 9 years ago by each

Also, opinion from Evan might be helpful. He has added

some new RR types using this scheme, and it's nice to know how useful
or not-so-useful he felt it was.

I heard my name!

I found the script-based generation of Rdata types to be quite helpful and straightforward, with one exception: We need RRtypes ANY, IXFR and AXFR, and RRClass ANY, and these are not going to have concrete implementations in the rdata directory, so they cannot be generated by the script. For the each-ds branch, I simply hard-coded them into rrtype-placeholder.h and rrclass-placeholder.h, with the expectation that when I get around to it, I'm going to figure out how to use the rrparamregistry to do this correctly, and where to do it. (In the meantime, while you can specify RRType::ANY() in the code, its toText() function reports it as "TYPE255"; I understand the rrparamregistry will fix this.)

I don't mind moving it to the name module. I actually thought about
that, but decided to place it in rdata. I (perhaps implicitly)
documented why I did so: it's actually almost a private helper
function, and the only expected usage is to implement a compare method
of an rdata derived class (and as documented BIND9 proved that). IMO
such a specialized interface is better to be hidden from a more
visible place to avoid making more fundamental base class (in this
case name) a monolithic kitchen sink.

On this point I disagree. Some uses of Name are obvious and universal, and the ability to compare two of them and sort on the basis of the comparison is such a use. I'll open a different ticket to discuss it though.

Note: See TracTickets for help on using tickets.