f l a m e . o r g

organized flames

Refactoring a simple function in BIND 9: zone_settimer()

Posted on June 04, 2010

Just like a garage, code needs an occasional cleanup. Without spending some time going back over existing code to make it better, shorter, and better tested, it will be filled with spiders, too. I mean bugs, my garage has the spiders.

As a practical example, I’ve taken a function in BIND 9 and rewritten it incrementally to be more testable, and I think easier to understand. This function, as we will learn later, sets a timer inside the zone to expire at the time of the next zone event. What the various events mean is beyond the scope of this example, but it’s unimportant also.

While the changes are incremental, I will include the full listing in each iteration. This will make this page unbelievably large, but it will make it easier to see the steps as we follow along.

Here’s the code as it exists in BIND 9.7.0’s version of lib/dns/zone.c:

  1 static void
  2 zone_settimer(dns_zone_t *zone, isc_time_t *now) {
  3 	const char me[] = "zone_settimer";
  4 	isc_time_t next;
  5 	isc_result_t result;
  6 
  7 	ENTER;
  8 	REQUIRE(DNS_ZONE_VALID(zone));
  9 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING))
 10 		return;
 11 
 12 	isc_time_settoepoch(&next);
 13 
 14 	switch (zone->type) {
 15 	case dns_zone_master:
 16 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
 17 			next = zone->notifytime;
 18 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
 19 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
 20 			INSIST(!isc_time_isepoch(&zone->dumptime));
 21 			if (isc_time_isepoch(&next) ||
 22 			    isc_time_compare(&zone->dumptime, &next) < 0)
 23 				next = zone->dumptime;
 24 		}
 25 		if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING) &&
 26 		    !isc_time_isepoch(&zone->refreshkeytime)) {
 27 			if (isc_time_isepoch(&next) ||
 28 			    isc_time_compare(&zone->refreshkeytime, &next) < 0)
 29 				next = zone->refreshkeytime;
 30 		}
 31 		if (!isc_time_isepoch(&zone->resigntime)) {
 32 			if (isc_time_isepoch(&next) ||
 33 			    isc_time_compare(&zone->resigntime, &next) < 0)
 34 				next = zone->resigntime;
 35 		}
 36 		if (!isc_time_isepoch(&zone->keywarntime)) {
 37 			if (isc_time_isepoch(&next) ||
 38 			    isc_time_compare(&zone->keywarntime, &next) < 0)
 39 				next = zone->keywarntime;
 40 		}
 41 		if (!isc_time_isepoch(&zone->signingtime)) {
 42 			if (isc_time_isepoch(&next) ||
 43 			    isc_time_compare(&zone->signingtime, &next) < 0)
 44 				next = zone->signingtime;
 45 		}
 46 		if (!isc_time_isepoch(&zone->nsec3chaintime)) {
 47 			if (isc_time_isepoch(&next) ||
 48 			    isc_time_compare(&zone->nsec3chaintime, &next) < 0)
 49 				next = zone->nsec3chaintime;
 50 		}
 51 		break;
 52 
 53 	case dns_zone_slave:
 54 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
 55 			next = zone->notifytime;
 56 		/*FALLTHROUGH*/
 57 
 58 	case dns_zone_stub:
 59 		if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESH) &&
 60 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOMASTERS) &&
 61 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOREFRESH) &&
 62 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING)) {
 63 			INSIST(!isc_time_isepoch(&zone->refreshtime));
 64 			if (isc_time_isepoch(&next) ||
 65 			    isc_time_compare(&zone->refreshtime, &next) < 0)
 66 				next = zone->refreshtime;
 67 		}
 68 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED)) {
 69 			INSIST(!isc_time_isepoch(&zone->expiretime));
 70 			if (isc_time_isepoch(&next) ||
 71 			    isc_time_compare(&zone->expiretime, &next) < 0)
 72 				next = zone->expiretime;
 73 		}
 74 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
 75 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
 76 			INSIST(!isc_time_isepoch(&zone->dumptime));
 77 			if (isc_time_isepoch(&next) ||
 78 			    isc_time_compare(&zone->dumptime, &next) < 0)
 79 				next = zone->dumptime;
 80 		}
 81 		break;
 82 
 83 	case dns_zone_key:
 84 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
 85 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
 86 			INSIST(!isc_time_isepoch(&zone->dumptime));
 87 			if (isc_time_isepoch(&next) ||
 88 			    isc_time_compare(&zone->dumptime, &next) < 0)
 89 				next = zone->dumptime;
 90 		}
 91 		if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING)) {
 92 			if (isc_time_isepoch(&next) ||
 93 			    (!isc_time_isepoch(&zone->refreshkeytime) &&
 94 			    isc_time_compare(&zone->refreshkeytime, &next) < 0))
 95 				next = zone->refreshkeytime;
 96 		}
 97 		break;
 98 
 99 	default:
