BIND 10 #2498: support generic version of rdata::createRdata(text) in HINFO and NAPTR

[BIND 10 Development]

#2498: support generic version of rdata::createRdata(text) in HINFO and NAPTR
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  vorner
            Priority:  medium        |                       Status:
           Component:  libdns++      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130122
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  3             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => vorner


Comment:

 Replying to [comment:8 vorner]:
 > Hello
 >
 > I see some details about the branch.
 >
 > First, I don't see why the pimpl is needed. If it's because the
 `CharString` is not available in the header, neither is the pimpl class,
 so it could be forward-declared and used the same way as the pimpl class.
 Then there'd be one less indirection layer (we already seem to have enough
 to confuse casual reader).
 >

 There's 2 reasons I preferred pimpl over forward-declared charstrings; now
 there is only one malloc/free instead of one for ever charstring member.
 Also CharString is a typedef and that does not play well with forward
 declarations un

 Less indirection was the reason I only made the constructors 'pimpld' and
 left the outer methods to directly access the inner members. Both these
 things (and the one below) are also reasons I want CharString to be public
 (so we don't need any of this stuff, nor dynamic allocation)

 > Also, as the class now contains a pointer, it should either be made
 uncopyable or a copy constructor and operator be defined. The generated
 ones would lead to serious memory problems, like double-free bugs and
 accesses to invalid memory (this would be the case with the proposed use
 of forward-declared `CharString`).
 >

 They already had copy constructors, but I indeed forgot operator=, added.

 > And, would it be better to have it in some kind of `scoped_ptr` instead
 of manually deleting it? It would save us some code and it would feel
 safer (though in practice it probably is as safe as this code).
 >

 ack, doesn't matter much indeed in this case but it's good practice to use
 them, updated. (destructor is now empty but must still be explicitely
 defined to be able to use scoped_ptr in the header btw)

 > The `CharString` comparison function, could it be simplified? Is the
 zero length handling necessary? I think memcmp should work with 0 length
 too (and not touch any data at the provided addresses).
 >

 memcmp with length 0 isn't the problem, it's that the first byte contains
 the length (again) and must initially be skipped to check the data first,
 and using size()-1 instead of self[0] would still need the zero check.

 If it were a real class as opposed to a typedef we can probably make
 compare() simpler as we could guarantee that the size is always 1 or more
 (with a 'length' byte of 0). But as it is I can't at this moment think of
 an easier way to do it.

 > And this one looks wrong. AFAIK charStringToString already does
 escaping. It would therefore generate both quotes and escape sequences,
 which is duplicate (indeed, the previous version was wrong too):

 It's not escaping, it's requoting the quoted string, which has been
 stripped by the parser; it'll quote unquoted input, but that's just having
 more strict output than input :) Added a couple of tests to make sure it
 isn't doing it twice

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