Opened 7 years ago

Closed 7 years ago

#2497 closed task (fixed)

introduce wrapper version of "from lexer" rdata factory

Reported by: jinmei Owned by: muks
Priority: medium Milestone: Sprint-20121204
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: loadzone-ng
Estimated Difficulty: 0 Add Hours to Ticket: 2
Total Hours: 0 Internal?: no

Description

This task is to introduce an intermediate wrapper so that we can use
the "from master lexer" Rdata factory transparently, until we complete
supporting all currently defined Rdata's. The basic idea is to
forward the work to the existing "from string" factory by default, and
use the dedicated versions as we implement those RR types of Rdata.

Specifically:

  • introduce AbstractRdataFactory::create(lexer):
    class AbstractRdataFactory {
        virtual RdataPtr create(MasterLexer& lexer, const Name* origin,
                                MasterLoader::Option options,
                                MasterLoader::Callbacks& callbacks) const;
    };
    
    and define the default version of it (unlike other create() methods we need the default version for the moment). It will use the lexer to get all token until EOL or EOF as strings, concatenate them separated by a space into a single std::string object, and then call this version of create():
        virtual RdataPtr create(const std::string& rdata_str) const = 0;
    
  • update rrparamregistry-placeholder.cc as follows:
    • rename current RdataFactory to OldRdataFactory. for the "from lexer" create() method, it will use the default version.
    • introduce a new RdataFactory, which has the "from lexer" create method:
          virtual RdataPtr create(MasterLexer& lexer, const Name* origin,
                                  MasterLoader::Option options,
                                  MasterLoader::Callbacks& callbacks) const
          {
              return (RdataPtr(new T(dynamic_cast<const T&>(lexer, origin, options,
                                                            callbacks))));
          }
      
  • update gen-rdatacode.py:generate_rrparam() so that, except for hardcoded exceptions, use OldRdataFactory instead of RdataFactory. we update the exception list as we implement RDATAs

Subtickets

Attachments (1)

rrparamregistry.diff (4.7 KB) - added by jinmei 7 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 Changed 7 years ago by jinmei

  • Milestone changed from New Tasks to Sprint-20121204

pushing this to the current sprint as proposed at bind10-dev.

comment:2 Changed 7 years ago by muks

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

Picking

comment:3 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to UnAssigned

Returning. This needs #2377 and other bits (MasterLoader::Callbacks) to be implemented first.

comment:4 in reply to: ↑ 3 Changed 7 years ago by jinmei

Replying to muks:

Returning. This needs #2377 and other bits (MasterLoader::Callbacks) to be implemented first.

I don't think this requires #2377. Rather, #2377 (indirectly) depends on this ticket.
So, in terms of dependency control it's even better to start this one before #2377.

comment:5 Changed 7 years ago by muks

  • Owner changed from UnAssigned to muks

Picking to take another look.

comment:6 Changed 7 years ago by muks

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

Up for review. I don't think a ChangeLog update is necessary as it is internal refactoring.

comment:7 Changed 7 years ago by jinmei

Please confirm it works as expected for RR types we are currently
supporting, not only for IN/A. We'll rely on this wrapper version
in the next beta, so I think a comprehensive check is necessary.

comment:8 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to muks

comment:9 follow-up: Changed 7 years ago by muks

OldRdataFactory is not exported outside rrparamregistry.cc, so it's not possible to call the create() method on any templated classes in that file, except on mock objects derived from AbstractRdataFactory such as TestRdataFactory. Testing more than in::A() in TestRdataFactory did not appear to do more in this context (as this test just asserts that the string variant of create() is called).

Either all this, or I'm not following the question. I'll take another look and see if we can rewrite bits to help testing.

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

Replying to muks:

OldRdataFactory is not exported outside rrparamregistry.cc, so

it's not possible to call the create() method on any templated
classes in that file, except on mock objects derived from
AbstractRdataFactory such as TestRdataFactory.