100 		break;
101 	}
102 
103 	if (isc_time_isepoch(&next)) {
104 		zone_debuglog(zone, me, 10, "settimer inactive");
105 		result = isc_timer_reset(zone->timer, isc_timertype_inactive,
106 					  NULL, NULL, ISC_TRUE);
107 		if (result != ISC_R_SUCCESS)
108 			dns_zone_log(zone, ISC_LOG_ERROR,
109 				     "could not deactivate zone timer: %s",
110 				     isc_result_totext(result));
111 	} else {
112 		if (isc_time_compare(&next, now) <= 0)
113 			next = *now;
114 		result = isc_timer_reset(zone->timer, isc_timertype_once,
115 					 &next, NULL, ISC_TRUE);
116 		if (result != ISC_R_SUCCESS)
117 			dns_zone_log(zone, ISC_LOG_ERROR,
118 				     "could not reset zone timer: %s",
119 				     isc_result_totext(result));
120 	}
121 }

Document

At a first glance, I notice two things. One, there is no description of this function, so I have to read the code to really get what it does. Two, there is a lot of duplication. Let’s fix the documentation part first. I’m something of a C expert and I know well what things like DNS_ZONEFLG_NEEDDUMP means, but if I did not, I would spend quite a bit of time here.

  1 /*
  2  * A zone has one timer, but has events that occur at various times.
  3  * Go through the various events as needed for this zone type, and find
  4  * the one that will trigger next.  Set the zone's timer to this event's
  5  * time.
  6  *
  7  * If no events are scheduled to trigger, stop the timer.
  8  */
  9 static void
 10 zone_settimer(dns_zone_t *zone, isc_time_t *now) {
 11 	const char me[] = "zone_settimer";
 12 	isc_time_t next;
 13 	isc_result_t result;
 14 
 15 	ENTER;
 16 	REQUIRE(DNS_ZONE_VALID(zone));
 17 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING))
 18 		return;
 19 
 20 	isc_time_settoepoch(&next);
 21 
 22 	switch (zone->type) {
 23 	case dns_zone_master:
 24 		/*
 25 		 * If we need to send a notify, set the timer.
 26 		 */
 27 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
 28 			next = zone->notifytime;
 29 
 30 		/* Do we need to dump to disk, and are not doing so? */
 31 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
 32 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
 33 			INSIST(!isc_time_isepoch(&zone->dumptime));
 34 			if (isc_time_isepoch(&next) ||
 35 			    isc_time_compare(&zone->dumptime, &next) < 0)
 36 				next = zone->dumptime;
 37 		}
 38 
 39 		/* Do we need to refresh the signatures? */
 40 		if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING) &&
 41 		    !isc_time_isepoch(&zone->refreshkeytime)) {
 42 			if (isc_time_isepoch(&next) ||
 43 			    isc_time_compare(&zone->refreshkeytime, &next) < 0)
 44 				next = zone->refreshkeytime;
 45 		}
 46 		
 47 		/* Do we need to re-sign any data? */
 48 		if (!isc_time_isepoch(&zone->resigntime)) {
 49 			if (isc_time_isepoch(&next) ||
 50 			    isc_time_compare(&zone->resigntime, &next) < 0)
 51 				next = zone->resigntime;
 52 		}
 53 		
 54 		/* Do we need to warn about expiring keys? */
 55 		if (!isc_time_isepoch(&zone->keywarntime)) {
 56 			if (isc_time_isepoch(&next) ||
 57 			    isc_time_compare(&zone->keywarntime, &next) < 0)
 58 				next = zone->keywarntime;
 59 		}
 60 		
 61 		/* Do we need to sign the zone? */
 62 		if (!isc_time_isepoch(&zone->signingtime)) {
 63 			if (isc_time_isepoch(&next) ||
 64 			    isc_time_compare(&zone->signingtime, &next) < 0)
 65 				next = zone->signingtime;
 66 		}
 67 		
 68 		/* Do we need build or rebuild nsec3 chains? */
 69 		if (!isc_time_isepoch(&zone->nsec3chaintime)) {
 70 			if (isc_time_isepoch(&next) ||
 71 			    isc_time_compare(&zone->nsec3chaintime, &next) < 0)
 72 				next = zone->nsec3chaintime;
 73 		}
 74 		break;
 75 
 76 	case dns_zone_slave:
 77 		/* Do we need to send notifies? */
 78 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
 79 			next = zone->notifytime;
 80 		/*FALLTHROUGH*/
 81 
 82 	case dns_zone_stub:
 83 		/*
 84 		 * If we are not refreshing and can, we have masters,
 85 		 * and we are not loading then check if we should do
 86 		 * do that next.
 87 		 */
 88 		if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESH) &&
 89 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOMASTERS) &&
 90 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOREFRESH) &&
 91 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING)) {
 92 			INSIST(!isc_time_isepoch(&zone->refreshtime));
 93 			if (isc_time_isepoch(&next) ||
 94 			    isc_time_compare(&zone->refreshtime, &next) < 0)
 95 				next = zone->refreshtime;
 96 		}
 97 		
 98 		/* Check expire time if we are loaded */
 99 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED)) {
100 			INSIST(!isc_time_isepoch(&zone->expiretime));
101 			if (isc_time_isepoch(&next) ||
102 			    isc_time_compare(&zone->expiretime, &next) < 0)
103 				next = zone->expiretime;
104 		}
105 		
106 		/* Dump if we need to, unless we already are */
107 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
108 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
109 			INSIST(!isc_time_isepoch(&zone->dumptime));
110 			if (isc_time_isepoch(&next) ||
111 			    isc_time_compare(&zone->dumptime, &next) < 0)
112 				next = zone->dumptime;
113 		}
114 		break;
115 
116 	case dns_zone_key:
117 		/* Dump if we need to, unless we already are */
118 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
119 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
120 			INSIST(!isc_time_isepoch(&zone->dumptime));
121 			if (isc_time_isepoch(&next) ||
122 			    isc_time_compare(&zone->dumptime, &next) < 0)
123 				next = zone->dumptime;
124 		}
125 		
126 		/* Refresh our keys if needed */
127 		if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING)) {
128 			if (isc_time_isepoch(&next) ||
129 			    (!isc_time_isepoch(&zone->refreshkeytime) &&
130 			    isc_time_compare(&zone->refreshkeytime, &next) < 0))
131 				next = zone->refreshkeytime;
132 		}
133 		break;
134 
135 	default:
136 		break;
137 	}
138 
139 	if (isc_time_isepoch(&next)) {  /* No timers are scheduled to trigger */
140 		zone_debuglog(zone, me, 10, "settimer inactive");
141 		result = isc_timer_reset(zone->timer, isc_timertype_inactive,
142 					  NULL, NULL, ISC_TRUE);
143 		if (result != ISC_R_SUCCESS)
144 			dns_zone_log(zone, ISC_LOG_ERROR,
145 				     "could not deactivate zone timer: %s",
146 				     isc_result_totext(result));
147 	} else { /* Set the timer to be the soonest event we need to look at */
148 		if (isc_time_compare(&next, now) <= 0)
149 			next = *now;
150 		result = isc_timer_reset(zone->timer, isc_timertype_once,
151 					 &next, NULL, ISC_TRUE);
152 		if (result != ISC_R_SUCCESS)
153 			dns_zone_log(zone, ISC_LOG_ERROR,
154 				     "could not reset zone timer: %s",
155 				     isc_result_totext(result));
156 	}
157 }

