Skip to content
Snippets Groups Projects

RRSet refactoring - remove RRSIGs, custom memory allocator

Merged Ghost User requested to merge rr_refactor into master
  • rrset->rrsigs removed + code removal where applicable
  • mem_ctx_t added to RRSet API, only used in pkt.c now

State of the review

Up to nsec_proofs.c

Merge request reports

Merged by avatar (Apr 18, 2025 9:30am UTC)

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
  • 71 72 return false;
    72 73 }
    73 74
    74 knot_rrset_t *a_rrset = a->rrset_tree[0];
    75 knot_rrset_t *b_rrset = b->rrset_tree[0];
    75 const knot_rrset_t *a_rrset = knot_node_rrset(a, KNOT_RRTYPE_NSEC3);
    76 const knot_rrset_t *b_rrset = knot_node_rrset(b, KNOT_RRTYPE_NSEC3);
  • Ghost User
    Ghost User @ghost started a thread on the diff
  • 418
    419 assert(to_rrset->rrsigs == NULL);
    420
    421 to_rrset->rrsigs = from_rrset->rrsigs;
    422 knot_rrset_t *from_sig = knot_node_get_rrset(from, KNOT_RRTYPE_RRSIG);
    423 if (from_sig == NULL) {
    424 return KNOT_EOK;
    425 }
    426 return knot_node_add_rrset(to, from_sig);
    422 427 }
    423 428
    424 429 /*!
    425 430 * \brief Reuse signatatures by shallow copying them from one tree to another.
    426 431 */
    427 static void copy_signatures(const knot_zone_tree_t *from, knot_zone_tree_t *to)
    432 static int copy_signatures(const knot_zone_tree_t *from, knot_zone_tree_t *to)
    • Author Contributor

      This confuses me, in C/libc most of the copy operations are (dst, src) (memcpy, strcpy, ...) but we have it the other way round (in libknot in general). Just a thought.

    • Author Contributor

      I've never though of this, it's probably not even consistent.

  • Ghost User
    Ghost User @ghost started a thread on the diff
  • 741 * Remove possible NSEC from the node. (Do not allow both NSEC
    742 * and NSEC3 in the zone at once.)
    743 */
    744 result = knot_nsec_changeset_remove(knot_node_rrset(node,
    745 KNOT_RRTYPE_NSEC),
    746 knot_node_rrset(node,
    747 KNOT_RRTYPE_RRSIG),
    748 chgset);
    749 if (result != KNOT_EOK) {
    750 break;
    751 }
    752 if (knot_node_rrset(node, KNOT_RRTYPE_NSEC)) {
    753 knot_node_set_replaced_nsec(node);
    754 }
    755
    756 if (knot_node_is_non_auth(node)) {
    • Author Contributor

      Why is this check moved? Can non-auth node have NSEC? (I thought this applied only for NSEC3) Also a comment would be nice, what does the code on line 752 do for example.

    • Author Contributor

      Yes it can, this is a post-DDNS code, and a glue node might have been changed into an authoritative node, or vice-versa. I'll add the comment.

    • Author Contributor

      Yeah, but that would make it authoritative. Then if I understand, this happens before adjusting so the flag is wrong. That makes a case that maybe it would be a good idea not to cringe on the flags and see if the node is authoritative by chasing node parents to the closest delegation point (I don't see where this could slow down the answering).

    • Author Contributor

      It happens after adjusting (partial, NSEC3 hashes are not calculated), so when DDNS changed a previously-authoritative node into glue (I know, but it can happen), its now-obsolete NSEC would stay in the zone - but I'm not totally sure what labels the node as non-auth. I agree 100% with the flag dependency part.

    • Author Contributor

      Node is non-auth if it is below delegation point, this function could probably check in realtime whether this is true or not (shouldn't be expensive).

  • Ghost User
    Ghost User @ghost started a thread on the diff
  • 436 return add_missing_rrsigs(covered, rrsigs, zone_keys, policy,
    417 437 changeset);
    418 438 }
    419 439
    440 static int remove_standalone_rrsigs(const knot_node_t *node,
    441 const knot_rrset_t *rrsigs,
    442 knot_changeset_t *changeset)
    443 {
    444 if (rrsigs == NULL) {
    445 return KNOT_EOK;
    446 }
    447
    448 for (size_t i = 0; i < rrsigs->rdata_count; ++i) {
    449 uint16_t type_covered = knot_rdata_rrsig_type_covered(rrsigs, i);
    450 if (!knot_node_rrset(node, type_covered)) {
    451 knot_rrset_t *to_remove = knot_rrset_new_from(rrsigs, NULL);
    • Author Contributor

      I'm not sure here - why aren't rrsigs removed along with what the stuff they're covering? Is the rrsigs[i] removed from the rrset with the changeset application later on?

    • Author Contributor

      1.) You cannot do that, since the removal might have occurred as a change in a zonefile. You no longer have any information about previous state of node. This is about removal of RRSIGs that no longer have their RRSet. 2.) Yes, since it's a normal RR type now.

    • Author Contributor

      Fair enough then.

  • Ghost User
    Ghost User @ghost started a thread on the diff
  • 473 519 }
    474 520 }
    475 521
    476 return result;
    522 if (result != KNOT_EOK) {
  • Ghost User
    Ghost User @ghost started a thread on the diff
  • 177 182 return KNOT_ESPACE;
    178 183 }
    179 184 for (unsigned i = 0; i < knot_node_rrset_count(qdata->node); ++i) {
    180 ret = put_rr(pkt, rrsets[i], compr_hint, 0, qdata);
    185 ret = put_rr(pkt, rrsets[i], NULL, compr_hint, 0, qdata);
    181 186 if (ret != KNOT_EOK) {
    182 187 break;
    183 188 }
    184 189 }
    185 190 break;
    186 case KNOT_RRTYPE_RRSIG: /* Append all RRSIGs. */
  • Ghost User
    Ghost User @ghost started a thread on the diff
  • 650 648
    651 649 /*----------------------------------------------------------------------------*/
    652 650
    651 static int insert_rr(knot_zone_contents_t *z, knot_rrset_t *rr, knot_node_t **n,
  • Ghost User
    Ghost User @ghost started a thread on the diff
  • 95 }
    96
    97 int zcreator_step(zcreator_t *zc, knot_rrset_t *rr)
    98 {
    99 assert(zc && rr);
    100
    101 if (rr->type == KNOT_RRTYPE_SOA &&
    102 knot_node_rrset(zc->z->apex, KNOT_RRTYPE_SOA)) {
    103 // Ignore extra SOA
    104 knot_rrset_deep_free(&rr, true, NULL);
    105 return KNOT_EOK;
    106 }
    107
    108 knot_node_t *n;
    109 if (zc->last_node &&
    110 knot_dname_is_equal(zc->last_node->owner, rr->owner)) {
    • Author Contributor

      I wonder if this optimization still has its place, it is O(key_length) as is a trie search, so it should be in the same ballpark.

    • Author Contributor

      Good point, I'll test it.

    • Author Contributor

      2% difference, I'll keep there for now, further optimizations will come with new zone API.

    • Author Contributor

      Hmm, that's on the verge of measurement error (and depends on the dataset). On the other hand, it's not so much extra code, so okay. :white_check_mark:

  • Ghost User
    Ghost User @ghost started a thread on the diff
  • 127 knot_rrset_deep_free(&rr, true, NULL);
    128 }
    129 assert(n);
    130 zc->last_node = n;
    131
    132 bool sem_fatal_error = false;
    133 err_handler_t err_handler;
    134 err_handler_init(&err_handler);
    135 ret = sem_check_node_plain(zc->z, n,
    136 &err_handler, true,
    137 &sem_fatal_error);
    138 if (ret != KNOT_EOK) {
    139 return ret;
    140 }
    141
    142 return sem_fatal_error ? KNOT_EMALF : KNOT_EOK;
  • Ghost User
    Ghost User @ghost started a thread on the diff
  • 27 #ifndef _KNOT_ZONELOAD_H_
    28 #define _KNOT_ZONELOAD_H_
    29
    30 #include <stdio.h>
    31
    32 #include "knot/zone/zone.h"
    33 #include "knot/zone/semantic-check.h"
    34 #include "zscanner/zscanner.h"
    35 /*!
    36 * \brief Zone creator structure.
    37 */
    38 typedef struct zone_creator {
    39 knot_zone_contents_t *z; /*!< Created zone. */
    40 knot_node_t *last_node; /*!< Last used node, use to save zone lookup. */
    41 int ret; /*!< Return value. */
    42 } zcreator_t;
  • Ghost User
    Ghost User @ghost started a thread on the diff
  • 71 *
    72 * \retval Loaded zone contents on success.
    73 * \retval NULL otherwise.
    74 */
    75 knot_zone_contents_t *zonefile_load(zloader_t *loader);
    76
    77 /*!
    78 * \brief Close zone file loader.
    79 *
    80 * \param loader Zone loader instance.
    81 */
    82 void zonefile_close(zloader_t *loader);
    83
    84 knot_zone_contents_t *create_zone_from_name(const char *origin);
    85
    86 int zcreator_step(zcreator_t *zl, knot_rrset_t *rr);
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading