organized flames

Search

Refactoring a simple function in BIND 9: zone_settimer()

Posted on June 04, 2010 by Michael

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
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121

static void
zone_settimer(dns_zone_t *zone, isc_time_t *now) {
  const char me[] = "zone_settimer";
  isc_time_t next;
  isc_result_t result;

  ENTER;
  REQUIRE(DNS_ZONE_VALID(zone));
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING))
    return;

  isc_time_settoepoch(&next);

  switch (zone->type) {
  case dns_zone_master:
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
      next = zone->notifytime;
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->dumptime, &next) < 0)
        next = zone->dumptime;
    }
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING) &&
        !isc_time_isepoch(&zone->refreshkeytime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->refreshkeytime, &next) < 0)
        next = zone->refreshkeytime;
    }
    if (!isc_time_isepoch(&zone->resigntime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->resigntime, &next) < 0)
        next = zone->resigntime;
    }
    if (!isc_time_isepoch(&zone->keywarntime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->keywarntime, &next) < 0)
        next = zone->keywarntime;
    }
    if (!isc_time_isepoch(&zone->signingtime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->signingtime, &next) < 0)
        next = zone->signingtime;
    }
    if (!isc_time_isepoch(&zone->nsec3chaintime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->nsec3chaintime, &next) < 0)
        next = zone->nsec3chaintime;
    }
    break;

  case dns_zone_slave:
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
      next = zone->notifytime;
    /*FALLTHROUGH*/

  case dns_zone_stub:
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESH) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOMASTERS) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOREFRESH) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING)) {
      INSIST(!isc_time_isepoch(&zone->refreshtime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->refreshtime, &next) < 0)
        next = zone->refreshtime;
    }
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED)) {
      INSIST(!isc_time_isepoch(&zone->expiretime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->expiretime, &next) < 0)
        next = zone->expiretime;
    }
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->dumptime, &next) < 0)
        next = zone->dumptime;
    }
    break;

  case dns_zone_key:
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->dumptime, &next) < 0)
        next = zone->dumptime;
    }
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING)) {
      if (isc_time_isepoch(&next) ||
          (!isc_time_isepoch(&zone->refreshkeytime) &&
          isc_time_compare(&zone->refreshkeytime, &next) < 0))
        next = zone->refreshkeytime;
    }
    break;

  default:
    break;
  }

  if (isc_time_isepoch(&next)) {
    zone_debuglog(zone, me, 10, "settimer inactive");
    result = isc_timer_reset(zone->timer, isc_timertype_inactive,
            NULL, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not deactivate zone timer: %s",
             isc_result_totext(result));
  } else {
    if (isc_time_compare(&next, now) <= 0)
      next = *now;
    result = isc_timer_reset(zone->timer, isc_timertype_once,
           &next, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not reset zone timer: %s",
             isc_result_totext(result));
  }
}

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 DNSZONEFLGNEEDDUMP means, but if I did not, I would spend quite a bit of time here.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157

/*
 * A zone has one timer, but has events that occur at various times.
 * Go through the various events as needed for this zone type, and find
 * the one that will trigger next.  Set the zone's timer to this event's
 * time.
 *
 * If no events are scheduled to trigger, stop the timer.
 */
