From 11689b417ff49528233c0b13bb8bf51a45bb9393 Mon Sep 17 00:00:00 2001
From: Jan Kadlec <jan.kadlec@nic.cz>
Date: Mon, 31 Mar 2014 12:54:38 +0200
Subject: [PATCH] new_node: DDNS fixes, improvements.

---
 src/common/errcode.c         |   2 +-
 src/knot/nameserver/update.c |  55 ++++++--------
 src/knot/server/zones.c      |   8 +--
 src/knot/updates/ddns.c      | 134 +++++++++++------------------------
 src/knot/updates/xfr-in.c    |  22 ++----
 src/libknot/rrset.c          |  28 ++++++++
 6 files changed, 104 insertions(+), 145 deletions(-)

diff --git a/src/common/errcode.c b/src/common/errcode.c
index d85f83adf..ec486961f 100644
--- a/src/common/errcode.c
+++ b/src/common/errcode.c
@@ -39,7 +39,7 @@ const error_table_t knot_error_msgs[] = {
 	{ KNOT_ERANGE, "Value is out of range." },
 
 	/* General errors. */
-	{ KNOT_ERROR, "General error." },
+	{ -10000 /*KNOT_ERROR*/, "General error." },
 	{ KNOT_ENOTRUNNING, "Resource is not running." },
 	{ KNOT_EPARSEFAIL, "Parser failed." },
 	{ KNOT_EEXPIRED, "Resource is expired." },
diff --git a/src/knot/nameserver/update.c b/src/knot/nameserver/update.c
index f29a34501..f094d6562 100644
--- a/src/knot/nameserver/update.c
+++ b/src/knot/nameserver/update.c
@@ -168,25 +168,13 @@ int update_answer(knot_pkt_t *pkt, struct query_data *qdata)
 	}
 }
 
-/*
- * This function should:
- * 1) Create zone shallow copy and the changes structure.
- * 2) Call knot_ddns_process_update().
- *    - If something went bad, call xfrin_rollback_update() and return an error.
- *    - If everything went OK, continue.
- * 3) Finalize the updated zone.
- *
- * NOTE: Mostly copied from xfrin_apply_changesets(). Should be refactored in
- *       order to get rid of duplicate code.
- */
 static int knot_ns_process_update(const knot_pkt_t *query,
                                   zone_t *old_zone,
-                                  knot_zone_contents_t **new_contents,
                                   knot_changesets_t *chgs, uint16_t *rcode,
                                   uint32_t new_serial)
 {
 	if (query == NULL || old_zone == NULL || chgs == NULL ||
-	    EMPTY_LIST(chgs->sets) || new_contents == NULL || rcode == NULL) {
+	    EMPTY_LIST(chgs->sets) || rcode == NULL) {
 		return KNOT_EINVAL;
 	}
 
@@ -197,15 +185,7 @@ static int knot_ns_process_update(const knot_pkt_t *query,
 	int ret = knot_ddns_process_update(old_zone->contents, query,
 	                                   knot_changesets_get_last(chgs),
 	                                   rcode, new_serial);
-	if (ret != KNOT_EOK) {
-		return ret;
-	}
-	
-	const bool change_made =
-		knot_changeset_is_empty(knot_changesets_get_last(chgs));
-	return change_made ? 
-	       xfrin_apply_changesets(old_zone, chgs, new_contents) :
-	       KNOT_EOK;
+	return ret;
 }
 
 static int replan_zone_sign_after_ddns(zone_t *zone, uint32_t refresh_at)
@@ -266,7 +246,7 @@ static int zones_process_update_auth(struct query_data *qdata)
 		return ret;
 	}
 
