Refactor dns_journal_rollforward() to work over opened journal

Too much logic was cramped inside the dns_journal_rollforward() that
made it harder to follow.  The dns_journal_rollforward() was refactored
to work over already opened journal and some of the previous logic was
moved to new static zone_journal_rollforward() that separates the
journal "rollforward" logic from the "zone" logic.

bin/tests/system/journal/tests.sh

@@ -28,7 +28,7 @@ ret=0
 dig_with_opts changed soa > dig.out.test$n
 grep 'status: NOERROR' dig.out.test$n > /dev/null || ret=1
 grep '2012010902' dig.out.test$n > /dev/null || ret=1
-grep 'zone changed/IN: retried using old journal format' ns1/named.run > /dev/null || ret=1
+grep 'zone changed/IN: journal rollforward completed successfully using old journal format' ns1/named.run > /dev/null || ret=1
 [ $ret -eq 0 ] || echo_i "failed"
 status=`expr $status + $ret`
 
@@ -62,7 +62,8 @@ ret=0
 dig_with_opts changed2 soa > dig.out.test$n
 grep 'status: NOERROR' dig.out.test$n > /dev/null || ret=1
 grep '2012010902' dig.out.test$n > /dev/null || ret=1
-grep 'zone changed2/IN: retried using old journal format' ns1/named.run > /dev/null && ret=1
+grep 'zone changed2/IN: journal rollforward completed successfully: success' ns1/named.run > /dev/null || ret=1
+grep 'zone changed2/IN: journal rollforward completed successfully using old journal format' ns1/named.run > /dev/null && ret=1
 [ $ret -eq 0 ] || echo_i "failed"
 status=`expr $status + $ret`
 
@@ -72,7 +73,8 @@ ret=0
 dig_with_opts unchanged2 soa > dig.out.test$n
 grep 'status: NOERROR' dig.out.test$n > /dev/null || ret=1
 grep '2012010901' dig.out.test$n > /dev/null || ret=1
-grep 'zone unchanged2/IN: retried using old journal format' ns1/named.run > /dev/null && ret=1
+grep 'zone unchanged2/IN: journal rollforward completed successfully' ns1/named.run > /dev/null && ret=1
+grep 'zone unchanged2/IN: journal rollforward completed successfully using old journal format' ns1/named.run > /dev/null && ret=1
 [ $ret -eq 0 ] || echo_i "failed"
 status=`expr $status + $ret`
 
@@ -90,7 +92,7 @@ ret=0
 dig_with_opts -t soa ixfr > dig.out.test$n
 grep 'status: NOERROR' dig.out.test$n > /dev/null || ret=1
 grep '2012010902' dig.out.test$n > /dev/null || ret=1
-grep 'zone ixfr/IN: journal rollforward completed successfully: up to date' ns1/named.run > /dev/null || ret=1
+grep 'zone ixfr/IN: journal rollforward completed successfully using old journal format: up to date' ns1/named.run > /dev/null || ret=1
 [ $ret -eq 0 ] || echo_i "failed"
 status=`expr $status + $ret`
 
@@ -107,7 +109,7 @@ ret=0
 dig_with_opts -t soa hdr1d1d2d1d2 > dig.out.test$n
 grep 'status: NOERROR' dig.out.test$n > /dev/null || ret=1
 grep '2012010905' dig.out.test$n > /dev/null || ret=1
-grep 'zone hdr1d1d2d1d2/IN: journal rollforward completed successfully: success' ns1/named.run > /dev/null || ret=1
+grep 'zone hdr1d1d2d1d2/IN: journal rollforward completed successfully using old journal format: success' ns1/named.run > /dev/null || ret=1
 grep 'zone_journal_compact: zone hdr1d1d2d1d2/IN: repair full journal' ns1/named.run > /dev/null || ret=1
 grep 'hdr1d1d2d1d2/IN: dns_journal_compact: success' ns1/named.run > /dev/null || ret=1
 [ $ret -eq 0 ] || echo_i "failed"