static void
zone_settimer(dns_zone_t *zone, isc_time_t *now) {
  const char me[] = "zone_settimer";
  isc_time_t next;
  isc_result_t result;

  ENTER;
  REQUIRE(DNS_ZONE_VALID(zone));
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING))
    return;

  isc_time_settoepoch(&next);

  switch (zone->type) {
  case dns_zone_master:
    /*
     * If we need to send a notify, set the timer.
     */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
      next = zone->notifytime;

    /* Do we need to dump to disk, and are not doing so? */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->dumptime, &next) < 0)
        next = zone->dumptime;
    }

    /* Do we need to refresh the signatures? */
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING) &&
        !isc_time_isepoch(&zone->refreshkeytime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->refreshkeytime, &next) < 0)
        next = zone->refreshkeytime;
    }
    
    /* Do we need to re-sign any data? */
    if (!isc_time_isepoch(&zone->resigntime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->resigntime, &next) < 0)
        next = zone->resigntime;
    }
    
    /* Do we need to warn about expiring keys? */
    if (!isc_time_isepoch(&zone->keywarntime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->keywarntime, &next) < 0)
        next = zone->keywarntime;
    }
    
    /* Do we need to sign the zone? */
    if (!isc_time_isepoch(&zone->signingtime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->signingtime, &next) < 0)
        next = zone->signingtime;
    }
    
    /* Do we need build or rebuild nsec3 chains? */
    if (!isc_time_isepoch(&zone->nsec3chaintime)) {
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->nsec3chaintime, &next) < 0)
        next = zone->nsec3chaintime;
    }
    break;

  case dns_zone_slave:
    /* Do we need to send notifies? */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
      next = zone->notifytime;
    /*FALLTHROUGH*/

  case dns_zone_stub:
    /*
     * If we are not refreshing and can, we have masters,
     * and we are not loading then check if we should do
     * do that next.
     */
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESH) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOMASTERS) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOREFRESH) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING)) {
      INSIST(!isc_time_isepoch(&zone->refreshtime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->refreshtime, &next) < 0)
        next = zone->refreshtime;
    }
    
    /* Check expire time if we are loaded */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED)) {
      INSIST(!isc_time_isepoch(&zone->expiretime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->expiretime, &next) < 0)
        next = zone->expiretime;
    }
    
    /* Dump if we need to, unless we already are */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->dumptime, &next) < 0)
        next = zone->dumptime;
    }
    break;

  case dns_zone_key:
    /* Dump if we need to, unless we already are */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      if (isc_time_isepoch(&next) ||
          isc_time_compare(&zone->dumptime, &next) < 0)
        next = zone->dumptime;
    }
    
    /* Refresh our keys if needed */
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING)) {
      if (isc_time_isepoch(&next) ||
          (!isc_time_isepoch(&zone->refreshkeytime) &&
          isc_time_compare(&zone->refreshkeytime, &next) < 0))
        next = zone->refreshkeytime;
    }
    break;

  default:
    break;
  }

  if (isc_time_isepoch(&next)) {  /* No timers are scheduled to trigger */
    zone_debuglog(zone, me, 10, "settimer inactive");
    result = isc_timer_reset(zone->timer, isc_timertype_inactive,
            NULL, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not deactivate zone timer: %s",
             isc_result_totext(result));
  } else { /* Set the timer to be the soonest event we need to look at */
    if (isc_time_compare(&next, now) <= 0)
      next = *now;
    result = isc_timer_reset(zone->timer, isc_timertype_once,
           &next, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not reset zone timer: %s",
             isc_result_totext(result));
  }
}

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
2
3
4
5

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

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
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140

/*
 * Set "previous_time" to "new_time" if "new_time" is not the epoch
 * and "new_time" will happen sooner than "previous_time", or if
 * "previous_time" is the epoch.  In this case, epoch means
 * that timer is not set.
 */
static void
set_time_if_sooner(isc_time_t *previous_time, isc_time_t *new_time) {
  if (!isc_time_isepoch(new_time)) {
    if (isc_time_isepoch(previous_time) ||
        isc_time_compare(new_time, previous_time) < 0)
      *previous_time = *new_time;
  }
}

/*
 * A zone has one timer, but has events that occur at various times.
 * Go through the various events as needed for this zone type, and find
 * the one that will trigger next.  Set the zone's timer to this event's
 * time.
 *
 * If no events are scheduled to trigger, stop the timer.
 */