Well, things did not get smaller, but larger! That’s OK though since readability and documentation improved. Just in case, do a compile to ensure we did not break things. If changing comments broke things, get more caffeine. Let’s get to actual code changes!

Factor out some duplicated code

The goal in this refactoring is to end up with a bit of code that could be more easily tested, is more readable, and is, well, less. Less duplicated code means less to test, less possible locations for bugs, and less code to drag along into the future. It is specifically not to change the behavior of the code, just its structure.

One thing that should immediately pop out is the repeated pattern:

1 		if (!isc_time_isepoch(&zone->resigntime)) {
2 			if (isc_time_isepoch(&next) ||
3 			    isc_time_compare(&zone->resigntime, &next) < 0)
4 				next = zone->resigntime;
5 		}

This seems like a first place to start on our cleanup.

This particular code is tested by existing system-level tests. What a good test-driven coder would do here is write the tests for the code they wish they had. For this cleanup, I wish I had a function that would update an accumulator-type variable with a timer if it occurred sooner than what it already held. Go be a good TDD programmer and write that.

Good, got it written? Send it to ISC, please, in the form of a unit test! I’m just going to fumble along and rely on the system-level tests to check my work for this example.

Factoring out some of that repeated code, I came up with this helper function, and refactored the code to use it:

  1 /*
  2  * Set "previous_time" to "new_time" if "new_time" is not the epoch
  3  * and "new_time" will happen sooner than "previous_time", or if
  4  * "previous_time" is the epoch.  In this case, epoch means
  5  * that timer is not set.
  6  */
  7 static void
  8 set_time_if_sooner(isc_time_t *previous_time, isc_time_t *new_time) {
  9 	if (!isc_time_isepoch(new_time)) {
 10 		if (isc_time_isepoch(previous_time) ||
 11 		    isc_time_compare(new_time, previous_time) < 0)
 12 			*previous_time = *new_time;
 13 	}
 14 }
 15 
 16 /*
 17  * A zone has one timer, but has events that occur at various times.
 18  * Go through the various events as needed for this zone type, and find
 19  * the one that will trigger next.  Set the zone's timer to this event's
 20  * time.
 21  *
 22  * If no events are scheduled to trigger, stop the timer.
 23  */
 24 static void
 25 zone_settimer(dns_zone_t *zone, isc_time_t *now) {
 26 	const char me[] = "zone_settimer";
 27 	isc_time_t next;
 28 	isc_result_t result;
 29 
 30 	ENTER;
 31 	REQUIRE(DNS_ZONE_VALID(zone));
 32 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING))
 33 		return;
 34 
 35 	isc_time_settoepoch(&next);
 36 
 37 	switch (zone->type) {
 38 	case dns_zone_master:
 39 		/*
 40 		 * If we need to send a notify, set the timer.
 41 		 */
 42 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
 43 			next = zone->notifytime;
 44 
 45 		/* Do we need to dump to disk, and are not doing so? */
 46 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
 47 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
 48 			INSIST(!isc_time_isepoch(&zone->dumptime));
 49 			set_time_if_sooner(&next, &zone->dumptime);
 50 		}
 51 
 52 		/* Do we need to refresh the signatures? */
 53 		if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
 54 			set_time_if_sooner(&next, &zone->refreshkeytime);
 55 		
 56 		/* Do we need to re-sign any data? */
 57 		set_time_if_sooner(&next, &zone->resigntime);
 58 		
 59 		/* Do we need to warn about expiring keys? */
 60 		set_time_if_sooner(&next, &zone->keywarntime);
 61 		
 62 		/* Do we need to sign the zone? */
 63 		set_time_if_sooner(&next, &zone->signingtime);
 64 		
 65 		/* Do we need build or rebuild nsec3 chains? */
 66 		set_time_if_sooner(&next, &zone->nsec3chaintime);
 67 
 68 		break;
 69 
 70 	case dns_zone_slave:
 71 		/* Do we need to send notifies? */
 72 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
 73 			next = zone->notifytime;
 74 		/*FALLTHROUGH*/
 75 
 76 	case dns_zone_stub:
 77 		/*
 78 		 * If we are not refreshing and can, we have masters,
 79 		 * and we are not loading then check if we should do
 80 		 * do that next.
 81 		 */
 82 		if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESH) &&
 83 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOMASTERS) &&
 84 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOREFRESH) &&
 85 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING))
 86 			set_time_if_sooner(&next, &zone->refreshtime);
 87 		
 88 		/* Check expire time if we are loaded */
 89 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED)) {
 90 			INSIST(!isc_time_isepoch(&zone->expiretime));
 91 			set_time_if_sooner(&next, &zone->expiretime);
 92 		}
 93 		
 94 		/* Dump if we need to, unless we already are */
 95 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
 96 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
 97 			INSIST(!isc_time_isepoch(&zone->dumptime));
 98 			set_time_if_sooner(&next, &zone->dumptime);
 99 		}