@@ -129,8 +131,7 @@ ret=0
 dig_with_opts -t soa hdr1d2d1d2d1 > dig.out.test$n
 grep 'status: NOERROR' dig.out.test$n > /dev/null || ret=1
 grep '2012010905' dig.out.test$n > /dev/null || ret=1
-grep 'zone hdr1d2d1d2d1/IN: journal rollforward completed successfully: success' ns1/named.run > /dev/null || ret=1
+grep 'zone hdr1d2d1d2d1/IN: journal rollforward completed successfully using old journal format: success' ns1/named.run > /dev/null || ret=1
-grep 'zone hdr1d2d1d2d1/IN: retried using old journal format' ns1/named.run > /dev/null || ret=1
 grep 'zone_journal_compact: zone hdr1d2d1d2d1/IN: repair full journal' ns1/named.run > /dev/null || ret=1
 grep 'zone hdr1d2d1d2d1/IN: dns_journal_compact: success' ns1/named.run > /dev/null || ret=1
 [ $ret -eq 0 ] || echo_i "failed"

lib/dns/include/dns/journal.h

@@ -187,6 +187,12 @@ dns_journal_empty(dns_journal_t *j);
  * Find out if a journal is empty.
  */
 
+bool
+dns_journal_recovered(dns_journal_t *j);
+/*<
+ * Find out if the journal could be opened using old journal format
+ */
+
 uint32_t
 dns_journal_first_serial(dns_journal_t *j);
 uint32_t