static void
zone_settimer(dns_zone_t *zone, isc_time_t *now) {
  const char me[] = "zone_settimer";
  isc_time_t next;
  isc_result_t result;

  ENTER;
  REQUIRE(DNS_ZONE_VALID(zone));
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING))
    return;

  isc_time_settoepoch(&next);

  switch (zone->type) {
  case dns_zone_master:
    /*
     * If we need to send a notify, set the timer.
     */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
      next = zone->notifytime;

    /* Do we need to dump to disk, and are not doing so? */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      set_time_if_sooner(&next, &zone->dumptime);
    }

    /* Do we need to refresh the signatures? */
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
      set_time_if_sooner(&next, &zone->refreshkeytime);
    
    /* Do we need to re-sign any data? */
    set_time_if_sooner(&next, &zone->resigntime);
    
    /* Do we need to warn about expiring keys? */
    set_time_if_sooner(&next, &zone->keywarntime);
    
    /* Do we need to sign the zone? */
    set_time_if_sooner(&next, &zone->signingtime);
    
    /* Do we need build or rebuild nsec3 chains? */
    set_time_if_sooner(&next, &zone->nsec3chaintime);

    break;

  case dns_zone_slave:
    /* Do we need to send notifies? */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
      next = zone->notifytime;
    /*FALLTHROUGH*/

  case dns_zone_stub:
    /*
     * If we are not refreshing and can, we have masters,
     * and we are not loading then check if we should do
     * do that next.
     */
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESH) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOMASTERS) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOREFRESH) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING))
      set_time_if_sooner(&next, &zone->refreshtime);
    
    /* Check expire time if we are loaded */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED)) {
      INSIST(!isc_time_isepoch(&zone->expiretime));
      set_time_if_sooner(&next, &zone->expiretime);
    }
    
    /* Dump if we need to, unless we already are */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      set_time_if_sooner(&next, &zone->dumptime);
    }

    break;

  case dns_zone_key:
    /* Dump if we need to, unless we already are */
    if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
        !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
      INSIST(!isc_time_isepoch(&zone->dumptime));
      set_time_if_sooner(&next, &zone->dumptime);
    }
    
    /* Refresh our keys if needed */
    if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
      set_time_if_sooner(&next, &zone->refreshkeytime);

    break;

  default:

    break;
  }

  if (isc_time_isepoch(&next)) {  /* No timers are scheduled to trigger */
    zone_debuglog(zone, me, 10, "settimer inactive");
    result = isc_timer_reset(zone->timer, isc_timertype_inactive,
            NULL, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not deactivate zone timer: %s",
             isc_result_totext(result));
  } else { /* Set the timer to be the soonest event we need to look at */
    if (isc_time_compare(&next, now) <= 0)
      next = *now;
    result = isc_timer_reset(zone->timer, isc_timertype_once,
           &next, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not reset zone timer: %s",
             isc_result_totext(result));
  }
}

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
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161

/*
 * Set "previous_time" to "new_time" if "new_time" is not the epoch
 * and "new_time" will happen sooner than "previous_time", or if
 * "previous_time" is the epoch.  In this case, epoch means
 * that timer is not set.
 */
static void
set_time_if_sooner(isc_time_t *previous_time, isc_time_t *new_time) {
  if (!isc_time_isepoch(new_time)) {
    if (isc_time_isepoch(previous_time) ||
        isc_time_compare(new_time, previous_time) < 0)
      *previous_time = *new_time;
  }
}

/* find next event for a master zone */
static void
check_master_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  /*
   * If we need to send a notify, set the timer.
   */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
    *next = zone->notifytime;

  /* Do we need to dump to disk, and are not doing so? */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
    INSIST(!isc_time_isepoch(&zone->dumptime));
    set_time_if_sooner(next, &zone->dumptime);
  }

  /* Do we need to refresh the signatures? */
  if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
    set_time_if_sooner(next, &zone->refreshkeytime);
  
  /* Do we need to re-sign any data? */
  set_time_if_sooner(next, &zone->resigntime);
  
  /* Do we need to warn about expiring keys? */
  set_time_if_sooner(next, &zone->keywarntime);
  
  /* Do we need to sign the zone? */
  set_time_if_sooner(next, &zone->signingtime);
  
  /* Do we need build or rebuild nsec3 chains? */
  set_time_if_sooner(next, &zone->nsec3chaintime);
}

/* find next event for a slave zone */
static void
check_slave_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  /* Do we need to send notifies? */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
    *next = zone->notifytime;
}

