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:  jelte
                Type:  task          |                       Status:
            Priority:  medium        |  reviewing
           Component:  libdns++      |                    Milestone:
            Keywords:                |  Sprint-20130122
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DNS           |                 CVSS Scoring:
Estimated Difficulty:  3             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jelte


Comment:

 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).

 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`).

 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).

 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).

 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):
 {{{#!diff
      result += "\"";
 -    result += cpu_;
 +    result += detail::charStringToString(impl_->cpu);
      result += "\" \"";
 -    result += os_;
 +    result += detail::charStringToString(impl_->os);
      result += "\"";
 }}}

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