BIND 10 #1754: small issues in LabelSequence

[BIND 10 Development]

#1754: small issues in LabelSequence
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  UnAssigned
  jinmei                             |                Status:  new
                       Type:         |             Milestone:  Next-Sprint-
  defect                             |  Proposed
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  libdns++                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:1 vorner]:

 > > - Also, strncasecmp() is not in the C++ standard library and it's less
 > >   portable.  And, while it happens to be okay today, I'm afraid it's a
 > >   bit fragile to assume the label length field can be compared as a
 > >   case insensitive value.
 >
 > Hmm, interesting point. I think we could create two names that would not
 be equal because of different lengths of the labels (but the whole length
 being equal), however this would compare them as equal.

 As long as the "label length" field is always <= 63, this cannot
 happen (except for a compression pointer, which should be excluded in
 this context), because it's smaller than 'A'.  But if and when
 the IETF introduces another extension like now-deprecated bit-string
 labels, we'll encounter a trouble.  Of course, we'll then have to
 modify a lot of other parts of the code, but the use of strncasecmp()
 would make the dependency implicit and difficult to identify.4

 We already have a home-brew version of comparison in the Name class:
 Name::equals().  I think it's better to share the main logic of this
 code for both Name and LabelSequence somehow.  strncasecmp() would
 need to do compare each character one by one anyway, so performance
 wise it won't be much different (and actually Name::equals() could be
 faster because it has shortcuts like comparing the number labels).

 > > - Maybe pedantic, but I'm afraid using char* is not technically
 > >   correct because size of char may not necessarily be 8-bit (and with
 > >   this class we'd use the pair of char* + its "size" as a 8-bit
 > >   wire-format sequence.
 > >
 > > (As a hindsight using std::string (which is basic_string<char>) as the
 > > internal representation of Name was also a bad choice, and I've been
 > > wanting to fix it but missing the chance.  Maybe we should clarify
 > > that, e.g., by changing to basic_string<uint8_t> at this opportunity).
 >
 > I'd like to point out that the standard guarantees that sizeof(char)
 > == 1.

 Right, but it doesn't matter in this context...

 > That is, it doesn't have to be 8 bit, because one byte
 > could be 7 or 9 or something. But in that case, you won't have

 ...which is the point, and...

 > uint8_t anyway (the existence of fixed-width types is optional),
 > so we wouldn't work anyway.

 Hmm, that seems to be a valid argument.

 > The only thing I'd worry is that
 > char might be signed, in which case using it as the length of
 > sequence might be problematic.

 I didn't explicitly mention it, but that was actually another
 concern...as far as I understand the code, it doesn't actually cause a
 problem at least in the Name class, but it should be safer to be
 explicitly unsigned.  This would make strncasecmp() unusable (unless
 we convert the pointers with reinterpret_cast), but as discussed above
 if we don't rely on it for the equality check this is not a problem.

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