/* find next event for a stub (or slave) zone */
static void
check_stub_or_slave_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  /*
   * If we are not refreshing and can, we have masters,
   * and we are not loading then check if we should do
   * do that next.
   */
  if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESH) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOMASTERS) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOREFRESH) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING))
    set_time_if_sooner(next, &zone->refreshtime);
  
  /* Check expire time if we are loaded */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED)) {
    INSIST(!isc_time_isepoch(&zone->expiretime));
    set_time_if_sooner(next, &zone->expiretime);
  }
  
  /* Dump if we need to, unless we already are */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
    INSIST(!isc_time_isepoch(&zone->dumptime));
    set_time_if_sooner(next, &zone->dumptime);
  }
}

/* find next event for a key zone */
static void
check_key_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  /* Dump if we need to, unless we already are */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
    INSIST(!isc_time_isepoch(&zone->dumptime));
    set_time_if_sooner(next, &zone->dumptime);
  }
  
  /* Refresh our keys if needed */
  if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
    set_time_if_sooner(next, &zone->refreshkeytime);
}

/*
 * A zone has one timer, but has events that occur at various times.
 * Go through the various events as needed for this zone type, and find
 * the one that will trigger next.  Set the zone's timer to this event's
 * time.
 *
 * If no events are scheduled to trigger, stop the timer.
 */
static void
zone_settimer(dns_zone_t *zone, isc_time_t *now) {
  const char me[] = "zone_settimer";
  isc_time_t next;
  isc_result_t result;

  ENTER;
  REQUIRE(DNS_ZONE_VALID(zone));
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING))
    return;

  isc_time_settoepoch(&next);

  switch (zone->type) {
  case dns_zone_master:
    check_master_zone_timers(&next, zone);
    break;

  case dns_zone_slave:
    check_slave_zone_timers(&next, zone);
    /*FALLTHROUGH*/

  case dns_zone_stub:
    check_stub_or_slave_zone_timers(&next, zone);
    break;

  case dns_zone_key:
    check_key_zone_timers(&next, zone);
    break;

  default:

    break;
  }

  if (isc_time_isepoch(&next)) {  /* No timers are scheduled to trigger */
    zone_debuglog(zone, me, 10, "settimer inactive");
    result = isc_timer_reset(zone->timer, isc_timertype_inactive,
            NULL, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not deactivate zone timer: %s",
             isc_result_totext(result));
  } else { /* Set the timer to be the soonest event we need to look at */
    if (isc_time_compare(&next, now) <= 0)
      next = *now;
    result = isc_timer_reset(zone->timer, isc_timertype_once,
           &next, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not reset zone timer: %s",
             isc_result_totext(result));
  }
}

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
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167

/*
 * Set "previous_time" to "new_time" if "new_time" is not the epoch
 * and "new_time" will happen sooner than "previous_time", or if
 * "previous_time" is the epoch.  In this case, epoch means
 * that timer is not set.
 */
static void
set_time_if_sooner(isc_time_t *previous_time, isc_time_t *new_time) {
  if (!isc_time_isepoch(new_time)) {
    if (isc_time_isepoch(previous_time) ||
        isc_time_compare(new_time, previous_time) < 0)
      *previous_time = *new_time;
  }
}

/* find next event for a master zone */
static void
check_master_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  /*
   * If we need to send a notify, set the timer.
   */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
    *next = zone->notifytime;

  /* Do we need to dump to disk, and are not doing so? */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
    INSIST(!isc_time_isepoch(&zone->dumptime));
    set_time_if_sooner(next, &zone->dumptime);
  }

  /* Do we need to refresh the signatures? */
  if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
    set_time_if_sooner(next, &zone->refreshkeytime);
  
  /* Do we need to re-sign any data? */
  set_time_if_sooner(next, &zone->resigntime);
  
  /* Do we need to warn about expiring keys? */
  set_time_if_sooner(next, &zone->keywarntime);
  
  /* Do we need to sign the zone? */
  set_time_if_sooner(next, &zone->signingtime);
  
  /* Do we need build or rebuild nsec3 chains? */
  set_time_if_sooner(next, &zone->nsec3chaintime);
}