Ah, okay, but can't you use RRParamRegistry::getRegistry().createRdata()?
...hmm, the ticket description doesn't include that part. The intent was
to extend RRParamRegistry::createRdata(), too.

I also suggest defining a trivial wrapper version of
rdata::createRdata() in rdata.{h,cc}:

RdataPtr
createRdata(const RRType& rrtype, const RRClass& rrclass,
            MasterLexer& lexer, const Name* origin,     
            MasterLoader::Options options,
            MasterLoaderCallbacks& callbacks)
{
    return (RRParamRegistry::getRegistry().createRdata(rrtype, rrclass,
                                                       lexer, origin,
                                                       options, callbacks));
}

(we need to do a lot more for the complete version of this function,
which is the subject of a different ticket).

Then when we complete this ticket we can start MasterLoader at least
for normal cases.

And I suggest moving the implementation of create() in .cc. That's a
matter of preference in some sense, but I don't like to define too
many non trivial things in .h unless it has clear advantage for that
(a typical reason is to make it inline for very trivial methods like a
simple setter/getter, which isn't the case here).

comment:11 Changed 7 years ago by jinmei

I've noticed a syntax error and fixed in the trac2382 branch
at commit cd9255d. Please cherry-pick it.

comment:12 follow-up: Changed 7 years ago by jinmei

And I pushed what I meant in
http://bind10.isc.org/ticket/2497#comment:10
to the trac2382 branch at commit 8398a6a.

Please cherry-pick and enhance it for complete tests.

comment:13 in reply to: ↑ 12 Changed 7 years ago by muks

Replying to jinmei:

And I pushed what I meant in
http://bind10.isc.org/ticket/2497#comment:10
to the trac2382 branch at commit 8398a6a.

Please cherry-pick and enhance it for complete tests.

An implementation is already in trac2497 for this. I just have to finish tests for all the RRtypes. Only some are in the branch currently.

comment:14 follow-up: Changed 7 years ago by jinmei

Another thing:

We need to handle END_OF_LINE too:

        if (token.getType() == MasterLexer::Token::END_OF_FILE) {
            break;
        }

(actually that's the more common case).

This also means we need some more tests that can cover that.

comment:15 in reply to: ↑ 14 Changed 7 years ago by muks

Replying to jinmei:

Another thing:

We need to handle END_OF_LINE too:

        if (token.getType() == MasterLexer::Token::END_OF_FILE) {
            break;
        }

(actually that's the more common case).

This also means we need some more tests that can cover that.

This is now done. Tests for various RRtypes are done except:

ds_like
nsec3
nsec3param
nsec3param_like
nsecbitmap
tsig
txt_like

I'll finish them and put this bug to review.

comment:16 Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Up for review now. Note that API docs are lacking, but I want to get the code in the branch reviewed first. (Most of the method args are unused currently anyway.)

comment:17 follow-up: Changed 7 years ago by jinmei

gen-rdatacode.py.in

  • new_rdatafactory_users must be able to work pairs of RR type and RR class, not only for types (e.g. A/IN and A/CH are different types).
  • Also, please add a comment on what should be added new_rdatafactory_users to with an example:
    new_rdatafactory_users = []
    
    No one except the original author can figure out what should be added to the list when a new type is supported unless they bother to read the code to understand how this list is used.

rrparamregistry-placeholder.cc

  • not really for this particular task, but the repeated patter of code to identify the factory now look redundant duplicate:
        RdataFactoryMap::const_iterator found =
            impl_->rdata_factories.find(RRTypeClass(rrtype, rrclass));
        if (found != impl_->rdata_factories.end()) {
            return (found->second->create(PARAMETERS));
        }
    
        GenericRdataFactoryMap::const_iterator genfound =
            impl_->genericrdata_factories.find(rrtype);
        if (genfound != impl_->genericrdata_factories.end()) {
            return (genfound->second->create(PARAMETERS));
        }
    
    I'm attaching a proposed diff to unify these.

rdata_hinfo_unittest.cc

  • I suspect this is not a good example of bad input:
    +    // Exceptions cause NULL to be returned.
    +    EXPECT_FALSE(test::createRdataUsingLexer(RRType::HINFO(), RRClass::IN(),
    +                                             "\"Pentium\"\"Linux\""));
    
    With the generic lexer it would become valid (and BIND 9 would accept it too)

rdata_txt_like_unittest.cc

  • I'd remove these cases:
        EXPECT_EQ(0, this->rdata_txt_like.compare(
            *test::createRdataUsingLexer(RRTYPE<TypeParam>(), RRClass::IN(),
                                         "Test String")));
        EXPECT_EQ(0, this->rdata_txt_like_empty.compare(
            *test::createRdataUsingLexer(RRTYPE<TypeParam>(), RRClass::IN(),
                                         "")));
    
    they will soon result in different results with the generic lexer.

Changed 7 years ago by jinmei

comment:18 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:19 Changed 7 years ago by jinmei

Regarding documentation, I've given a certain amount of it in #2382.
Please check it. I guess there's no need for adding anything specific
to this branch.

comment:20 in reply to: ↑ 17 ; follow-ups: Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Hi Jinmei

Replying to jinmei:

gen-rdatacode.py.in

  • new_rdatafactory_users must be able to work pairs of RR type and RR class, not only for types (e.g. A/IN and A/CH are different types).
  • Also, please add a comment on what should be added new_rdatafactory_users to with an example:
    new_rdatafactory_users = []
    
    No one except the original author can figure out what should be added to the list when a new type is supported unless they bother to read the code to understand how this list is used.
  • Comments were added
  • The list takes tuples of (rrtype, rrclass) now.

rrparamregistry-placeholder.cc

  • not really for this particular task, but the repeated patter of code to identify the factory now look redundant duplicate:
        RdataFactoryMap::const_iterator found =
            impl_->rdata_factories.find(RRTypeClass(rrtype, rrclass));
        if (found != impl_->rdata_factories.end()) {
            return (found->second->create(PARAMETERS));
        }
    
        GenericRdataFactoryMap::const_iterator genfound =
            impl_->genericrdata_factories.find(rrtype);
        if (genfound != impl_->genericrdata_factories.end()) {
            return (genfound->second->create(PARAMETERS));
        }
    
    I'm attaching a proposed diff to unify these.

Patch looks good. I've applied it.

rdata_hinfo_unittest.cc

  • I suspect this is not a good example of bad input:
    +    // Exceptions cause NULL to be returned.
    +    EXPECT_FALSE(test::createRdataUsingLexer(RRType::HINFO(), RRClass::IN(),
    +                                             "\"Pentium\"\"Linux\""));
    
    With the generic lexer it would become valid (and BIND 9 would accept it too)

I don't follow this. I can't think of another way to get an invalid HINFO without similar syntax.

rdata_txt_like_unittest.cc

  • I'd remove these cases:
        EXPECT_EQ(0, this->rdata_txt_like.compare(
            *test::createRdataUsingLexer(RRTYPE<TypeParam>(), RRClass::IN(),
                                         "Test String")));
        EXPECT_EQ(0, this->rdata_txt_like_empty.compare(
            *test::createRdataUsingLexer(RRTYPE<TypeParam>(), RRClass::IN(),
                                         "")));
    
    they will soon result in different results with the generic lexer.

These tests have been removed now.

Also there's a commit pushed to master branch (403af33, reviewed by vorner) which generates gen-rdatacode.py from the gen-rdatacode.py.in file.

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

Replying to muks:

  • I suspect this is not a good example of bad input:
    +    // Exceptions cause NULL to be returned.
    +    EXPECT_FALSE(test::createRdataUsingLexer(RRType::HINFO(), RRClass::IN(),
    +                                             "\"Pentium\"\"Linux\""));
    
    With the generic lexer it would become valid (and BIND 9 would accept it too)

I don't follow this. I can't think of another way to get an invalid HINFO without similar syntax.

This is a valid textual representation:

example.com. IN HINFO "Pentium""Linux"

Our std::string based parser incorrectly rejects it, but we shouldn't,
and the generic lexer version will correctly accept it.

But this is absolutely invalid:

example.com. IN HINFO "Pentium""Linux""garbage"

or for that matter

example.com. IN HINFO "Pentium" "Linux" "garbage"

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

Replying to muks:

new_rdatafactory_users = []

No one except the original author can figure out what should be
added to the list when a new type is supported unless they bother to
read the code to understand how this list is used.

  • Comments were added
  • The list takes tuples of (rrtype, rrclass) now.

I'm afraid this doesn't cover the pairs of generic types for class
"IN" like this:

        add("SOA", 6, "IN", 1, RdataFactoryPtr(new OldRdataFactory<generic::SOA>()));

It's probably due to poor/no documentation of the script, but we have
this trick as a heuristics optimization for class "IN":

                        if class_txt == 'generic':
                            typeandclass.append((type_txt, int(type_code),
                                                 (class_txt, 'in'), 1))

So, for example, if we have ('xxx', 'generic') in new_rdata_factory_users,
we need to handle the case of ('xxx', 'in'), too. One easy way to
just clarify it in the comment (after all this is temporary
workaround). Another is to handle it automatically in generate_rrparam().

  • I suspect this is not a good example of bad input:
    +    // Exceptions cause NULL to be returned.
    +    EXPECT_FALSE(test::createRdataUsingLexer(RRType::HINFO(), RRClass::IN(),
    +                                             "\"Pentium\"\"Linux\""));
    
    With the generic lexer it would become valid (and BIND 9 would accept it too)

I don't follow this. I can't think of another way to get an invalid HINFO without similar syntax.

See the previous comment.

comment:23 Changed 7 years ago by jinmei

  • Owner changed from jinmei to muks

comment:24 in reply to: ↑ 22 ; follow-up: Changed 7 years ago by muks

Replying to jinmei:

Replying to muks:

new_rdatafactory_users = []

No one except the original author can figure out what should be
added to the list when a new type is supported unless they bother to
read the code to understand how this list is used.

  • Comments were added
  • The list takes tuples of (rrtype, rrclass) now.

I'm afraid this doesn't cover the pairs of generic types for class
"IN" like this:

        add("SOA", 6, "IN", 1, RdataFactoryPtr(new OldRdataFactory<generic::SOA>()));

It's probably due to poor/no documentation of the script, but we have
this trick as a heuristics optimization for class "IN":

                        if class_txt == 'generic':
                            typeandclass.append((type_txt, int(type_code),
                                                 (class_txt, 'in'), 1))

So, for example, if we have ('xxx', 'generic') in new_rdata_factory_users,
we need to handle the case of ('xxx', 'in'), too. One easy way to
just clarify it in the comment (after all this is temporary
workaround). Another is to handle it automatically in generate_rrparam().

I've updated the code to handle this, and also verified it with a [('soa', 'generic')] sample list, which also applies it to the ('soa', 'in') case.

comment:25 in reply to: ↑ 21 Changed 7 years ago by muks

  • Owner changed from muks to jinmei

Hi Jinmei

Replying to jinmei:

Replying to muks:

  • I suspect this is not a good example of bad input:
    +    // Exceptions cause NULL to be returned.
    +    EXPECT_FALSE(test::createRdataUsingLexer(RRType::HINFO(), RRClass::IN(),
    +                                             "\"Pentium\"\"Linux\""));
    
    With the generic lexer it would become valid (and BIND 9 would accept it too)

I don't follow this. I can't think of another way to get an invalid HINFO without similar syntax.

This is a valid textual representation:

example.com. IN HINFO "Pentium""Linux"

Our std::string based parser incorrectly rejects it, but we shouldn't,
and the generic lexer version will correctly accept it.

But this is absolutely invalid:

example.com. IN HINFO "Pentium""Linux""garbage"

or for that matter

example.com. IN HINFO "Pentium" "Linux" "garbage"

The HINFO string parsing has been updated for these cases. Tests have also been updated/added.

comment:26 in reply to: ↑ 24 Changed 7 years ago by jinmei

Replying to muks:

So, for example, if we have ('xxx', 'generic') in new_rdata_factory_users,
we need to handle the case of ('xxx', 'in'), too. One easy way to
just clarify it in the comment (after all this is temporary
workaround). Another is to handle it automatically in generate_rrparam().

I've updated the code to handle this, and also verified it with a [('soa', 'generic')] sample list, which also applies it to the ('soa', 'in') case.

Change looks okay, but not very readable (and one line is too long).
I've committed a suggested fix to that. I also added one example
for the 'generic' class.

HINFO changes seem overkill (we'll soon discard them anyway), but
don't look incorrect so I'm okay with them.

So, if the suggested changes to the script are okay please merge.

comment:27 Changed 7 years ago by jinmei

  • Add Hours to Ticket changed from 0 to 2
  • Owner changed from jinmei to muks

comment:28 Changed 7 years ago by muks

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

Merged to master in commit d5f52afdc5419756f514f79ecd53f59090555c43:

* 203b4a0 [2497] some suggested editorial cleanups: long line, identation, better e.g.
* 20ff1f4 [2497] Apply generic rules to class IN too
* ec24cb1 [2497] Add a direct HINFO testcase for the string parser
* 1a2787f [2497] Fix HINFO parsing in the string parser
* 7237139 [2497] Return whether the string is quoted in characterstr::getNextCharacterString()
* 3e5f225 [2497] Remove a couple of txt tests
* 947a2d7 [2497] Move repeated code to a function
* 292db14 [2497] Make new_rdata_factory_users[] a list of tuples
* 8ab540a [2497] constify
* 45b4e93 [2497] Return NULL upon exception in rdata::createRdata()
* 9e98b47 [2497] Add tests for NSEC3PARAM-like rrtypes
* 43292d6 [2497] Add tests for NSEC3PARAM, DS-like and TXT-like rrtypes
* e3afa9c [2497] Add tests for NSEC3, NSEC and TSIG rrtypes
* c73280d [2497] Also check and stop at END_OF_LINE
* 1c4683d [2497] Add tests for AFSDB, DHCID and DNSKEY rrtypes
* 25f6513 [2497] Add tests for HINFO, MINFO and NAPTR rrtypes
* 5c8cd40 [2497] Add tests for OPT, PTR and RP rrtypes
* 3563065 [2497] Add tests for RRSIG, SOA and SRV rrtypes
* 41754f4 [2497] Refactor RRSIG rrtype test code
* 5e0e2e5 [2382] corrected trivial syntax error in #2497
* 2b8518d [2497] Add a comment for RRParamRegistryTest.createFromLexer
* 022d388 [2497] Add rdata::createRdata() and tests for some RRtypes
* aac7b17 [2497] Move create() definition to .cc file
* 6921ae3 [2497] Add missing testcase for AbstractRdataFactory::create() from lexer
* 331d377 [2497] Add a new RdataFactory class which includes the new create() impl
* e980e67 [2497] Delete trailing whitespace
* 0bfb997 [2497] Rename existing RdataFactory to OldRdataFactory
* 1b66148 [2497] Introduce a new AbstractRdataFactory::create() that takes a MasterLexer
* 1051eb4 [2497] Add placeholder MasterLoader class
* 79968a2 [2497] Fix #endif comment to match #if comment

I've verified that lettuce also passes after merge to master.

Resolving as fixed. Thank you for the reviews Jinmei.

Note: See TracTickets for help on using tickets.