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.