100 
101 		break;
102 
103 	case dns_zone_key:
104 		/* Dump if we need to, unless we already are */
105 		if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
106 		    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
107 			INSIST(!isc_time_isepoch(&zone->dumptime));
108 			set_time_if_sooner(&next, &zone->dumptime);
109 		}
110 		
111 		/* Refresh our keys if needed */
112 		if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
113 			set_time_if_sooner(&next, &zone->refreshkeytime);
114 
115 		break;
116 
117 	default:
118 
119 		break;
120 	}
121 
122 	if (isc_time_isepoch(&next)) {  /* No timers are scheduled to trigger */
123 		zone_debuglog(zone, me, 10, "settimer inactive");
124 		result = isc_timer_reset(zone->timer, isc_timertype_inactive,
125 					  NULL, NULL, ISC_TRUE);
126 		if (result != ISC_R_SUCCESS)
127 			dns_zone_log(zone, ISC_LOG_ERROR,
128 				     "could not deactivate zone timer: %s",
129 				     isc_result_totext(result));
130 	} else { /* Set the timer to be the soonest event we need to look at */
131 		if (isc_time_compare(&next, now) <= 0)
132 			next = *now;
133 		result = isc_timer_reset(zone->timer, isc_timertype_once,
134 					 &next, NULL, ISC_TRUE);
135 		if (result != ISC_R_SUCCESS)
136 			dns_zone_log(zone, ISC_LOG_ERROR,
137 				     "could not reset zone timer: %s",
138 				     isc_result_totext(result));
139 	}
140 }

