Merge branch 'zone_load_refactor' into 'master'

zone: refactored post-load procedures to use zone_update structure with advantage

See merge request !767

src/knot/events/handlers/load.c

@@ -15,10 +15,10 @@
  */
 
 #include <assert.h>
-#include <urcu.h>
 
 #include "knot/common/log.h"
 #include "knot/conf/conf.h"
+#include "knot/dnssec/key-events.h"
 #include "knot/dnssec/zone-events.h"
 #include "knot/events/handlers.h"
 #include "knot/events/log.h"
@@ -27,12 +27,37 @@
 #include "knot/zone/zone.h"
 #include "knot/zone/zonefile.h"
 
+static int post_load_dnssec_actions(conf_t *conf, zone_t *zone)
+{
+	kdnssec_ctx_t kctx = { 0 };
+	int ret = kdnssec_ctx_init(conf, &kctx, zone->name, NULL);
+	if (ret != KNOT_EOK) {
+		return ret;
+	}
+
+	bool ignore1 = false; knot_time_t ignore2 = 0;
+	ret = knot_dnssec_nsec3resalt(&kctx, &ignore1, &ignore2);
+	if (ret != KNOT_EOK) {
+		kdnssec_ctx_deinit(&kctx);
+		return ret;
+	}
+
+	if (zone_has_key_sbm(&kctx)) {
+		zone_events_schedule_now(zone, ZONE_EVENT_PARENT_DS_Q);
+	}
+
+	kdnssec_ctx_deinit(&kctx);
+	return KNOT_EOK;
+}
+
 int event_load(conf_t *conf, zone_t *zone)
 {
 	assert(zone);
 
+	conf_val_t val;
 	zone_contents_t *contents = NULL;
 	bool load_from_journal = false;
+	bool contents_in_update = true;
 	zone_sign_reschedule_t dnssec_refresh = { 0 };
 	dnssec_refresh.allow_rollover = true;
 
@@ -76,21 +101,56 @@ int event_load(conf_t *conf, zone_t *zone)
 	zone->zonefile.mtime = mtime;
 
 load_post:
-	/* Post load actions - calculate delta, sign with DNSSEC... */
-	/*! \todo issue #242 dnssec signing should occur in the special event */
-	ret = zone_load_post(conf, zone, &contents, &dnssec_refresh);
+	val = conf_zone_get(conf, C_DNSSEC_SIGNING, zone->name);
+	bool dnssec_enable = conf_bool(&val);
+	val = conf_zone_get(conf, C_IXFR_DIFF, zone->name);
+	bool build_diffs = conf_bool(&val);
+
+	bool old_contents = (zone->contents != NULL);
+	const bool contents_changed = old_contents && (contents != zone->contents);
+
+	/* Build the post-load update structure */
+	zone_update_t post_load = { 0 };
+	if (old_contents) {
+		if (build_diffs && contents_changed) {
+			ret = zone_update_from_differences(&post_load, zone, contents, UPDATE_INCREMENTAL);
+			if (ret == KNOT_ERANGE || ret == KNOT_ENODIFF) {
+				log_zone_warning(zone->name, (ret == KNOT_ENODIFF ?
+					"failed to create journal entry, zone file changed without "
+					"SOA serial update" : "IXFR history will be lost, "
+					"zone file changed, but SOA serial decreased"));
+				ret = zone_update_from_contents(&post_load, zone, contents, UPDATE_FULL);
+			}
+		} else {
+			ret = zone_update_from_contents(&post_load, zone, contents, UPDATE_FULL);
+		}
+	} else {
+		ret = zone_update_from_contents(&post_load, zone, contents, UPDATE_INCREMENTAL);
+	}
 	if (ret != KNOT_EOK) {
+		contents_in_update = false;
 		goto fail;
 	}
 
+	/* Sign zone using DNSSEC (if configured). */
+	if (dnssec_enable) {
+		ret = post_load_dnssec_actions(conf, zone);
+		if (ret == KNOT_EOK) {
+			ret = knot_dnssec_zone_sign(&post_load, 0, &dnssec_refresh);
+		}
+		if (ret != KNOT_EOK) {
+			zone_update_clear(&post_load);
+			goto fail;
+		}
+	}
+
 	/* Everything went alright, switch the contents. */
 	zone->zonefile.exists = !load_from_journal;
-	zone_contents_t *old = zone_switch_contents(zone, contents);
-	bool old_contents = (old != NULL);
-	uint32_t old_serial = zone_contents_serial(old);
-	if (old != NULL) {
-		synchronize_rcu();
-		zone_contents_deep_free(&old);
+	uint32_t old_serial = zone_contents_serial(zone->contents);
+	ret = zone_update_commit(conf, &post_load);
+	zone_update_clear(&post_load);
+	if (ret != KNOT_EOK) {
+		goto fail;
 	}
 
 	uint32_t current_serial = zone_contents_serial(zone->contents);
@@ -112,8 +172,7 @@ load_post:
 	zone->timers.soa_expire = knot_soa_expire(soa);
 	replan_from_timers(conf, zone);
 
-	conf_val_t val = conf_zone_get(conf, C_DNSSEC_SIGNING, zone->name);
-	if (conf_bool(&val)) {
+	if (dnssec_enable) {
 		zone_events_schedule_now(zone, ZONE_EVENT_NSEC3RESALT);
 		// if nothing to be done NOW for any of those, they will replan themselves for later
 
@@ -129,7 +188,9 @@ load_post:
 
 fail:
 	zone->zonefile.exists = false;
-	zone_contents_deep_free(&contents);
+	if (!contents_in_update) {
+		zone_contents_deep_free(&contents);
+	}
 
 	/* Try to bootstrap the zone if local error. */
 	replan_from_timers(conf, zone);

src/knot/updates/zone-update.c

@@ -18,6 +18,7 @@
 #include "knot/dnssec/zone-events.h"
 #include "knot/updates/zone-update.h"
 #include "knot/zone/serial.h"
+#include "knot/zone/zone-diff.h"
 #include "contrib/mempattern.h"
 #include "contrib/ucw/lists.h"
 #include "contrib/ucw/mempool.h"
@@ -35,6 +36,8 @@ static int init_incremental(zone_update_t *update, zone_t *zone)
 		return ret;
 	}
 
+	update->new_cont_deep_copy = false;
+
 	ret = apply_prepare_zone_copy(zone->contents, &update->new_cont);
 	if (ret != KNOT_EOK) {
 		changeset_clear(&update->change);
@@ -63,6 +66,8 @@ static int init_full(zone_update_t *update, zone_t *zone)
 		return KNOT_ENOMEM;
 	}
 
+	update->new_cont_deep_copy = true;
+
 	apply_init_ctx(&update->a_ctx, update->new_cont, 0);
 
 	return KNOT_EOK;
@@ -108,6 +113,75 @@ int zone_update_init(zone_update_t *update, zone_t *zone, zone_update_flags_t fl
 	}
 }
 
+int zone_update_from_differences(zone_update_t *update, zone_t *zone,
+                                 zone_contents_t *new_cont, zone_update_flags_t flags)
+{
+	if (update == NULL || zone == NULL || new_cont == NULL ||
+	    !(flags & UPDATE_INCREMENTAL) || (flags & UPDATE_FULL)) {
+		return KNOT_EINVAL;
+	}
+
+	memset(update, 0, sizeof(*update));
+	update->zone = zone;
+
+	mm_ctx_mempool(&update->mm, MM_DEFAULT_BLKSIZE);
+	update->flags = flags;
+
+	update->new_cont = new_cont;
+	update->new_cont_deep_copy = true;
+
+	int ret = changeset_init(&update->change, zone->name);
+	if (ret != KNOT_EOK) {
+		return ret;
+	}
+
+	ret = zone_contents_diff(zone->contents, new_cont, &update->change);
+	if (ret != KNOT_EOK) {
+		changeset_clear(&update->change);
+		return ret;
+	}
+
+	uint32_t apply_flags = update->flags & UPDATE_STRICT ? APPLY_STRICT : 0;
+	apply_init_ctx(&update->a_ctx, update->new_cont, apply_flags);
+
+	return KNOT_EOK;
+}
+
+int zone_update_from_contents(zone_update_t *update, zone_t *zone_without_contents,
+                              zone_contents_t *new_cont, zone_update_flags_t flags)
+{
+	if (update == NULL || zone_without_contents == NULL || new_cont == NULL) {
+		return KNOT_EINVAL;
+	}
+
+	memset(update, 0, sizeof(*update));
+	update->zone = zone_without_contents;
+
+	mm_ctx_mempool(&update->mm, MM_DEFAULT_BLKSIZE);
+	update->flags = flags;
+
+	update->new_cont = new_cont;
+	update->new_cont_deep_copy = true;
+
+	if (flags & UPDATE_INCREMENTAL) {
+		int ret = changeset_init(&update->change, zone_without_contents->name);
+		if (ret != KNOT_EOK) {
+			return ret;
+		}
+
+		update->change.soa_from = node_create_rrset(new_cont->apex, KNOT_RRTYPE_SOA);
+		if (update->change.soa_from == NULL) {
+			changeset_clear(&update->change);
+			return KNOT_ENOMEM;
+		}
+	}
+
+	uint32_t apply_flags = update->flags & UPDATE_STRICT ? APPLY_STRICT : 0;
+	apply_init_ctx(&update->a_ctx, update->new_cont, apply_flags);
+
+	return KNOT_EOK;
+}
+
 const zone_node_t *zone_update_get_node(zone_update_t *update, const knot_dname_t *dname)
 {
 	if (update == NULL || dname == NULL) {
@@ -178,9 +252,14 @@ void zone_update_clear(zone_update_t *update)
 	if (update->flags & UPDATE_INCREMENTAL) {
 		/* Revert any changes on error, do nothing on success. */
 		update_rollback(&update->a_ctx);
-		update_free_zone(&update->new_cont);
+		if (update->new_cont_deep_copy) {
+			zone_contents_deep_free(&update->new_cont);
+		} else {
+			update_free_zone(&update->new_cont);
+		}
 		changeset_clear(&update->change);
 	} else if (update->flags & UPDATE_FULL) {
+		assert(update->new_cont_deep_copy);
 		zone_contents_deep_free(&update->new_cont);
 	}
 	mp_delete(update->mm.ctx);
@@ -437,6 +516,9 @@ static int commit_incremental(conf_t *conf, zone_update_t *update,
 
 	if (changeset_empty(&update->change)) {
 		changeset_clear(&update->change);
+		if (update->zone->contents == NULL) {
+			*contents_out = update->new_cont;
+		}
 		return KNOT_EOK;
 	}
 
@@ -540,9 +622,14 @@ int zone_update_commit(conf_t *conf, zone_update_t *update)
 	/* Sync RCU. */
 	synchronize_rcu();
 	if (update->flags & UPDATE_FULL) {
+		assert(update->new_cont_deep_copy);
 		zone_contents_deep_free(&old_contents);
 	} else if (update->flags & UPDATE_INCREMENTAL) {
-		update_free_zone(&old_contents);
+		if (update->new_cont_deep_copy) {
+			zone_contents_deep_free(&old_contents);
+		} else {
+			update_free_zone(&old_contents);
+		}
 		changeset_clear(&update->change);
 	}
 	update_cleanup(&update->a_ctx);

src/knot/updates/zone-update.h

@@ -27,6 +27,7 @@
 typedef struct zone_update {
 	zone_t *zone;                /*!< Zone being updated. */
 	zone_contents_t *new_cont;   /*!< New zone contents for full updates. */
+	bool new_cont_deep_copy;     /*!< On update_clear, perform deep free instead of shallow. */
 	changeset_t change;          /*!< Changes we want to apply. */
 	apply_ctx_t a_ctx;           /*!< Context for applying changesets. */
 	uint32_t flags;              /*!< Zone update flags. */
@@ -59,6 +60,36 @@ typedef enum {
  */
 int zone_update_init(zone_update_t *update, zone_t *zone, zone_update_flags_t flags);
 
+/*!
+ * \brief Inits update structure, the update is built like IXFR from differences.
+ *
+ * The existing zone with its own contents is taken as a base,
+ * the new candidate zone contents are taken as new contents,
+ * the diff is calculated, so that this update is INCREMENTAL.
+ *
+ * \param update   Zone update structure to init.
+ * \param zone     Init with this zone.
+ * \param new_cont New zone contents. Will be taken over (and later freed) by zone update.
+ * \param flags    Flags for update. Must be UPDATE_INCREMENTAL.
+ *
+ * \return KNOT_E*
+ */
+int zone_update_from_differences(zone_update_t *update, zone_t *zone,
+                                 zone_contents_t *new_cont, zone_update_flags_t flags);
+
+/*!
+ * \brief Inits a zone update based on new zone contents.
+ *
+ * \param update                 Zone update structure to init.
+ * \param zone_without_contents  Init with this zone. Its contents may be NULL.
+ * \param new_cont               New zone contents. Will be taken over (and later freed) by zone update.
+ * \param flags                  Flags for update.
+ *
+ * \return KNOT_E*
+ */
+int zone_update_from_contents(zone_update_t *update, zone_t *zone_without_contents,
+                              zone_contents_t *new_cont, zone_update_flags_t flags);
+
 /*!
  * \brief Returns node that would be in the zone after updating it.
  *

src/knot/zone/zone-load.c

@@ -208,111 +208,6 @@ int zone_load_from_journal(conf_t *conf, zone_t *zone, zone_contents_t **content
 	return ret;
 }
 
-int zone_load_post(conf_t *conf, zone_t *zone, zone_contents_t **contents,
-		   zone_sign_reschedule_t *dnssec_refresh)
-{
-	if (conf == NULL || zone == NULL || contents == NULL || *contents == NULL) {
-		return KNOT_EINVAL;
-	}
-
-	int ret = KNOT_EOK;
-	changeset_t change = { { 0 } };
-	zone_update_t up = { 0 };
-
-	conf_val_t val = conf_zone_get(conf, C_DNSSEC_SIGNING, zone->name);
-	bool dnssec_enable = conf_bool(&val);
-	val = conf_zone_get(conf, C_IXFR_DIFF, zone->name);
-	bool build_diffs = conf_bool(&val);
-
-	/* Calculate IXFR from differences (if configured or auto DNSSEC). */
-	const bool contents_changed = zone->contents && (*contents != zone->contents);
-	if (contents_changed && build_diffs) {
-		/* The resulting diff will cover DNSSEC changes as well. */
-		ret = changeset_init(&change, zone->name);
-		if (ret != KNOT_EOK) {
-			return ret;
-		}
-		ret = zone_contents_diff(zone->contents, *contents, &change);
-		if (ret == KNOT_ENODIFF) {
-			log_zone_warning(zone->name, "failed to create journal "
-			                 "entry, zone file changed without "
-			                 "SOA serial update");
-			ret = KNOT_EOK;
-		} else if (ret == KNOT_ERANGE) {
-			log_zone_warning(zone->name, "IXFR history will be lost, "
-			                 "zone file changed, but SOA serial decreased");
-			ret = KNOT_EOK;
-		} else if (ret != KNOT_EOK) {
-			log_zone_error(zone->name, "failed to calculate "
-			               "differences from the zone file update (%s)",
-			               knot_strerror(ret));
-			return ret;
-		}
-	}
-	if (!changeset_empty(&change)) {
-		ret = zone_change_store(conf, zone, &change);
-		if (ret == KNOT_ESPACE) {
-			log_zone_error(zone->name, "journal size is too small "
-			               "to fit the changes");
-		} else if (ret != KNOT_EOK) {
-			log_zone_error(zone->name, "failed to store changes into "
-			               "journal (%s)", knot_strerror(ret));
-		}
-	}
-
-	/* Sign zone using DNSSEC (if configured). */
-	if (dnssec_enable) {
-		/* Perform NSEC3 resalt and ZSK rollover if needed. */
-		kdnssec_ctx_t kctx = { 0 };
-		ret = kdnssec_ctx_init(conf, &kctx, zone->name, NULL);
-		if (ret != KNOT_EOK) {
-			return ret;
-		}
-
-		bool ignore1 = false; knot_time_t ignore2 = 0;
-		ret = knot_dnssec_nsec3resalt(&kctx, &ignore1, &ignore2);
-		if (ret != KNOT_EOK) {
-			kdnssec_ctx_deinit(&kctx);
-			return ret;
-		}
-
-		if (zone_has_key_sbm(&kctx)) {
-			zone_events_schedule_now(zone, ZONE_EVENT_PARENT_DS_Q);
-		}
-
-		kdnssec_ctx_deinit(&kctx);
-
-		// zone_update demands zone_t with nonnull contents
-		zone_t fake_zone;
-		memcpy(&fake_zone, zone, sizeof(zone_t));
-		fake_zone.contents = *contents;
-
-		ret = zone_update_init(&up, &fake_zone, UPDATE_INCREMENTAL);
-		if (ret != KNOT_EOK) {
-			return ret;
-		}
-
-		ret = knot_dnssec_zone_sign(&up, 0, dnssec_refresh);
-		if (ret != KNOT_EOK) {
-			zone_update_clear(&up);
-			return ret;
-		}
-
-		ret = zone_update_commit(conf, &up);
-		if (up.new_cont) {
-			zone_update_clear(&up);
-		}
-		if (ret != KNOT_EOK) {
-			return ret;
-		}
-
-		*contents = fake_zone.contents;
-	}
-
-	changeset_clear(&change);
-	return ret;
-}
-
 bool zone_load_can_bootstrap(conf_t *conf, const knot_dname_t *zone_name)
 {
 	if (conf == NULL || zone_name == NULL) {

src/knot/zone/zone-load.h

@@ -54,18 +54,6 @@ int zone_load_journal(conf_t *conf, zone_t *zone, zone_contents_t *contents);
 int zone_load_from_journal(conf_t *conf, zone_t *zone,
                            zone_contents_t **contents);
 
-/*!
- * \brief Zone loading post-actions (zone resign, calculation of delta)
- *
- * \param conf
- * \param zone
- * \param contents
- * \param dnssec_refresh
- * \return KNOT_EOK or an error
- */
-int zone_load_post(conf_t *conf, zone_t *zone, zone_contents_t **contents,
-                   zone_sign_reschedule_t *dnssec_refresh);
-
 /*!
  * \brief Check if zone can be bootstrapped.
  *