/* find next event for a slave zone */
static void
check_slave_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  /* Do we need to send notifies? */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDNOTIFY))
    *next = zone->notifytime;
}

/* find next event for a stub (or slave) zone */
static void
check_stub_or_slave_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  /*
   * If we are not refreshing and can, we have masters,
   * and we are not loading then check if we should do
   * do that next.
   */
  if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESH) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOMASTERS) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NOREFRESH) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADING))
    set_time_if_sooner(next, &zone->refreshtime);
  
  /* Check expire time if we are loaded */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_LOADED)) {
    INSIST(!isc_time_isepoch(&zone->expiretime));
    set_time_if_sooner(next, &zone->expiretime);
  }
  
  /* Dump if we need to, unless we already are */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
    INSIST(!isc_time_isepoch(&zone->dumptime));
    set_time_if_sooner(next, &zone->dumptime);
  }
}

/* find next event for a key zone */
static void
check_key_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  /* Dump if we need to, unless we already are */
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_NEEDDUMP) &&
      !DNS_ZONE_FLAG(zone, DNS_ZONEFLG_DUMPING)) {
    INSIST(!isc_time_isepoch(&zone->dumptime));
    set_time_if_sooner(next, &zone->dumptime);
  }
  
  /* Refresh our keys if needed */
  if (!DNS_ZONE_FLAG(zone, DNS_ZONEFLG_REFRESHING))
    set_time_if_sooner(next, &zone->refreshkeytime);
}

/* Find the next event based on zone type. */
static void
check_zone_timers(isc_time_t *next, dns_zone_t *zone) {
  switch (zone->type) {
  case dns_zone_master:
    check_master_zone_timers(next, zone);
    break;

  case dns_zone_slave:
    check_slave_zone_timers(next, zone);
    /*FALLTHROUGH*/

  case dns_zone_stub:
    check_stub_or_slave_zone_timers(next, zone);
    break;

  case dns_zone_key:
    check_key_zone_timers(next, zone);
    break;

  default:

    break;
  }
}

/*
 * A zone has one timer, but has events that occur at various times.
 * Go through the various events as needed for this zone type, and find
 * the one that will trigger next.  Set the zone's timer to this event's
 * time.
 *
 * If no events are scheduled to trigger, stop the timer.
 */
static void
zone_settimer(dns_zone_t *zone, isc_time_t *now) {
  const char me[] = "zone_settimer";
  isc_time_t next;
  isc_result_t result;

  ENTER;
  REQUIRE(DNS_ZONE_VALID(zone));
  if (DNS_ZONE_FLAG(zone, DNS_ZONEFLG_EXITING))
    return;

  isc_time_settoepoch(&next);

  check_zone_timers(&next, zone);

  if (isc_time_isepoch(&next)) {  /* No timers are scheduled to trigger */
    zone_debuglog(zone, me, 10, "settimer inactive");
    result = isc_timer_reset(zone->timer, isc_timertype_inactive,
            NULL, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not deactivate zone timer: %s",
             isc_result_totext(result));
  } else { /* Set the timer to be the soonest event we need to look at */
    if (isc_time_compare(&next, now) <= 0)
      next = *now;
    result = isc_timer_reset(zone->timer, isc_timertype_once,
           &next, NULL, ISC_TRUE);
    if (result != ISC_R_SUCCESS)
      dns_zone_log(zone, ISC_LOG_ERROR,
             "could not reset zone timer: %s",
             isc_result_totext(result));
  }
}

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.

Filed under: Programming | Tagged with:   | 0 comments

DNSSEC vs Firewall

Posted on March 27, 2009 by Michael

A very common cause for DNSSEC validation failure under BIND 9 is firewall issues. Specifically, a firewall that blocks fragments.

To work around this, limiting the packet size one is willing to accept so to avoid fragmentation is a good, but temporary, solution.

options {
  edns-udp-size 1460;
};

This has the side-effect of causing TCP retries on large packets, which are often the DNSKEY responses. However, it also causes DNSSEC to work, so overall it’s a good thing.

Tagged with:   | 0 comments