What we have now is a small helper function that we can test, and when it has proven itself we don’t need to worry about cut and paste typos nearly as much. We have less code to test, because we removed a lot of duplication.

Zone-type specific helper functions

Is there more we can do, you ask? There certainly is! I immediately notice that we have a switch statement, and each of its cases are rather long. Let’s make some more helper functions for each of these cases.

  1 /*
  2  * Set "previous_time" to "new_time" if "new_time" is not the epoch
  3  * and "new_time" will happen sooner than "previous_time", or if
  4  * "previous_time" is the epoch.  In this case, epoch means
  5  * that timer is not set.
  6  */
  7 static void
  8 set_time_if_sooner(isc_time_t *previous_time, isc_time_t *new_time) {
  9 	if (!isc_time_isepoch(new_time)) {
 10 		if (isc_time_isepoch(previous_time) ||
 11 		    isc_time_compare(new_time, previous_time) < 0)
 12 			*previous_time = *new_time;
 13 	}
 14 }
 15 
 16 /* find next event for a master zone */
 17 static void
 18 check_master_zone_timers(isc_time_t *next, dns_zone_t *zone) {
 19 	/*
 20 	 * If we need to send a notify, set the timer.
 21 	 */
 22 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
 23 		*next = zone->notifytime;
 24 
 25 	/* Do we need to dump to disk, and are not doing so? */
 26 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
 27 	    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
 28 		INSIST(!isc_time_isepoch(&zone->dumptime));
 29 		set_time_if_sooner(next, &zone->dumptime);
 30 	}
 31 
 32 	/* Do we need to refresh the signatures? */
 33 	if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
 34 		set_time_if_sooner(next, &zone->refreshkeytime);
 35 	
 36 	/* Do we need to re-sign any data? */
 37 	set_time_if_sooner(next, &zone->resigntime);
 38 	
 39 	/* Do we need to warn about expiring keys? */
 40 	set_time_if_sooner(next, &zone->keywarntime);
 41 	
 42 	/* Do we need to sign the zone? */
 43 	set_time_if_sooner(next, &zone->signingtime);
 44 	
 45 	/* Do we need build or rebuild nsec3 chains? */
 46 	set_time_if_sooner(next, &zone->nsec3chaintime);
 47 }
 48 
 49 /* find next event for a slave zone */
 50 static void
 51 check_slave_zone_timers(isc_time_t *next, dns_zone_t *zone) {
 52 	/* Do we need to send notifies? */
 53 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
 54 		*next = zone->notifytime;
 55 }
 56 
 57 /* find next event for a stub (or slave) zone */
 58 static void
 59 check_stub_or_slave_zone_timers(isc_time_t *next, dns_zone_t *zone) {
 60 	/*
 61 	 * If we are not refreshing and can, we have masters,
 62 	 * and we are not loading then check if we should do
 63 	 * do that next.
 64 	 */
 65 	if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESH) &&
 66 	    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOMASTERS) &&
 67 	    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOREFRESH) &&
 68 	    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING))
 69 		set_time_if_sooner(next, &zone->refreshtime);
 70 	
 71 	/* Check expire time if we are loaded */
 72 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED)) {
 73 		INSIST(!isc_time_isepoch(&zone->expiretime));
 74 		set_time_if_sooner(next, &zone->expiretime);
 75 	}
 76 	
 77 	/* Dump if we need to, unless we already are */
 78 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
 79 	    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
 80 		INSIST(!isc_time_isepoch(&zone->dumptime));
 81 		set_time_if_sooner(next, &zone->dumptime);
 82 	}
 83 }
 84 
 85 /* find next event for a key zone */
 86 static void
 87 check_key_zone_timers(isc_time_t *next, dns_zone_t *zone) {
 88 	/* Dump if we need to, unless we already are */
 89 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
 90 	    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
 91 		INSIST(!isc_time_isepoch(&zone->dumptime));
 92 		set_time_if_sooner(next, &zone->dumptime);
 93 	}
 94 	
 95 	/* Refresh our keys if needed */
 96 	if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
 97 		set_time_if_sooner(next, &zone->refreshkeytime);
 98 }
 99 