-	// Process the UPDATE packet, apply to zone, create changesets.
+	// Process the UPDATE packet, create changesets.
 	if (knot_changesets_create_changeset(chgsets) == NULL) {
 		knot_changesets_free(&chgsets);
 		free(msg);
@@ -276,22 +256,34 @@ static int zones_process_update_auth(struct query_data *qdata)
 
 	uint32_t new_serial = zones_next_serial(zone);
 
-	knot_zone_contents_t *new_contents = NULL;
 	knot_zone_contents_t *old_contents = zone->contents;
-	ret = knot_ns_process_update(qdata->query, zone, &new_contents,
-	                             chgsets, &qdata->rcode, new_serial);
+	ret = knot_ns_process_update(qdata->query, zone, chgsets, &qdata->rcode, new_serial);
 	if (ret != KNOT_EOK) {
 		return ret;
 	}
-	
-	if (knot_changeset_is_empty(knot_changesets_get_last(chgsets))) {
+
+	knot_zone_contents_t *new_contents = NULL;
+	const bool change_made =
+		!knot_changeset_is_empty(knot_changesets_get_last(chgsets));
+	if (!change_made) {
 		log_zone_notice("%s: No change to zone made.\n", msg);
 		qdata->rcode = KNOT_RCODE_NOERROR;
 		knot_changesets_free(&chgsets);
 		free(msg);
 		return KNOT_EOK;
+	} else {
+		ret = xfrin_apply_changesets(zone, chgsets, &new_contents);
+		if (ret != KNOT_EOK) {
+			log_zone_notice("%s: Failed to process: %s.\n", msg, knot_strerror(ret));
+			qdata->rcode = KNOT_RCODE_SERVFAIL;
+			knot_changesets_free(&chgsets);
+			free(msg);
+			return ret;
+		}
 	}
 
+	assert(new_contents);
+
 	knot_changesets_t *sec_chs = NULL;
 	knot_changeset_t *sec_ch = NULL;
 	uint32_t refresh_at = 0;
@@ -324,10 +316,9 @@ static int zones_process_update_auth(struct query_data *qdata)
 		} else {
 			// Sign the created changeset
 			ret = knot_dnssec_sign_changeset(new_contents, zone_config,
-			                      knot_changesets_get_last(chgsets),
-			                      sec_ch, KNOT_SOA_SERIAL_KEEP,
-			                      &refresh_at,
-			                      new_serial);
+			                                 knot_changesets_get_last(chgsets),
+			                                 sec_ch, KNOT_SOA_SERIAL_KEEP,
+			                                 &refresh_at, new_serial);
 		}
 
 		if (ret != KNOT_EOK) {
diff --git a/src/knot/server/zones.c b/src/knot/server/zones.c
index 40a68dda9..bb66ac232 100644
--- a/src/knot/server/zones.c
+++ b/src/knot/server/zones.c
@@ -1239,7 +1239,7 @@ static int zones_serialize_and_store_chgset(const knot_changeset_t *chs,
 	int ret = zones_rrset_write_to_mem(chs->soa_from, &entry, &max_size);
 	if (ret != KNOT_EOK) {
 		dbg_zones("%s:%d ret = %s\n", __func__, __LINE__, knot_strerror(ret));
-		return KNOT_ERROR;  /*! \todo Other code? */
+		return ret;
 	}
 
 	/* Serialize RRSets from the 'remove' section. */
@@ -1249,7 +1249,7 @@ static int zones_serialize_and_store_chgset(const knot_changeset_t *chs,
 		ret = zones_rrset_write_to_mem(rrset, &entry, &max_size);
 		if (ret != KNOT_EOK) {
 			dbg_zones("%s:%d ret = %s\n", __func__, __LINE__, knot_strerror(ret));
-			return KNOT_ERROR;  /*! \todo Other code? */
+			return ret;
 		}
 	}
 
@@ -1257,7 +1257,7 @@ static int zones_serialize_and_store_chgset(const knot_changeset_t *chs,
 	ret = zones_rrset_write_to_mem(chs->soa_to, &entry, &max_size);
 	if (ret != KNOT_EOK) {
 		dbg_zones("%s:%d ret = %s\n", __func__, __LINE__, knot_strerror(ret));
-		return KNOT_ERROR;  /*! \todo Other code? */
+		return ret;
 	}
 
 	/* Serialize RRSets from the 'add' section. */
@@ -1266,7 +1266,7 @@ static int zones_serialize_and_store_chgset(const knot_changeset_t *chs,
 		ret = zones_rrset_write_to_mem(rrset, &entry, &max_size);
 		if (ret != KNOT_EOK) {
 			dbg_zones("%s:%d ret = %s\n", __func__, __LINE__, knot_strerror(ret));
-			return KNOT_ERROR;  /*! \todo Other code? */
+			return ret;
 		}
 
 	}
diff --git a/src/knot/updates/ddns.c b/src/knot/updates/ddns.c
index 069ca76db..dcacb5ba8 100644
--- a/src/knot/updates/ddns.c
+++ b/src/knot/updates/ddns.c
@@ -624,7 +624,7 @@ static void knot_ddns_check_add_rr(knot_changeset_t *changeset,
 	}
 }
 
-static int knot_ddns_add_rr_to_chgset(const knot_rrset_t *rr,
+static int add_rr_to_chgset(const knot_rrset_t *rr,
                                       knot_changeset_t *changeset)
 {
 	assert(rr != NULL);
@@ -724,7 +724,7 @@ dbg_ddns_exec_detail(
 	return KNOT_EOK;
 }
 
-static int knot_ddns_rem_rr_to_chgset(const knot_rrset_t *rr,
+static int rem_rr_to_chgset(const knot_rrset_t *rr,
                                       knot_changeset_t *changeset)
 {
 	assert(rr != NULL);
@@ -737,8 +737,6 @@ static int knot_ddns_rem_rr_to_chgset(const knot_rrset_t *rr,
 	if (chgset_rr == NULL) {
 		ret = knot_rrset_copy(rr, &chgset_rr, NULL);
 		if (ret != KNOT_EOK) {
-			dbg_ddns("Failed to copy RR to the changeset: "
-				 "%s\n", knot_strerror(ret));
 			return ret;
 		}
 		/* No such RR in the changeset, add it. */
@@ -746,8 +744,6 @@ static int knot_ddns_rem_rr_to_chgset(const knot_rrset_t *rr,
 		                               KNOT_CHANGESET_REMOVE);
 		if (ret != KNOT_EOK) {
 			knot_rrset_free(&chgset_rr, NULL);
-			dbg_ddns("Failed to add RR to changeset: %s.\n",
-				 knot_strerror(ret));
 			return ret;
 		}
 	} else {
@@ -761,29 +757,27 @@ static int process_add_cname(const knot_node_t *node,
                              const knot_rrset_t *rr,
                              knot_changeset_t *changeset)
 {
-	if (node == NULL) {
-		return KNOT_EOK;
-	}
 	assert(rr != NULL);
 	assert(changeset != NULL);
-
-	dbg_ddns_detail("Adding CNAME RR.\n");
-
-	/* Get the current CNAME RR from the node. */
-	knot_rrset_t *removed = knot_node_create_rrset(node, KNOT_RRTYPE_CNAME);
-
-	if (removed != NULL) {
-		/* If they are identical, ignore. */
-		if (knot_rrset_equal(removed, rr, KNOT_RRSET_COMPARE_WHOLE)
-		    == 1) {
-			dbg_ddns_verb("CNAME identical to one in the node.\n");
-			return 1;
+	// Get the current CNAME RR from the node.
+	knot_rrset_t removed = RRSET_INIT(node, KNOT_RRTYPE_CNAME);
+	if (!knot_rrset_empty(&removed)) {
+		// If they are identical, ignore.
+		if (knot_rrset_equal(&removed, rr, KNOT_RRSET_COMPARE_WHOLE)) {
+			return KNOT_EOK;
 		}
 	
-		return knot_ddns_rem_rr_to_chgset(rr, changeset);
-	} else if (knot_node_rrset_count(node) != 0) {
-		/* 2) Other occupied node => ignore. */
-		return 1;
+		int ret = rem_rr_to_chgset(rr, changeset);
+		if (ret != KNOT_EOK) {
+			return ret;
+		}
+
+		return add_rr_to_chgset(rr, changeset);
+	} else if (node && knot_node_rrset_count(node) > 0) {
+		// Other occupied node => ignore.
+		return KNOT_EOK;
+	} else if (node) {
+		return add_rr_to_chgset(rr, changeset);
 	}
 
 	return KNOT_EOK;
@@ -825,18 +819,17 @@ static int process_rem_rr(const knot_rrset_t *rr,
 	knot_rrset_t intersection;
 	int ret = knot_rrset_intersection(&to_modify, rr, &intersection, NULL);
 	if (ret != KNOT_EOK) {
-		dbg_ddns("ddns: proces_rem_rr: Could not remove RDATA from"
-		         " RRSet (%s).\n", knot_strerror(ret));
 		return ret;
 	}
 
-	/* No such RR in the RRSet. */
 	if (knot_rrset_empty(&intersection)) {
-		dbg_ddns_detail("No such RR found to be removed.\n");
+		// No such RR
 		return KNOT_EOK;
 	}
 
-	ret = knot_ddns_rem_rr_to_chgset(&intersection, changeset);
+	printf("Intersection remove: %d\n", intersection.rrs.rr_count);
+
+	ret = rem_rr_to_chgset(&intersection, changeset);
 	knot_rrs_clear(&intersection.rrs, NULL);
 	return ret;
 }
@@ -868,14 +861,8 @@ static int process_rem_rrset(const knot_rrset_t *rrset,
 		return KNOT_EOK;
 	}
 
-	knot_rrset_t *to_remove = knot_node_create_rrset(node, type);
-	if (to_remove == NULL) {
-		return KNOT_ENOMEM;
-	}
-	
-	int ret = knot_ddns_rem_rr_to_chgset(to_remove, changeset);
-	knot_rrset_free(&to_remove, NULL);
-	return ret;
+	knot_rrset_t to_remove = RRSET_INIT(node, type);
+	return rem_rr_to_chgset(&to_remove, changeset);
 }
 
 static int process_rem_node(const knot_node_t *node, knot_changeset_t *changeset)
@@ -884,8 +871,6 @@ static int process_rem_node(const knot_node_t *node, knot_changeset_t *changeset
 		knot_rrset_t rrset = RRSET_INIT_N(node, i);
 		int ret = process_rem_rrset(&rrset, node, changeset);
 		if (ret != KNOT_EOK) {
-			dbg_ddns("Failed to remove RRSet: %s\n",
-			         knot_strerror(ret));
 			return ret;
 		}
 	}
@@ -898,37 +883,27 @@ static int process_add_soa(const knot_node_t *node,
                            knot_changeset_t *changeset)
 {
 	if (node == NULL) {
+		// Adding SOA to non-existent node, ignore
 		return KNOT_EOK;
 	}
-	assert(node != NULL);
-	assert(rr != NULL);
-
-	dbg_ddns_detail("Adding SOA RR.\n");
-
-	/*
-	 * Just remove the SOA from the node, together with its RRSIGs.
-	 * Adding the RR is done in the caller function. Note that only SOA
-	 * with larger SERIAL than the current one will get to these functions,
-	 * so we don't have to check the SERIALS again.
-	 */
 
 	/* Get the current SOA RR from the node. */
 	knot_rrset_t removed = RRSET_INIT(node, KNOT_RRTYPE_SOA);
 	if (!knot_rrset_empty(&removed)) {
-		dbg_ddns_detail("Found SOA in the node.\n");
 		/* If they are identical, ignore. */
 		if (knot_rrset_equal(&removed, rr, KNOT_RRSET_COMPARE_WHOLE)) {
-			dbg_ddns_detail("Old and new SOA identical.\n");
-			return 1;
+			return KNOT_EOK;
 		}
-		return knot_ddns_rem_rr_to_chgset(rr, changeset);
+		int ret = rem_rr_to_chgset(rr, changeset);
+		if (ret != KNOT_EOK) {
+			return ret;
+		}
+
+		return add_rr_to_chgset(rr, changeset);
 	} else {
-		dbg_ddns_detail("No SOA in node, ignoring.\n");
-		/* If there is no SOA in the node, ignore. */
-		return 1;
+		/* If there is no SOA in the node (i.e. non-apex), ignore. */
+		return KNOT_EOK;
 	}
-
-	return KNOT_EOK;
 }
 
 static int process_add_normal(const knot_node_t *node,
@@ -936,23 +911,13 @@ static int process_add_normal(const knot_node_t *node,
                               knot_changeset_t *changeset)
 {
 	if (knot_node_rrtype_exists(node, KNOT_RRTYPE_CNAME)) {
-		/*
-		 * Adding RR to CNAME node. Ignore the UPDATE RR.
-		 *
-		 * TODO: This may or may not be according to the RFC, it's quite
-		 * unclear (see 3.4.2.2)
-		 */
+		// Adding RR to CNAME node. Ignore the UPDATE RR.
 		return KNOT_EOK;
 	}
 
-	/*
-	 * In all other cases, the RR should just be added to the node.
-	 */
-	dbg_ddns_detail("Adding RR to the changeset.\n");
-	return knot_ddns_add_rr_to_chgset(rr, changeset);
+	return add_rr_to_chgset(rr, changeset);
 }
 
-
 static int process_add(const knot_rrset_t *rr,
                        const knot_node_t *node,
                        knot_changeset_t *changeset)
@@ -1037,18 +1002,6 @@ static int knot_ddns_process_rr(const knot_rrset_t *rr,
 	}
 }
 
-
-
-/*
- * NOTES:
- * - changeset must be allocated
- *
- * All this is done in the first parts of xfrin_apply_changesets() - extract
- * to separate function, if possible.
- *
- * If anything fails, rollback must be done. The xfrin_rollback_update() may
- * be good for this.
- */
 int knot_ddns_process_update(knot_zone_contents_t *zone,
                              const knot_pkt_t *query,
                              knot_changeset_t *changeset,
@@ -1062,7 +1015,7 @@ int knot_ddns_process_update(knot_zone_contents_t *zone,
 
 	/* Copy base SOA RR. */
 	knot_rrset_t *soa_begin = knot_node_create_rrset(knot_zone_contents_apex(zone),
-	                                              KNOT_RRTYPE_SOA);
+	                                                 KNOT_RRTYPE_SOA);
 	knot_rrset_t *soa_end = NULL;
 	if (ret == KNOT_EOK) {
 		knot_changeset_add_soa(changeset, soa_begin,
@@ -1088,7 +1041,6 @@ int knot_ddns_process_update(knot_zone_contents_t *zone,
 	/* Process all RRs the Authority (Update) section. */
 
 	const knot_rrset_t *rr = NULL;
-	knot_rrset_t *rr_copy = NULL;
 
 	dbg_ddns("Processing UPDATE section.\n");
 	size_t apex_ns_removals = 0;
@@ -1147,17 +1099,17 @@ int knot_ddns_process_update(knot_zone_contents_t *zone,
 			assert(knot_serial_compare(sn_rr, sn) > 0);
 			sn_new = sn_rr;
 			soa_end = knot_rrset_cpy(rr, NULL);
+			if (soa_end == NULL) {
+				return KNOT_ENOMEM;
+			}
 		}
 	}
 
 	/* Ending SOA (not in the UPDATE) */
 	if (soa_end == NULL) {
-		/* If the changeset is empty, do not process anything further
-		 * and indicate this to the caller, so that the changeset is not
-		 * saved and zone is not switched.
-		 */
+		// If the changeset is empty, do not process anything further
 		if (knot_changeset_is_empty(changeset)) {
-			return 1;
+			return KNOT_EOK;
 		}
 
 		/* If not set, create new SOA. */
diff --git a/src/knot/updates/xfr-in.c b/src/knot/updates/xfr-in.c
index f1f86a665..e9f5be6f5 100644
--- a/src/knot/updates/xfr-in.c
+++ b/src/knot/updates/xfr-in.c
@@ -726,17 +726,12 @@ dbg_xfrin_exec_detail(
 	}
 	/*!< \todo either one of these checks should be enough. */
 	if (knot_rrset_rr_count(rr_remove) == 0) {
-		/* No RDATA, no need to deep free. */
 		knot_rrset_free(&rr_remove, NULL);
 		dbg_xfrin_verb("Failed to remove RDATA from RRSet\n");
 		// In this case, the RDATA was not found in the RRSet
 		return 1;
 	}
 
-	if (knot_rrset_rr_count(rr_remove) == 0) {
-		knot_rrset_free(&rr_remove, NULL);
-	}
-
 	// if the RRSet is empty, remove from node and add to old RRSets
 	if (knot_rrset_rr_count(*rrset) == 0) {
 		knot_node_remove_rrset(node, knot_rrset_type(*rrset));
@@ -1166,19 +1161,19 @@ dbg_xfrin_exec_verb(
 				// the ADD RRSet was used, i.e. it should be
 				// removed from the changeset and saved in the
 				// list of new RRSets
-				rem_node((node_t *)rr_node);
+//				rem_node((node_t *)rr_node);
 			} else if (ret == 2) {
 				// the copy of the RRSet was used, but it was
 				// already stored in the new RRSets list
 				// just delete the add RRSet, but without RDATA
 				// DNAMES as these were merged to the copied RRSet
-				knot_rrset_free(&rr, NULL);
-				rem_node((node_t *)rr_node);
+//				knot_rrset_free(&rr, NULL);
+//				rem_node((node_t *)rr_node);
 			} else if (ret == 3) {
 				// the RRSet was used and both RRSet and RDATA
 				// were properly stored. Just clear the place
 				// in the changeset
-				rem_node((node_t *)rr_node);
+//				rem_node((node_t *)rr_node);
 			} else {
 				assert(0);
 			}
@@ -1202,14 +1197,7 @@ static int xfrin_apply_replace_soa(knot_zone_contents_t *contents,
 
 	assert(node != NULL);
 
-	int ret = xfrin_replace_rrset_in_node(node, chset->soa_to, contents);
-	if (ret == KNOT_EOK) {
-		// remove the SOA from the changeset, so it will not be deleted
-		// after successful apply
-		chset->soa_to = NULL;
-	}
-
-	return ret;
+	return xfrin_replace_rrset_in_node(node, chset->soa_to, contents);
 }
 
 /*----------------------------------------------------------------------------*/
diff --git a/src/libknot/rrset.c b/src/libknot/rrset.c
index a1832bb09..72fa4f7da 100644
--- a/src/libknot/rrset.c
+++ b/src/libknot/rrset.c
@@ -1266,9 +1266,37 @@ int knot_rrset_remove_rr_using_rrset(knot_rrset_t *from,
 	return KNOT_EOK;
 }
 
+static bool has_rr(const knot_rrset_t *rrset, const knot_rr_t *rr)
+{
+	for (uint16_t i = 0; i < rrset->rrs.rr_count; ++i) {
+		const knot_rr_t *cmp_rr = knot_rrs_rr(&rrset->rrs, i);
+		if (knot_rr_cmp(cmp_rr, rr) == 0) {
+			return true;
+		}
+	}
+	return false;
+}
+
 int knot_rrset_intersection(const knot_rrset_t *a, const knot_rrset_t *b,
                             knot_rrset_t *out, mm_ctx_t *mm)
 {
+	if (a == NULL || b == NULL || !knot_dname_is_equal(a->owner, b->owner) ||
+	    a->type != b->type) {
+		return KNOT_EINVAL;
+	}
+
+	knot_rrset_init(out, a->owner, a->type, a->rclass);
+	for (uint16_t i = 0; i < a->rrs.rr_count; ++i) {
+		const knot_rr_t *rr = knot_rrs_rr(&a->rrs, i);
+		if (has_rr(b, rr)) {
+			int ret = knot_rrset_add_rr_from_rrset(out, a, i, mm);
+			if (ret != KNOT_EOK) {
+				knot_rrs_clear(&out->rrs, mm);
+				return ret;
+			}
+		}
+	}
+
 	return KNOT_EOK;
 }
 
-- 
GitLab