Skip to content
Snippets Groups Projects

RDATA lowercase

Merged Ghost User requested to merge lowercase into master

Some more tests would be handy, will add next week.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Ghost User
    Ghost User @ghost started a thread on the diff
  • 174 174 */
    175 175 uint32_t knot_rrset_ttl(const knot_rrset_t *rrset);
    176 176
    177 /*!
    178 * \brief Convert one RR into canonical format.
    179 *
    180 * Owner is always converted to lowercase. RDATA domain names are converted only
    181 * for types listed in RFC 4034, Section 6.2, except for NSEC (updated by
    182 * RFC 6840, Section 5.1) and A6 (not supported).
    183 *
    184 * \note If RRSet with more RRs is given to this function, only the first RR
    185 * will be converted.
    • Author Contributor

      Why? This will likely break something in the future.

    • Author Contributor

      Discussed with @jvcelak. Function for converting whole RRSet would have too much overhead - it would have to order the RRSet as well. (If a whole RRSet is not converted to lowercase, it will likely be ordered wrongly.) This is completely useless now, everywhere, where it is used, the server works only with one RR. There is this note in the doxygen, so if in any time we need to convert whole RRSet to canonical format, we may add a new function or modify this one if it proves suitable.

    • I do not recall this being discussed. Let's talk about this tomorrow.

    • Author Contributor

      OK, maybe it was someone else, never mind.

    • Author Contributor

      One more reason for leaving it here in this file: the function converts also the owner domain name to lowercase. That is another thing that would have to (unnecessarily) be done on the caller functions (=> redundant code).

      Also: the function for getting TTL also takes it only from the first RR (as there is no other way to do it), so I don't think it's a problem that this function works only with one RR. We have no structure for representing one (whole) RR, deal with it.

    • This is hopefully the last issue in this MR.

      @jkadlec Do you have an idea, how to resolve this problem?

  • 803 803 while(sp != lstack) { /* consume stack */
    804 804 l = *--sp; /* fetch rightmost label */
    805 805 memcpy(dst, l+1, *l); /* write label */
    806 for (int i = 0; i < *l; ++i) { /* convert to lowercase */
    807 dst[i] = knot_tolower(dst[i]);
    808 }
    • This will do. :thumbsup:

      However, there are some interesting facts about performance of this construct:

      • The label will be iterated twice (which will be hopefully optimized by the compiler).
      • knot_tolower is probably slower than tolower (except that the second one is known to be broken in some implementations).
      • De-Asmed algorithm gives a nice performance: https://code.google.com/p/stringencoders/wiki/PerformanceAscii

      This will be performed for each lookup (possible multiple times per query). We should make sure, that it has insignificant impact .

  • 522 510 assert(dst && *dst);
    523 511 assert(dst_avail);
    524 512 assert(dname_cfg);
    525 UNUSED(flags);
    513 UNUSED(dname_type);
    526 514
    527 int compr_size = knot_dname_wire_check(*src, *src + *src_avail, dname_cfg->pkt_wire);
    515 int compr_size = knot_dname_wire_check(*src, *src + *src_avail,
  • 165 return KNOT_EINVAL;
    166 }
    167
    168 /* Convert owner for all RRSets. */
    169 int ret = knot_dname_to_lower(rrset->owner);
    170 if (ret != KNOT_EOK) {
    171 return ret;
    172 }
    173
    174 /* Convert DNAMEs in RDATA only for RFC4034 types. */
    175 if (knot_rrtype_should_be_lowercased(rrset->type)) {
    176 const rdata_descriptor_t *desc =
    177 knot_get_rdata_descriptor(rrset->type);
    178 if (desc->type_name == NULL) {
    179 desc = knot_get_obsolete_rdata_descriptor(rrset->type);
    180 }
  • I'm missing a test zone with uppercase characters in all owner names, signed by BIND dnssec-signzone and NSEC. I think it might reveal additional problems (in semantic checks).

    There are some glitches which need a fix, but overall everything seems reasonable to me. :-)

  • Author Contributor

    Why uppercase in all owner names is better than uppercase in only some of them (there should be some zone like that)? But I agree, that some more tests would be useful.

  • I just don't think we have covered all cases for next dname field in NSEC chain (for semantic checks).

  • Please register or sign in to reply
    Loading