100 /*
101  * A zone has one timer, but has events that occur at various times.
102  * Go through the various events as needed for this zone type, and find
103  * the one that will trigger next.  Set the zone's timer to this event's
104  * time.
105  *
106  * If no events are scheduled to trigger, stop the timer.
107  */
108 static void
109 zone_settimer(dns_zone_t *zone, isc_time_t *now) {
110 	const char me[] = "zone_settimer";
111 	isc_time_t next;
112 	isc_result_t result;
113 
114 	ENTER;
115 	REQUIRE(DNS_ZONE_VALID(zone));
116 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING))
117 		return;
118 
119 	isc_time_settoepoch(&next);
120 
121 	switch (zone->type) {
122 	case dns_zone_master:
123 		check_master_zone_timers(&next, zone);
124 		break;
125 
126 	case dns_zone_slave:
127 		check_slave_zone_timers(&next, zone);
128 		/*FALLTHROUGH*/
129 
130 	case dns_zone_stub:
131 		check_stub_or_slave_zone_timers(&next, zone);
132 		break;
133 
134 	case dns_zone_key:
135 		check_key_zone_timers(&next, zone);
136 		break;
137 
138 	default:
139 
140 		break;
141 	}
142 
143 	if (isc_time_isepoch(&next)) {  /* No timers are scheduled to trigger */
144 		zone_debuglog(zone, me, 10, "settimer inactive");
145 		result = isc_timer_reset(zone->timer, isc_timertype_inactive,
146 					  NULL, NULL, ISC_TRUE);
147 		if (result != ISC_R_SUCCESS)
148 			dns_zone_log(zone, ISC_LOG_ERROR,
149 				     "could not deactivate zone timer: %s",
150 				     isc_result_totext(result));
151 	} else { /* Set the timer to be the soonest event we need to look at */
152 		if (isc_time_compare(&next, now) <= 0)
153 			next = *now;
154 		result = isc_timer_reset(zone->timer, isc_timertype_once,
155 					 &next, NULL, ISC_TRUE);
156 		if (result != ISC_R_SUCCESS)
157 			dns_zone_log(zone, ISC_LOG_ERROR,
158 				     "could not reset zone timer: %s",
159 				     isc_result_totext(result));
160 	}
161 }

