Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#1113 closed enhancement (complete)

RR type implementation: MINFO

Reported by: shane Owned by: zzchen_pku
Priority: low Milestone: Sprint-20110816
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Implement the MINFO type, from RFC 1035.

See ticket #809 for more discussion.

Subtickets

Change History (15)

comment:1 Changed 9 years ago by stephen

  • Milestone changed from Next-Sprint-Proposed to Sprint-20110816

comment:2 Changed 9 years ago by zzchen_pku

  • Owner changed from UnAssigned to zzchen_pku
  • Status changed from new to assigned

comment:3 Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to UnAssigned
  • Status changed from assigned to reviewing

trac1113 is ready for review.

Proposed ChangeLog? entry:

278.    [func]      jerry
    libdns++: Implement the MINFO rrtype according to RFC1035.
    (Trac #1113, git TBD)

comment:4 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:5 follow-up: Changed 9 years ago by jinmei

First off, I've fixed a couple of trivial things and pushed the
changes directly.

implementation

  • I suggest using \exception for exceptions throughout the implementation. From a quick look one of the constructors and toWire() will have to be updated.
  • this is not correct:
    +    /// We use the default copy constructor and assignment operator.
    
    Because the copy constructor is actually defined explicitly. Even if it's true the position of the documentation doesn't seem to be correct (did you check the doxygen output?). As for the assignment operator, you'll need to define it (if you need it) because by default it's inherited from the base class, which is private. Further, these are not tested in the unittests, so the discrepancy isn't properly caught. If this is because this branch wasn't developed test driven (I'm saying this because this type of uncovered test cannot happen in theory in TDD) please revisit the way of developing things.
  • The "from parameter" constructor isn't tested. Again, this type of thing shouldn't happen if it was developed test-driven. BTW, I'm not sure if we want to have this constructor at least at the moment.
  • IMO, we don't need '%' in the documentation of for the "from parameter" constructor:
        /// The parameters are a straightforward mapping of %MINFO RDATA
    
  • toWire(): is this correct?
    /// As specified in RFC1035, the rmailbox and emailbox fields (domain names)
    /// will be compressed.
    
    where in RFC1035 is it specified? (I have no doubt these should be compressed though).

unittest

  • now that we've merged #904, I strongly suggest extending gen_wiredata (in the latest master it's in lib/util/python) to support MINFO and using it for generating wire-format test data. hardcoding all hex like:
    const uint8_t compressed_wiredata_minfo[] = {
        0x04, 0x72, 0x6f, 0x6f, 0x74, 0x07, 0x65, 0x78, 0x61, 0x6d, 0x70,
        ...
    
    is difficult to create or maintain and is not human readable.
  • not a big deal, but there's inconsistency between rmailbox vs rmailbx (with or without 'o') in comment lines and test strings. same for emailbox.
  • I'd avoid using namespace level statically-initialized objects:
    string minfo_txt("root.example.com. emailbx.example.com.");
    string minfo_txt2("rmailbx.example.com. emailbx.example.com.");
    string too_long_label("012345678901234567890123456789"
        "0123456789012345678901234567890123");
    ...
    const generic::MINFO rdata_minfo(minfo_txt);
    const generic::MINFO rdata_minfo2(minfo_txt2);
    
    due to the possibility of causing initialization fiasco.
  • "from text" test: what if it has three fields?
  • "from text" test: the case where the rmailbox is bogus should also be tested.
  • I'd prefer RRType::MINFO over RRType("MINFO") as the former should be faster. Same for RRClass. (for the RRType we may want to use both so that we can implicitly test them).
  • In createFromWire test, it's not obvious why we use test data file named "cname" for testing MINFO:
        EXPECT_THROW(rdataFactoryFromFile(RRType("MINFO"), RRClass("IN"),
                                          "rdata_cname_fromWire", 48),
    
    we need a comment about why it's here.

other
As we talked at the biweekly call, please consider picking up review
before proceeding to another RDATA task (you seem to have picked up
#1114 while, e.g, #1067 was in the review queue at that time)

comment:6 Changed 9 years ago by jinmei

  • Owner changed from jinmei to zzchen_pku

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

  • Owner changed from zzchen_pku to jinmei

Replying to jinmei:

First off, I've fixed a couple of trivial things and pushed the
changes directly.

Thanks.

implementation

  • I suggest using \exception for exceptions throughout the implementation. From a quick look one of the constructors and toWire() will have to be updated.

Done.

  • this is not correct:
    +    /// We use the default copy constructor and assignment operator.
    
    Because the copy constructor is actually defined explicitly. Even if it's true the position of the documentation doesn't seem to be correct (did you check the doxygen output?). As for the assignment operator, you'll need to define it (if you need it) because by default it's inherited from the base class, which is private. Further, these are not tested in the unittests, so the discrepancy isn't properly caught. If this is because this branch wasn't developed test driven (I'm saying this because this type of uncovered test cannot happen in theory in TDD) please revisit the way of developing things.
  • The "from parameter" constructor isn't tested. Again, this type of thing shouldn't happen if it was developed test-driven. BTW, I'm not sure if we want to have this constructor at least at the moment.

Removed the constructor , will add it if we need it later.

  • IMO, we don't need '%' in the documentation of for the "from parameter" constructor:
        /// The parameters are a straightforward mapping of %MINFO RDATA
    
  • toWire(): is this correct?
    /// As specified in RFC1035, the rmailbox and emailbox fields (domain names)
    /// will be compressed.
    
    where in RFC1035 is it specified? (I have no doubt these should be compressed though).

Updated comments.

unittest

  • now that we've merged #904, I strongly suggest extending gen_wiredata (in the latest master it's in lib/util/python) to support MINFO and using it for generating wire-format test data. hardcoding all hex like:
    const uint8_t compressed_wiredata_minfo[] = {
        0x04, 0x72, 0x6f, 0x6f, 0x74, 0x07, 0x65, 0x78, 0x61, 0x6d, 0x70,
        ...
    
    is difficult to create or maintain and is not human readable.

Done, added MINFO class and generated testdata file.

  • not a big deal, but there's inconsistency between rmailbox vs rmailbx (with or without 'o') in comment lines and test strings. same for emailbox.

Okay:) updated.

  • I'd avoid using namespace level statically-initialized objects:
    string minfo_txt("root.example.com. emailbx.example.com.");
    string minfo_txt2("rmailbx.example.com. emailbx.example.com.");
    string too_long_label("012345678901234567890123456789"
        "0123456789012345678901234567890123");
    ...
    const generic::MINFO rdata_minfo(minfo_txt);
    const generic::MINFO rdata_minfo2(minfo_txt2);
    
    due to the possibility of causing initialization fiasco.

Updated.

  • "from text" test: what if it has three fields?
  • "from text" test: the case where the rmailbox is bogus should also be tested.
  • I'd prefer RRType::MINFO over RRType("MINFO") as the former should be faster. Same for RRClass. (for the RRType we may want to use both so that we can implicitly test them).

Done, added missed cases.

  • In createFromWire test, it's not obvious why we use test data file named "cname" for testing MINFO:
        EXPECT_THROW(rdataFactoryFromFile(RRType("MINFO"), RRClass("IN"),
                                          "rdata_cname_fromWire", 48),
    
    we need a comment about why it's here.

Updated.

other
As we talked at the biweekly call, please consider picking up review
before proceeding to another RDATA task (you seem to have picked up
#1114 while, e.g, #1067 was in the review queue at that time)

Okay, I'll consider it next time.

Hopefully I haven't missed anything, thanks for your review.

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

Replying to zzchen_pku:

  • this is not correct:
    +    /// We use the default copy constructor and assignment operator.
    

    Because the copy constructor is actually defined explicitly. Even
    if it's true the position of the documentation doesn't seem to be
    correct (did you check the doxygen output?). [...]
    Further, these are not tested in the unittests, so the discrepancy
    isn't properly caught.

The copy constructor still isn't tested. Also, when we want to define
the copy constructor we should normally also define the assignment
operator. Why don't you omit the latter?

unittest

  • now that we've merged #904, I strongly suggest extending gen_wiredata (in the latest master it's in lib/util/python) to support MINFO and using it for generating wire-format test data. hardcoding all hex like:

Done, added MINFO class and generated testdata file.

The MINFO class in gen_wiredata looks good, but there are other things
to do:

  • .wire shouldn't be in EXTRA_DIST (or in the repo). They are auto generated.
  • some test data are still hardcoded. Although name compression isn't very easy to configure in gen-wiredata (because you need to calculate the offset), it should still be much more intuitive than hardcoding everything.
  • not a big deal, but there's inconsistency between rmailbox vs rmailbx (with or without 'o') in comment lines and test strings. same for emailbox.

Okay:) updated.

There are still some 'bx'.

  • I'd avoid using namespace level statically-initialized objects:

Updated.

There's still some:

const generic::MINFO rdata_minfo((string(minfo_txt)));

for safety, it should go to the Rdata_MINFO_Test class. Also, why did
you remove rdata_minfo2?

  • In createFromWire test, it's not obvious why we use test data file named "cname" for testing MINFO:
        EXPECT_THROW(rdataFactoryFromFile(RRType("MINFO"), RRClass("IN"),
                                          "rdata_cname_fromWire", 48),
    
    we need a comment about why it's here.

I'd use gen_wiredata for other cases, and for the last case reuse the
cname test data with a comment explaining why.

Other things in the tests:

  • using both RRType::MINFO() and RRType("MINFO") is good, but please leave a comment about why you explicitly use the latter form. Without any explanation the mixture would look just awkward.
  • in toWireBuffer, why did you remove one of the two tests?
  • in toWireBuffer: rdata_minfo_toWireUncompressed.wire can be generated without the rdlen field by specifying a negative value for 'rdlen' in the spec file, then (I think) the test code will be much simpler.
  • same for toWireRenderer. Also, this should use gen_wiredata.

other
As we talked at the biweekly call, please consider picking up review
before proceeding to another RDATA task (you seem to have picked up
#1114 while, e.g, #1067 was in the review queue at that time)

Okay, I'll consider it next time.

If you find you cannot take on open review requests for some reason,
it may be okay to move to another development task. Sometimes it may
be because the code to be reviewed is too immature or too big, in
which case we should rather push it back to the author. But even in
such a case, please don't silently go to another development task, but
do leave some comments in the ticket about the request ticket noting
why you think you cannot work on that and also raise that at the daily
call.

comment:9 Changed 9 years ago by jinmei

  • Owner changed from jinmei to zzchen_pku

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

  • Owner changed from zzchen_pku to jinmei

Replying to jinmei:

The copy constructor still isn't tested. Also, when we want to define
the copy constructor we should normally also define the assignment
operator. Why don't you omit the latter?

Added assignment operator.
As fast as I know, the default copy constructor and assginment operator should be enough for MINFO, but it seems gen-rdatacode.py.in requires all rdata types to define their own copy constructor ,don't know the reason.

The MINFO class in gen_wiredata looks good, but there are other things
to do:

  • .wire shouldn't be in EXTRA_DIST (or in the repo). They are auto generated.
  • some test data are still hardcoded. Although name compression isn't very easy to configure in gen-wiredata (because you need to calculate the offset), it should still be much more intuitive than hardcoding everything.

Updated, avoid using hardcoding test data.

There are still some 'bx'.

My miss, Updated.

There's still some:

const generic::MINFO rdata_minfo((string(minfo_txt)));

for safety, it should go to the Rdata_MINFO_Test class. Also, why did
you remove rdata_minfo2?

Updated, also added rdata_minfo2.

  • In createFromWire test, it's not obvious why we use test data file named "cname" for testing MINFO:
        EXPECT_THROW(rdataFactoryFromFile(RRType("MINFO"), RRClass("IN"),
                                          "rdata_cname_fromWire", 48),
    
    we need a comment about why it's here.

I'd use gen_wiredata for other cases, and for the last case reuse the
cname test data with a comment explaining why.

Avoid using cname test data.

Other things in the tests:

  • using both RRType::MINFO() and RRType("MINFO") is good, but please leave a comment about why you explicitly use the latter form. Without any explanation the mixture would look just awkward.

Done.

  • in toWireBuffer, why did you remove one of the two tests?

Because I thought there is no difference with regard to test coverage. Now I agree a double-test with different data is necessary, added again.

  • in toWireBuffer: rdata_minfo_toWireUncompressed.wire can be generated without the rdlen field by specifying a negative value for 'rdlen' in the spec file, then (I think) the test code will be much simpler.
  • same for toWireRenderer. Also, this should use gen_wiredata.

Updated, thanks for your suggestion.

If you find you cannot take on open review requests for some reason,
it may be okay to move to another development task. Sometimes it may
be because the code to be reviewed is too immature or too big, in
which case we should rather push it back to the author. But even in
such a case, please don't silently go to another development task, but
do leave some comments in the ticket about the request ticket noting
why you think you cannot work on that and also raise that at the daily
call.

Okay, good suggestion:)

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

Replying to zzchen_pku:

The latest version basically looks fine and ready for merge.

I have some minor additional comments though. I don't think we need
another cycle of review for this, so please update the code
accordingly and then merge it.

  • Unfortunately this comment is not 100% correct:
    +    /// This method never throws an exception.
    +    MINFO& operator=(const MINFO& source);
    
    because coping a name may result in std::bad_alloc. I personally even note this obvious, but others seem to rather ignore this level of exceptions in documentation. Either is okay for me, but at least "never throws" is not correct and should be fixed.
  • I'd remove the comment line because it now has MINFO specific code in it (although technically it may not be considered "specialization"):
    +class Rdata_MINFO_Test : public RdataTest {
    +    // there's nothing to specialize
    
  • The assignment test: this is fine, although it's probably too much because the implementation is a trivial copy (rather than using pimpl which requires a bit more care). But I'm okay with keeping the current test case.
  • rdata_minfo_toWireUncompressed2.spec: the comment doesn't seem to be correct (it customizes the rmailbox). Ideally it describes the point of the test correctly. Or at least the incorrect part of the comment should be removed.

The copy constructor still isn't tested. Also, when we want to define
the copy constructor we should normally also define the assignment
operator. Why don't you omit the latter?

Added assignment operator.
As fast as I know, the default copy constructor and assginment operator should be enough for MINFO, but it seems gen-rdatacode.py.in requires all rdata types to define their own copy constructor ,don't know the reason.

This is not a requirement of gen-rdatacode. It's a consequence of the
fact that the base class Rdata declares the copy constructor and
operator= as private methods (to make it non copyable, which is for
avoiding unexpected slicing). Even if they are private in terms of
accessibility, the compiler finds it when it tries to resolve the
definition in a derived class and then throw a compilation error due
to the accessibility restriction. So, admittedly it's a bit
inconvenient, we always have to define these methods in derived
classes explicitly.

comment:12 Changed 9 years ago by jinmei

  • Owner changed from jinmei to zzchen_pku

comment:13 in reply to: ↑ 11 Changed 9 years ago by zzchen_pku

Replying to jinmei:

The latest version basically looks fine and ready for merge.

I have some minor additional comments though. I don't think we need
another cycle of review for this, so please update the code
accordingly and then merge it.

  • Unfortunately this comment is not 100% correct:
    +    /// This method never throws an exception.
    +    MINFO& operator=(const MINFO& source);
    
    because coping a name may result in std::bad_alloc. I personally even note this obvious, but others seem to rather ignore this level of exceptions in documentation. Either is okay for me, but at least "never throws" is not correct and should be fixed.
  • I'd remove the comment line because it now has MINFO specific code in it (although technically it may not be considered "specialization"):
    +class Rdata_MINFO_Test : public RdataTest {
    +    // there's nothing to specialize
    
  • The assignment test: this is fine, although it's probably too much because the implementation is a trivial copy (rather than using pimpl which requires a bit more care). But I'm okay with keeping the current test case.
  • rdata_minfo_toWireUncompressed2.spec: the comment doesn't seem to be correct (it customizes the rmailbox). Ideally it describes the point of the test correctly. Or at least the incorrect part of the comment should be removed.

Fixed.

The copy constructor still isn't tested. Also, when we want to define
the copy constructor we should normally also define the assignment
operator. Why don't you omit the latter?

Added assignment operator.
As fast as I know, the default copy constructor and assginment operator should be enough for MINFO, but it seems gen-rdatacode.py.in requires all rdata types to define their own copy constructor ,don't know the reason.

This is not a requirement of gen-rdatacode. It's a consequence of the
fact that the base class Rdata declares the copy constructor and
operator= as private methods (to make it non copyable, which is for
avoiding unexpected slicing). Even if they are private in terms of
accessibility, the compiler finds it when it tries to resolve the
definition in a derived class and then throw a compilation error due
to the accessibility restriction. So, admittedly it's a bit
inconvenient, we always have to define these methods in derived
classes explicitly.

Yeah, got it, thanks.

comment:14 Changed 9 years ago by zzchen_pku

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged, closing it.

comment:15 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 4
Note: See TracTickets for help on using tickets.