From: Mark Andrews <marka@isc.org>
Date: Sun, 11 Apr 2021 23:36:18 +0000 (+1000)
Subject: Fixing a recoverable journal should not result in the zone being written
X-Git-Tag: v9.17.13~60^2~2
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=ec7a9af381708b341258b2399c901a3df0a6fd56;p=thirdparty%2Fbind9.git

Fixing a recoverable journal should not result in the zone being written

when dns_journal_rollforward returned ISC_R_RECOVERABLE the distintion
between 'up to date' and 'success' was lost, as a consequence
zone_needdump() was called writing out the zone file when it shouldn't
have been.   This change restores that distintion.  Adjust system
test to reflect visible changes.
---

diff --git a/bin/tests/system/journal/tests.sh b/bin/tests/system/journal/tests.sh
index 824c3723a26..94df34387e3 100644
--- a/bin/tests/system/journal/tests.sh
+++ b/bin/tests/system/journal/tests.sh
@@ -90,7 +90,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: recoverable' ns1/named.run > /dev/null || ret=1
+grep 'zone ixfr/IN: journal rollforward completed successfully: up to date' ns1/named.run > /dev/null || ret=1
 [ $ret -eq 0 ] || echo_i "failed"
 status=`expr $status + $ret`
 
@@ -107,7 +107,9 @@ 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: recoverable' ns1/named.run > /dev/null || ret=1
+grep 'zone hdr1d1d2d1d2/IN: journal rollforward completed successfully: 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"
 status=`expr $status + $ret`
 
@@ -127,7 +129,10 @@ 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: recoverable' ns1/named.run > /dev/null || ret=1
+grep 'zone hdr1d2d1d2d1/IN: journal rollforward completed successfully: 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"
 status=`expr $status + $ret`
 
@@ -175,10 +180,15 @@ status=`expr $status + $ret`
 n=`expr $n + 1`
 echo_i "check max-journal-size works after journal update ($n)"
 ret=0
-# a dump should have been triggered by repairing the journal,
-# which would have resulted in the journal already being
-# compacted.
-[ $(wc -c < ns1/maxjournal.db.jnl) -lt 4000 ] || ret=1
+# journal was repaired, it should still be big
+[ $(wc -c < ns1/maxjournal.db.jnl) -gt 12000 ] || ret=1
+# the zone hasn't been dumped yet, so 'rndc sync' should work without
+# needing a zone update first.
+rndc_with_opts 10.53.0.1 sync maxjournal
+check_size() (
+    [ $(wc -c < ns1/maxjournal.db.jnl) -lt 4000 ]
+)
+retry_quiet 10 check_size || ret=1
 [ $ret -eq 0 ] || echo_i "failed"
 status=`expr $status + $ret`
 
diff --git a/lib/dns/include/dns/journal.h b/lib/dns/include/dns/journal.h
index be6d72ba43f..aad1667bf87 100644
--- a/lib/dns/include/dns/journal.h
+++ b/lib/dns/include/dns/journal.h
@@ -249,7 +249,7 @@ 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,
-			const char *filename);
+			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
@@ -260,6 +260,8 @@ dns_journal_rollforward(isc_mem_t *mctx, dns_db_t *db, unsigned int options,
  *\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.
@@ -268,8 +270,6 @@ dns_journal_rollforward(isc_mem_t *mctx, dns_db_t *db, unsigned int options,
  *\li	DNS_R_UPTODATE when the database was already up to date.
  *\li	ISC_R_SUCCESS journal has been applied successfully to the
  *      database without any issues.
- *\li	DNS_R_RECOVERABLE if successful or up to date, but the journal
- *      was found to contain at least one outdated transaction header.
  *
  *	others
  */
diff --git a/lib/dns/journal.c b/lib/dns/journal.c
index b9318e8de6c..2245122513c 100644
--- a/lib/dns/journal.c
+++ b/lib/dns/journal.c
@@ -1599,7 +1599,7 @@ failure:
 
 isc_result_t
 dns_journal_rollforward(isc_mem_t *mctx, dns_db_t *db, unsigned int options,
-			const char *filename) {
+			const char *filename, bool *recovered) {
 	dns_journal_t *j = NULL;
 	isc_result_t result;
 
@@ -1621,12 +1621,11 @@ dns_journal_rollforward(isc_mem_t *mctx, dns_db_t *db, unsigned int options,
 	}
 
 	result = roll_forward(j, db, options);
-	if ((result == ISC_R_SUCCESS || result == DNS_R_UPTODATE) &&
-	    j->recovered) {
-		result = DNS_R_RECOVERABLE;
-	}
 
 failure:
+	if (recovered != NULL) {
+		*recovered = j->recovered;
+	}
 	dns_journal_destroy(&j);
 	return (result);
 }
diff --git a/lib/dns/zone.c b/lib/dns/zone.c
index 8c55c1774c5..04c65c5450a 100644
--- a/lib/dns/zone.c
+++ b/lib/dns/zone.c
@@ -4777,10 +4777,10 @@ zone_postload(dns_zone_t *zone, dns_db_t *db, isc_time_t loadtime,
 			options = 0;
 		}
 		result = dns_journal_rollforward(zone->mctx, db, options,
-						 zone->journal);
-		if (result != ISC_R_SUCCESS && result != DNS_R_RECOVERABLE &&
-		    result != ISC_R_NOTFOUND && result != DNS_R_UPTODATE &&
-		    result != DNS_R_NOJOURNAL && result != ISC_R_RANGE)
+						 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,
@@ -4801,12 +4801,11 @@ zone_postload(dns_zone_t *zone, dns_db_t *db, isc_time_t loadtime,
 			      dns_result_totext(result));
 		if (result == ISC_R_SUCCESS) {
 			needdump = true;
-		} else if (result == DNS_R_RECOVERABLE) {
+		}
+		if (fixjournal) {
 			dns_zone_logc(zone, DNS_LOGCATEGORY_ZONELOAD,
 				      ISC_LOG_ERROR,
 				      "retried using old journal format");
-			needdump = true;
-			fixjournal = true;
 		}
 	}
 
@@ -5130,12 +5129,12 @@ zone_postload(dns_zone_t *zone, dns_db_t *db, isc_time_t loadtime,
 
 	result = ISC_R_SUCCESS;
 
+	if (fixjournal) {
+		DNS_ZONE_SETFLAG(zone, DNS_ZONEFLG_FIXJOURNAL);
+		zone_journal_compact(zone, zone->db, 0);
+	}
 	if (needdump) {
-		if (fixjournal) {
-			DNS_ZONE_SETFLAG(zone, DNS_ZONEFLG_FIXJOURNAL);
-			zone_journal_compact(zone, zone->db, 0);
-			zone_needdump(zone, 0);
-		} else if (zone->type == dns_zone_key) {
+		if (zone->type == dns_zone_key) {
 			zone_needdump(zone, 30);
 		} else {
 			zone_needdump(zone, DNS_DUMP_DELAY);