Now we have even more bite-sized, testable functions which we can target and verify independently with unit tests. As before, a good TDD coder would have written unit tests for each of these functions before factoring them out.

Switching

Is there anything else that we might want to do? The switch statement itself should go into a helper function.

  1 /*
  2  * Set "previous_time" to "new_time" if "new_time" is not the epoch
  3  * and "new_time" will happen sooner than "previous_time", or if
  4  * "previous_time" is the epoch.  In this case, epoch means
  5  * that timer is not set.
  6  */
  7 static void
  8 set_time_if_sooner(isc_time_t *previous_time, isc_time_t *new_time) {
  9 	if (!isc_time_isepoch(new_time)) {
 10 		if (isc_time_isepoch(previous_time) ||
 11 		    isc_time_compare(new_time, previous_time) < 0)
 12 			*previous_time = *new_time;
 13 	}
 14 }
 15 
 16 /* find next event for a master zone */
 17 static void
 18 check_master_zone_timers(isc_time_t *next, dns_zone_t *zone) {
 19 	/*
 20 	 * If we need to send a notify, set the timer.
 21 	 */
 22 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
 23 		*next = zone->notifytime;
 24 
 25 	/* Do we need to dump to disk, and are not doing so? */
 26 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
 27 	    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
 28 		INSIST(!isc_time_isepoch(&zone->dumptime));
 29 		set_time_if_sooner(next, &zone->dumptime);
 30 	}
 31 
 32 	/* Do we need to refresh the signatures? */
 33 	if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
 34 		set_time_if_sooner(next, &zone->refreshkeytime);
 35 	
 36 	/* Do we need to re-sign any data? */
 37 	set_time_if_sooner(next, &zone->resigntime);
 38 	
 39 	/* Do we need to warn about expiring keys? */
 40 	set_time_if_sooner(next, &zone->keywarntime);
 41 	
 42 	/* Do we need to sign the zone? */
 43 	set_time_if_sooner(next, &zone->signingtime);
 44 	
 45 	/* Do we need build or rebuild nsec3 chains? */
 46 	set_time_if_sooner(next, &zone->nsec3chaintime);
 47 }
 48 
 49 /* find next event for a slave zone */
 50 static void
 51 check_slave_zone_timers(isc_time_t *next, dns_zone_t *zone) {
 52 	/* Do we need to send notifies? */
 53 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
 54 		*next = zone->notifytime;
 55 }
 56 
 57 /* find next event for a stub (or slave) zone */
 58 static void
 59 check_stub_or_slave_zone_timers(isc_time_t *next, dns_zone_t *zone) {
 60 	/*
 61 	 * If we are not refreshing and can, we have masters,
 62 	 * and we are not loading then check if we should do
 63 	 * do that next.
 64 	 */
 65 	if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESH) &&
 66 	    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOMASTERS) &&
 67 	    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOREFRESH) &&
 68 	    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING))
 69 		set_time_if_sooner(next, &zone->refreshtime);
 70 	
 71 	/* Check expire time if we are loaded */
 72 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED)) {
 73 		INSIST(!isc_time_isepoch(&zone->expiretime));
 74 		set_time_if_sooner(next, &zone->expiretime);
 75 	}
 76 	
 77 	/* Dump if we need to, unless we already are */
 78 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
 79 	    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
 80 		INSIST(!isc_time_isepoch(&zone->dumptime));
 81 		set_time_if_sooner(next, &zone->dumptime);
 82 	}
 83 }
 84 
 85 /* find next event for a key zone */
 86 static void
 87 check_key_zone_timers(isc_time_t *next, dns_zone_t *zone) {
 88 	/* Dump if we need to, unless we already are */
 89 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
 90 	    !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
 91 		INSIST(!isc_time_isepoch(&zone->dumptime));
 92 		set_time_if_sooner(next, &zone->dumptime);
 93 	}
 94 	
 95 	/* Refresh our keys if needed */
 96 	if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
 97 		set_time_if_sooner(next, &zone->refreshkeytime);
 98 }
 99 