@@ -248,23 +254,18 @@ dns_journal_current_rr(dns_journal_t *j, dns_name_t **name, uint32_t *ttl,
  */
 
 isc_result_t
-dns_journal_rollforward(isc_mem_t *mctx, dns_db_t *db, unsigned int options,
+dns_journal_rollforward(dns_journal_t *j, dns_db_t *db, unsigned int options);
-			const char *filename, bool *recovered);
 /*%<
  * Roll forward (play back) the journal file "filename" into the
  * database "db".  This should be called when the server starts
  * after a shutdown or crash.
  *
  * Requires:
- *\li   'mctx' is a valid memory context.
+ *\li   'journal' is a valid journal
  *\li	'db' is a valid database which does not have a version
  *           open for writing.
- *\li   'filename' is the name of the journal file belonging to 'db'.
- *\li	'recovered' is a optional pointer to a boolean that returns
- *	whether a recoverable error was detected.
  *
  * Returns:
- *\li	DNS_R_NOJOURNAL when journal does not exist.
  *\li	ISC_R_NOTFOUND when current serial in not in journal.
  *\li	ISC_R_RANGE when current serial in not in journals range.
  *\li	DNS_R_UPTODATE when the database was already up to date.

lib/dns/journal.c

@@ -1450,8 +1450,8 @@ dns_journal_destroy(dns_journal_t **journalp) {
 
 /* XXX Share code with incoming IXFR? */
 
-static isc_result_t
+isc_result_t
-roll_forward(dns_journal_t *j, dns_db_t *db, unsigned int options) {
+dns_journal_rollforward(dns_journal_t *j, dns_db_t *db, unsigned int options) {
 	isc_buffer_t source; /* Transaction data from disk */
 	isc_buffer_t target; /* Ditto after _fromwire check */
 	uint32_t db_serial;  /* Database SOA serial */
@@ -1597,39 +1597,6 @@ failure:
 	return (result);
 }
 
-isc_result_t
-dns_journal_rollforward(isc_mem_t *mctx, dns_db_t *db, unsigned int options,
-			const char *filename, bool *recovered) {
-	dns_journal_t *j = NULL;
-	isc_result_t result;
-
-	REQUIRE(DNS_DB_VALID(db));
-	REQUIRE(filename != NULL);
-
-	result = dns_journal_open(mctx, filename, DNS_JOURNAL_READ, &j);
-	if (result == ISC_R_NOTFOUND) {
-		isc_log_write(JOURNAL_DEBUG_LOGARGS(3), "no journal file, but "
-							"that's OK");
-		return (DNS_R_NOJOURNAL);
-	}
-	if (result != ISC_R_SUCCESS) {
-		return (result);
-	}
-
-	if (JOURNAL_EMPTY(&j->header)) {
-		CHECK(DNS_R_UPTODATE);
-	}
-
-	result = roll_forward(j, db, options);
-
-failure:
-	if (recovered != NULL) {
-		*recovered = j->recovered;
-	}
-	dns_journal_destroy(&j);
-	return (result);
-}
-
 isc_result_t
 dns_journal_print(isc_mem_t *mctx, uint32_t flags, const char *filename,
 		  FILE *file) {
@@ -1787,6 +1754,11 @@ dns_journal_empty(dns_journal_t *j) {
 	return (JOURNAL_EMPTY(&j->header));
 }
 
+bool
+dns_journal_recovered(dns_journal_t *j) {
+	return (j->recovered);
+}
+
 uint32_t
 dns_journal_first_serial(dns_journal_t *j) {
 	return (j->header.begin.serial);

lib/dns/win32/libdns.def.in

@@ -415,6 +415,7 @@ dns_journal_last_serial
 dns_journal_next_rr
 dns_journal_open
 dns_journal_print
+dns_journal_recovered
 dns_journal_rollforward
 dns_journal_set_sourceserial
 dns_journal_write_transaction

lib/dns/zone.c

@@ -901,6 +901,9 @@ static void
 setrl(isc_ratelimiter_t *rl, unsigned int *rate, unsigned int value);
 static void
 zone_journal_compact(dns_zone_t *zone, dns_db_t *db, uint32_t serial);
+static isc_result_t
+zone_journal_rollforward(dns_zone_t *zone, dns_db_t *db, bool *needdump,
+			 bool *fixjournal);
 
 #define ENTER zone_debuglog(zone, me, 1, "enter")
 
@@ -4680,7 +4683,6 @@ zone_postload(dns_zone_t *zone, dns_db_t *db, isc_time_t loadtime,
 	bool hasinclude = DNS_ZONE_FLAG(zone, DNS_ZONEFLG_HASINCLUDE);
 	bool nomaster = false;
 	bool had_db = false;
-	unsigned int options;
 	dns_include_t *inc;
 	bool is_dynamic = false;
 
@@ -4768,45 +4770,11 @@ zone_postload(dns_zone_t *zone, dns_db_t *db, isc_time_t loadtime,
 	    !DNS_ZONE_OPTION(zone, DNS_ZONEOPT_NOMERGE) &&
 	    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED))
 	{
-		if (zone->type == dns_zone_master &&
+		result = zone_journal_rollforward(zone, db, &needdump,
-		    (inline_secure(zone) ||
+						  &fixjournal);
-		     (zone->update_acl != NULL || zone->ssutable != NULL)))
+		if (result != ISC_R_SUCCESS) {
-		{
-			options = DNS_JOURNALOPT_RESIGN;
-		} else {
-			options = 0;
-		}
-		result = dns_journal_rollforward(zone->mctx, db, options,
-						 zone->journal, &fixjournal);
-		if (result != ISC_R_SUCCESS && result != ISC_R_NOTFOUND &&
-		    result != DNS_R_UPTODATE && result != DNS_R_NOJOURNAL &&
-		    result != ISC_R_RANGE)
-		{
-			dns_zone_logc(zone, DNS_LOGCATEGORY_ZONELOAD,
-				      ISC_LOG_ERROR,
-				      "journal rollforward failed: %s",
-				      dns_result_totext(result));
 			goto cleanup;
 		}
-		if (result == ISC_R_NOTFOUND || result == ISC_R_RANGE) {
-			dns_zone_logc(zone, DNS_LOGCATEGORY_ZONELOAD,
-				      ISC_LOG_ERROR,
-				      "journal rollforward failed: "
-				      "journal out of sync with zone");
-			goto cleanup;
-		}
-		dns_zone_logc(zone, DNS_LOGCATEGORY_ZONELOAD, ISC_LOG_DEBUG(1),
-			      "journal rollforward completed "
-			      "successfully: %s",
-			      dns_result_totext(result));
-		if (result == ISC_R_SUCCESS) {
-			needdump = true;
-		}
-		if (fixjournal) {
-			dns_zone_logc(zone, DNS_LOGCATEGORY_ZONELOAD,
-				      ISC_LOG_ERROR,
-				      "retried using old journal format");
-		}
 	}
 
 	/*
@@ -11335,6 +11303,82 @@ dns_zone_refresh(dns_zone_t *zone) {
 	UNLOCK_ZONE(zone);
 }
 
+static isc_result_t
+zone_journal_rollforward(dns_zone_t *zone, dns_db_t *db, bool *needdump,
+			 bool *fixjournal) {
+	dns_journal_t *journal = NULL;
+	unsigned int options;
+	isc_result_t result;
+
+	if (zone->type == dns_zone_master &&
+	    (inline_secure(zone) ||
+	     (zone->update_acl != NULL || zone->ssutable != NULL)))
+	{
+		options = DNS_JOURNALOPT_RESIGN;
+	} else {
+		options = 0;
+	}
+
+	result = dns_journal_open(zone->mctx, zone->journal, DNS_JOURNAL_READ,
+				  &journal);
+	if (result == ISC_R_NOTFOUND) {
+		dns_zone_logc(zone, DNS_LOGCATEGORY_ZONELOAD, ISC_LOG_DEBUG(3),
+			      "no journal file, but that's OK ");
+		return (ISC_R_SUCCESS);
+	} else if (result != ISC_R_SUCCESS) {
+		dns_zone_logc(zone, DNS_LOGCATEGORY_ZONELOAD, ISC_LOG_ERROR,
+			      "journal open failed: %s",
+			      dns_result_totext(result));
+		return (result);
+	}
+
+	if (dns_journal_empty(journal)) {
+		dns_zone_logc(zone, DNS_LOGCATEGORY_ZONELOAD, ISC_LOG_DEBUG(1),
+			      "journal empty");
+		dns_journal_destroy(&journal);
+		return (ISC_R_SUCCESS);
+	}
+
+	result = dns_journal_rollforward(journal, db, options);
+	switch (result) {
+	case ISC_R_SUCCESS:
+		*needdump = true;
+		/* FALLTHROUGH */
+	case DNS_R_UPTODATE:
+		if (dns_journal_recovered(journal)) {
+			*fixjournal = true;
+			dns_zone_logc(
+				zone, DNS_LOGCATEGORY_ZONELOAD,
+				ISC_LOG_DEBUG(1),
+				"journal rollforward completed successfully "
+				"using old journal format: %s",
+				dns_result_totext(result));
+		} else {
+			dns_zone_logc(zone, DNS_LOGCATEGORY_ZONELOAD,
+				      ISC_LOG_DEBUG(1),
+				      "journal rollforward completed "
+				      "successfully: %s",
+				      dns_result_totext(result));
+		}
+
+		dns_journal_destroy(&journal);
+		return (ISC_R_SUCCESS);
+	case ISC_R_NOTFOUND:
+	case ISC_R_RANGE:
+		dns_zone_logc(zone, DNS_LOGCATEGORY_ZONELOAD, ISC_LOG_ERROR,
+			      "journal rollforward failed: journal out of sync "
+			      "with zone");
+		dns_journal_destroy(&journal);
+		return (result);
+	default:
+		dns_zone_logc(zone, DNS_LOGCATEGORY_ZONELOAD, ISC_LOG_ERROR,
+			      "journal rollforward failed: %s",
+			      dns_result_totext(result));
+		dns_journal_destroy(&journal);
+		return (result);
+	}
+}
+
 static void
 zone_journal_compact(dns_zone_t *zone, dns_db_t *db, uint32_t serial) {
 	isc_result_t result;