100 /* Find the next event based on zone type. */
101 static void
102 check_zone_timers(isc_time_t *next, dns_zone_t *zone) {
103 	switch (zone->type) {
104 	case dns_zone_master:
105 		check_master_zone_timers(next, zone);
106 		break;
107 
108 	case dns_zone_slave:
109 		check_slave_zone_timers(next, zone);
110 		/*FALLTHROUGH*/
111 
112 	case dns_zone_stub:
113 		check_stub_or_slave_zone_timers(next, zone);
114 		break;
115 
116 	case dns_zone_key:
117 		check_key_zone_timers(next, zone);
118 		break;
119 
120 	default:
121 
122 		break;
123 	}
124 }
125 
126 /*
127  * A zone has one timer, but has events that occur at various times.
128  * Go through the various events as needed for this zone type, and find
129  * the one that will trigger next.  Set the zone's timer to this event's
130  * time.
131  *
132  * If no events are scheduled to trigger, stop the timer.
133  */
134 static void
135 zone_settimer(dns_zone_t *zone, isc_time_t *now) {
136 	const char me[] = "zone_settimer";
137 	isc_time_t next;
138 	isc_result_t result;
139 
140 	ENTER;
141 	REQUIRE(DNS_ZONE_VALID(zone));
142 	if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING))
143 		return;
144 
145 	isc_time_settoepoch(&next);
146 
147 	check_zone_timers(&next, zone);
148 
149 	if (isc_time_isepoch(&next)) {  /* No timers are scheduled to trigger */
150 		zone_debuglog(zone, me, 10, "settimer inactive");
151 		result = isc_timer_reset(zone->timer, isc_timertype_inactive,
152 					  NULL, NULL, ISC_TRUE);
153 		if (result != ISC_R_SUCCESS)
154 			dns_zone_log(zone, ISC_LOG_ERROR,
155 				     "could not deactivate zone timer: %s",
156 				     isc_result_totext(result));
157 	} else { /* Set the timer to be the soonest event we need to look at */
158 		if (isc_time_compare(&next, now) <= 0)
159 			next = *now;
160 		result = isc_timer_reset(zone->timer, isc_timertype_once,
161 					 &next, NULL, ISC_TRUE);
162 		if (result != ISC_R_SUCCESS)
163 			dns_zone_log(zone, ISC_LOG_ERROR,
164 				     "could not reset zone timer: %s",
165 				     isc_result_totext(result));
166 	}
167 }

Now we have a simpler function that is easier to read. It is something of a personal preference if we should factor out the case statement. I can see both sides of this. Is the final function easier or harder to read? This is also something of a personal preference, but I believe most people would say it is easier.

Knowing when to stop

Whew, lots of changes so far. Anything else? I would say no, at this point. We could factor the timer setting / canceling code into another helper function, especially if that pattern repeats a lot elsewhere. However, for this code, I think it’s fine to leave it as it stands. There is still some duplication, such as checking for dumptime in multiple functions, but I think that’s fine. The check for the result of timer operations is duplicated as well, but that too seems simple enough as it stands.

Knowing when to stop refactoring is just as critical as identifying the need to refactor. This comes with experience, time, and patience. When the code becomes longer and harder to read by refactoring, it’s a mistake; when the code becomes longer but is more readable, it’s OK. Most of the length increase we added was not code but comments, whitespace, and function declarations. That’s quite OK.