organized flames

Search

DumTek, my first released iPhone app!

Posted on August 20, 2010 by Michael

Some of you may know I’m quite interested in Middle Eastern music, and I consider myself a Middle Eastern drummer. It’s just a hobby – nothing professional or anything, but I do really enjoy it.

I also teach drumming when I can, and really enjoy it. Along those lines, for those who have an iPhone, iTouch, or iPad, I have written a drumming application called DumTek that will teach the basic and some filled-in rhythms. The sound is crappy unless you use a headset, and then it is still somewhat crappy, just less so.

I hope to get better samples of my drum, and even let people upload drums later on. For now, though, you’ll have to handle just mine. Sorry :)

Get DumTek from the Application Store!

Tagged with:     | 0 comments

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

Fittness or video game?

Posted on January 06, 2010 by Michael

I have to admit I far prefer video games to any form of exercise. Exercise has always seemed like something other people do. I’m not unhappy that I am overweight. After all, if I were, I’d do something about it.

At first, this Wii Fit thing really confused me. It is exercise, but with a score! Wow! I never got scores when I was in gym class in school. At least not ones I cared all that much about. But these are different. They’re on a video game console, so they are obviously different.

Now I find myself pushing just a little harder to “unlock” some feature or to get a better score. Today, I even realized I pushed myself to run/walk on the treadmill for 30 minutes rather than 20. I had the Wiimote in my pocket of course! The score must be recorded. Otherwise it would be exercise, and not a video game. I ran (at a slow speed, 3.8 miles per hour) for a straight 10 minutes. This might not be amazing to some, but I have never, in my entire life, ran for 10 minutes at any speed, until tonight. I even found the longer I did it, the easier it became. At first, I was panting, but a few minutes in I found breathing was in rhythm with walking, arms just did what arms do, and the feet kept the pace. All in all, it was quite thrilling.

I even find the graph of weight loss to be a seriously cool thing. I don’t get upset when it goes up a bit. Four pounds in two days was a little worrisome… I blame the Cheetos, and the sitting on my butt. However, every day since then the line has been going down. Not by a lot perhaps, 0.2 here and 0.4 there, but every small bit helps. I understand, and more importantly believe, it is the general trend that matters. It isn’t the small increases here and there that matter, nor the large decreases. They are nice to have of course, but not nearly as important as the overall trend line.

Tagged with:    | 1 comment

How my Wii is saving my life

Posted on January 06, 2010 by Michael

What a melodramatic title!

About two years ago, my wife and I purchased a Wii. We bought a few games, but mostly it was for the Wii Fit. For nearly a month we both would weigh in on the thing, play a few games, perhaps even run on the treadmill. Then interest fell off rapidly, and the Wii became more of an interesting Bluetooth device rather than a fitness machine.

Two months ago, we were walking through a local mall and saw a Wii Fit Plus kiosk. This kiosk was populated with beautiful, energetic people who would offer to let you play. If you did not want to embarrass yourself in a public forum, they would demo the new games and training for you.

Since we already had the Wii Board thingy, we could get this new game/exercise program for about $25 or so. We bought one, thinking that even if we got a month’s use out of it, we’d gain something. Or, rather, lose something.

We also bought EA Active, which is another fitness program. It is all serious though, and is much more of a “real” workout.

I am happy to report that for about a week now, my wife and I have been using both programs. We mostly use the Wii Fit Plus for games, a little of the yoga, and to weigh in and record EA Active and other activities. We also use it for “straight runs” which the EA Active program seems to lack. They want me to run, then walk, then do kick-ups, etc. This is all well and good, but I hate running in place – it hurts my knees. Running on the treadmill is fine, however. It’s hard to combine a treadmill with EA Active, at least with what we’re being told to do currently.

Will this trend continue? I actually think it will. What’s different this time is we are both more serious about following through. Last time we were as well, but there was something missing, some sort of small feedback between each other to want to do just a little more. I believe this time we have that, and while it isn’t a competition, it is a shared motivation and a desire to improve ourselves for each other as well as for ourselves.

Tagged with:    | 0 comments

New TiVo

Posted on January 04, 2010 by Michael

Yay! I now have an HD TiVo. Complete with a dual-tuner cable card, so I can finally get back to where I once was with the Driect-TV TiVo I started with so many years ago.

The only problem so far is that it won’t take any random eSATA hard drive. Why? Well, why not? Let’s get you to pay more. Oh well, it’s a one-time purchase I suppose. Still pisses me off.

Tagged with:    | 0 comments

Life without TiVo

Posted on November 30, 2009 by Michael

I’m trying to debug a long-standing (2.5 year!) problem with our Cox Communications service. The symptoms are:

  • The video service has blocking, and even an occasional audio pause. This happens frequently, around 1-5 times in a 2 hour period.
  • The phone service cuts out for short times, perhaps 1-2 seconds. This happens rarely.
  • The internet service drops packets, or only one-way flow of packets seem to to happen. This is hard to detect and diagnose.

Since the video side has the most problems, each of which was recorded on my TiVo, it became the best option to debug.

However, this means I cannot use my TiVo, as Cox suspects it is at fault. Thus, I am downgraded to live TV. Let me tell you, this sucks. I keep on reaching for the remote, hoping it will skip these commercials. Unfortunately, I’m once again in a very comfortable cage, and stuck watching them – not the last of which is that with 12 minutes of commercials in a one hour show, it’s very likely the video blocking will occur during one of these ads.

Tagged with:     | 0 comments

How I test Ruby on Rails with RSpec and Cucumber

Posted on November 19, 2009 by Michael

In any non-trivial application, I end up with several things in common.

  • Generic pages which anyone can see. (I usually make this the About controller)
  • Users
  • Things (DNS Zones, pictures, etc)
  • Access restrictions to those users and things
  • Tests to ensure access control to those users and things

When to use RSpec, and when to use Cucumber

I use RSpec for unit tests, and some low-level controller tests. I specifically do not use RSpec for “user experience” or multiple-step testing, so no views are tested using RSpec.

Cucumber is used for all things “user experience.” It will test that a user cannot access other’s pages, etc. but it doesn’t do anything that can’t easily be done by filling in forms or changing the URL.

Types of users

In my world, there are three kinds of users which appear over and over again.

  • Guests. These guys can look at anything in the About controller.
  • Logged-in users. These can look at anything they own and some things that they do not. They can edit anything they own (with security restrictions on some fields.)
  • Administrators. These are the ones who can look at and modify anything. I usually have an admin rights on/off toggle, and try hard to make what they see close to what users would see.

User tests

  • Tests to ensure that the public cannot do things.
  • Tests to ensure that a logged-in user cannot poke around in someone else’s business using usual page contents.
  • Tests to ensure the the public or a logged in user cannot use specially crafted requests to break things.
  • Tests to ensure that a user can change their own stuff.
  • Tests to ensure that an admin can do pretty much everything.

Most of these tests are a combination of RSpec and Cucumber. The “hack” type tests (like directly fiddling around with assignments which my normal forms may limit to only items the logged in user has access to) are best done with RSpec.

Examples

No blog post can possibly be useful without examples. They use Factory Girl to generate a user, which has an email, password, and an admin flag.

spec/spec_helper.rb snippit

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18

def login_user(options = {})
  @logged_in_user = Factory.create(:user, options)
  @controller.stub!(:current_user).and_return(@logged_in_user)
  @logged_in_user
end

def login_admin(options = {})
  options[:admin] = true
  @logged_in_user = Factory.create(:user, options)
  @controller.stub!(:current_user).and_return(@logged_in_user)
  @logged_in_user
end

def logout_user
  @logged_in_user = nil
  @controller.stub!(:current_user).and_return(@logged_in_user)
  @logged_in_user
end

My complete spec/controllers/userscontrollerspec.rb file

Sorry for the length of this part.

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
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270

require 'spec_helper'

describe UsersController do
  setup :activate_authlogic

  before(:each) do
    logout_user
    @other_user = Factory.create(:user)
  end

  def mock_user(stubs={})
    @mock_user ||= mock_model(User, stubs)
  end

  def make_users
    users = [ @other_user ]
    users << @logged_in_user if @logged_in_user
    4.times do
      users << Factory.create(:user)
    end
    users  
  end

  describe "GET index" do
    it "assigns all users as @users (admin)" do
      login_admin
      users = make_users
      get :index
      assigns[:users].sort.should == users.sort
    end

    it "tells me to bugger off (not admin)" do
      login_user
      users = make_users
      get :index
      flash[:error].should match "You must be an administrator to access this page."
      response.should redirect_to(root_path)
    end

    it "tells me to bugger off (not logged in)" do
      users = make_users
      get :index
      flash[:error].should match "You must be an administrator to access this page."
      response.should redirect_to(root_path)
    end
  end

  describe "GET show" do
    it "assigns myself as @user (admin)" do
      login_admin
      get :show, :id => @logged_in_user.id
      assigns[:user].should == @logged_in_user
    end

    it "assigns me as @user (my data)" do
      login_user
      get :show, :id => @logged_in_user.id
      assigns[:user].should == @logged_in_user
    end

    it "assigns the requested user as @user (admin)" do
      login_admin
      get :show, :id => @other_user.id
      assigns[:user].should == @other_user
    end

    it "tells me to bugger off (not admin)" do
      login_user
      get :show, :id => @other_user.id
      flash[:error].should match "You must be an administrator to access this page."
    end

    it "redirects to root (not admin)" do
      login_user
      get :show, :id => @other_user.id
      response.should redirect_to(root_path)
    end

    it "tells me to log in (not logged in)" do
      get :show, :id => @other_user.id
      flash[:error].should match "Please log in to access this page."
    end

    it "redirects to root (not logged in)" do
      get :show, :id => @other_user.id
      response.should redirect_to(login_path)
    end
  end

  describe "GET new" do
    it "assigns a new user as @user (not logged in)" do
      User.stub!(:new).and_return(mock_user)
      get :new
      assigns[:user].should equal(mock_user)
    end
  end

  describe "GET edit" do
    it "assigns me as @user (admin)" do
      login_admin
      get :edit, :id => @logged_in_user.id
      assigns[:user].should == @logged_in_user
    end

    it "assigns the requested user as @user (admin)" do
      login_admin
      get :edit, :id => @logged_in_user.id
      assigns[:user].should == @logged_in_user
    end

    it "assigns me as @user (my data)" do
      login_user
      get :edit, :id => @logged_in_user.id
      assigns[:user].should == @logged_in_user
    end

    it "tells me to bugger off (not admin)" do
      login_user
      get :edit, :id => @other_user.id
      flash[:error].should match "You must be an administrator to access this page."
      end

    it "redirects to root (not admin)" do
      login_user
      get :edit, :id => @other_user.id
      response.should redirect_to(root_path)
    end

    it "tells me to log in (not logged in)" do
      get :edit, :id => @other_user.id
      flash[:error].should match "Please log in to access this page."
    end

    it "redirects to login (not logged in)" do
      get :edit, :id => @other_user.id
      response.should redirect_to(login_path)
    end
  end

  describe "PUT update" do
    describe "with valid params" do
      it "assigns me as @user (my data)" do
        login_user
        put :update, :id => @logged_in_user.id
        assigns[:user].should == @logged_in_user
      end

      it "tells me to bugger off (not admin)" do
        login_user
        put :update, :id => @other_user.id
        flash[:error].should match "You must be an administrator to access this page."
      end

      it "tells me to log in (not logged in)" do
        get :edit, :id => @other_user.id
        flash[:error].should match "Please log in to access this page."
      end

      it "assigns the requested user as @user (admin)" do
        login_admin
        put :update, :id => @logged_in_user.id
        assigns[:user].should == @logged_in_user
      end

      it "redirects to the user (admin)" do
        login_admin
        put :update, :id => @logged_in_user.id
        response.should redirect_to(user_url(@logged_in_user))
      end

      it "redirects to me (my data)" do
        login_user
        put :update, :id => @logged_in_user.id
        response.should redirect_to(user_url(@logged_in_user))
      end

      it "redirects to root (not admin)" do
        login_user
        put :update, :id => @other_user.id
        response.should redirect_to(root_path)
      end

      it "redirects to login (not logged in)" do
        put :update, :id => @other_user.id
        response.should redirect_to(login_path)
      end

      it "can edit anyone's data (admin)" do
        login_admin
        put :update, :id => @other_user.id, :user => { :email => "new_email@example.com"}
        response.should redirect_to(user_url(@other_user))
        assigns[:user].email.should == "new_email@example.com"
      end

      it "can edit my own data (not admin)" do
        login_user
        put :update, :id => @logged_in_user.id, :user => { :email => "new_email@example.com"}
        response.should redirect_to(user_url(@logged_in_user))
        assigns[:user].email.should == "new_email@example.com"
      end
    end

    describe "with invalid params" do
      it "updates the requested user (admin)" do
        login_admin
        User.should_receive(:find).with("37").and_return(mock_user)
        mock_user.should_receive(:update_attributes).with({'these' => 'params'})
        put :update, :id => "37", :user => {:these => 'params'}
      end

      it "assigns the user as @user (admin)" do
        login_admin
        User.stub!(:find).and_return(mock_user(:update_attributes => false))
        put :update, :id => "1"
        assigns[:user].should equal(mock_user)
      end

      it "re-renders the 'edit' template (admin)" do
        login_admin
        User.stub!(:find).and_return(mock_user(:update_attributes => false))
        put :update, :id => "1"
        response.should render_template('edit')
      end
    end

  end

  describe "DELETE destroy" do
    it "destroys the requested user (admin)" do
      login_admin
      User.should_receive(:find).with("37").and_return(mock_user)
      mock_user.should_receive(:destroy)
      delete :destroy, :id => "37"
    end

    it "redirects to the users list (admin)" do
      login_admin
      User.stub!(:find).and_return(mock_user(:destroy => true))
      delete :destroy, :id => "1"
      response.should redirect_to(users_url)
    end

    it "destroys me (my data)" do
      login_user
      delete :destroy, :id => @logged_in_user.id
    end

    it "tells me to bugger off (not admin)" do
      login_user
      delete :destroy, :id => @other_user.id
      flash[:error].should match "You must be an administrator to access this page."
    end

    it "tells me to log in (not logged in)" do
      delete :destroy, :id => @other_user.id
      flash[:error].should match "Please log in to access this page."
    end

    it "redirects to root (not admin)" do
      login_user
      delete :destroy, :id => @other_user.id
      response.should redirect_to(root_path)
    end

    it "redirects to login (not logged in)" do
      delete :destroy, :id => @other_user.id
      response.should redirect_to(login_path)
    end
  end
end

Filed under: Programming | Tagged with:     | 0 comments

attr_accessible vs accepts_nested_attributes_for

Posted on November 10, 2009 by Michael

First, I just have to say that I never thought I’d be in Japan, let along blogging from here.

Today I ran into a little gotcha with restricting access to models when trying out the Rails 2.3 method of nested models. In my model, I have:

1
2
3
4
5
6

  has_many :dnskeys, :dependent => true
  accepts_nested_attributes_for :dnskeys,
    :allow_destroy => true,
    :reject_if => proc { |attrs| attrs.all? { |k, v| v.blank? }}

  attr_accessible :name

I was surprised, although I should not have been, that this prevented me from posting a zone and its dnskeys in the same action. Adding the proper attr_accessible line made the magic happen again:



  attr_accessible :dnskeys_attributes

Filed under: Programming | Tagged with:    | 1 comment

Experiments with HAML

Posted on November 05, 2009 by Michael

I decided to try HAML today, rather than the standard ERB templates. The jury is still out on if it is better or worse, but like many things Rails, it is better to just dive in and try it.

I have a little project (not yet released) which I’m converting over to use HAML. Since I also use rspec for testing, along with Cucumber and Webrat, I thought I had a long day ahead of me.

It turns out most of it can be made close to automatic, with just a little cleanup afterwards.

Using the rake task below, you can issue these two commands to convert your existing *.html.erb views into *.html.haml. The second target will convert the rspec view tests to expect haml. Renaming the spec tests is not required, but I like sanity.

1
2

  $ rake haml:convert:html
  $ rake haml:convert:spec

If you use a SCM (I still like Subversion best) you’ll want to remove the old files and add the new ones. For Subversion, I do:

1
2

  $ svn rm app/views/*/*.html.erb spec/views/*/*.html.erb_spec.rb
  $ svn add app/views/*/*.html.haml spec/views/*/*.html.haml_spec.rb

The main problems were that not all of the templates were properly indented, and “- end” lines appeared in the HAML templates. Fixing this was fast, as I really don’t have much in my templates yet as this is a new project. This step too could probably be done in an automated way. However, the indentation is probably going to require a bit of manual work.

While there, I also fixed up multi-line constructs generated with a much nicer compact format:

1
2
3
4
5
6
7
8

  %h1
    Title
  %tr
    %th
      Heading
    %th
      Heading
...

Compare with this:

1
2
3
4
5
6

  %h1 Title

  %tr
    %th Heading
    %th Heading
...

And now for the rake task:

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

# RAILS_ROOT/lib/tasks/haml_convert.rake

require 'haml'
require 'haml/exec'
 
namespace :haml do
  namespace :convert do
    desc "Convert all *.html.erb files to *.html.haml"
    task :html do
      Dir.glob("app/views/**/*.html.erb").each do |html|
        puts html + "..."
        Haml::Exec::HTML2Haml.new([html, html.gsub(".html.erb", ".html.haml")]).process_result
        File.delete(html)
      end
    end

    desc "Convert all *.html.erb_spec.rb files to *.html.haml_spec.rb"
    task :spec do
      Dir.glob("spec/views/**/*.html.erb_spec.rb").each do |oldname|
        puts oldname + "..."
        newname = oldname.gsub(".html.erb", ".html.haml")
        nf = File.open(newname, "w")
        File.open(oldname) do |of|
          of.each_line do |line|
            nf.puts line.gsub(".html.erb", ".html.haml")
          end
        end
        nf.close
        #File.delete(oldname)
      end
    end
  end
end

Filed under: Programming | Tagged with:    | 0 comments

ActiveResource and Shallow Nested Routes

Posted on November 04, 2009 by Michael

I recently tried to dive into ActiveResource, which allows a Ruby (usually Ruby on Rails) application to connect to a remote RESTful API and treat it almost as if it were local. There were some problems with my API from ActiveResource’s point of view.

Firstly, I use nested routes. This isn’t really a problem as it is possible to specify a prefix to add to the path, and even to substitution on this path. However, Rails added the concept of a “shallow nested route” a while back.

A shallow nested route is a short-hand notation which allows one to appear to scope out collections (that is, a list of things) from the actual thing itself. For example, in my application (https://dlv.isc.org/) I have:

/users/123 <– specific userid

/users/123/zones <— list of all zones for user 123

/zones/456 <— specific zone

This allows me to look at a specific user’s zones (or them, their own zones) without having to do some sort of special back-end processing which relies on something not in the path, such as the @current_user instance variable. After all, how would an admin list a user’s zones if /zones returned the current user’s zones only? Through an admin interface probably, but that seems messy. Shallow routes are seemingly more clean.

However, they break ActiveResource’s concept of the world.

With a little trickery, however, I managed to get shallow nested routes to work without having to do much additional work. I did have to pass in an additional argument to the collection list(s) however.

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

class Zone < ActiveResource::Base
  self.site = SITE
  self.user = USERNAME
  self.password = PASSWORD
  
  def self.collection_path(prefix_options = {}, query_options = nil)
    prefix_options, query_options = split_options(prefix_options) if query_options.nil?
    "/users/#{USERID}#{prefix(prefix_options)}#{collection_name}.#{format.extension}#{query_string(query_options)}"
  end
end

class Dnskey < ActiveResource::Base
  self.site = SITE
  self.user = USERNAME
  self.password = PASSWORD
  
  def self.collection_path(prefix_options = {}, query_options = nil)
    prefix_options, query_options = split_options(prefix_options) if query_options.nil?
    z = ''
    if query_options.has_key?(:zone_id)
      z = "/zones/#{query_options[:zone_id]}"
      query_options.delete(:zone_id)
    end
    "#{z}#{prefix(prefix_options)}#{collection_name}.#{format.extension}#{query_string(query_options)}"
  end
end

In my case, I used a constant USERID to the Zone’s collection, but left the specific item (aka “elementpath”) alone. For Dnskeys, where I needed to pass in different zoneid values, I had to do a small trick.

I call this as:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

all_zones = Zones.find :all # returns a list of all zones for the USERID
z = Zone.find(1) # returns only Zone with the id of 1
z.destroy # destroys the zone, uses the element_path()

z = Zone.find(2)
keys = Dnskey.find(:all, { :zone_id => z.id }) # disgusting, but works

#
# This is a hack.  I want to use Dnskey.create here, but it won't work since I cannot
# pass the zone_id along, and there are no association hints so I can't use
# zone.dnskeys.create() like I can with ActiveRecord.
#
d = Dnskey.new(:flags => key.flags, :algorithm => key.algorithm.to_i, ...)
d.prefix_options[:zone_id] = zone.id
d.save

Filed under: Programming | Tagged with:     | 0 comments

First steps at "real" web bling

Posted on August 14, 2009 by Michael

You’d not know it from the looks of this blog, but I’m starting to play with “real” web design stuff. You know, fonts that aren’t available on the web for title bars, buttons, etc. Yea, image replacement. Ahh well, I guess it’s time to stop thinking that purist views can be pretty too.

Along these lines, I’ve cracked out Illustrator. It pains me to do stuff in Photoshop for some reason – too limited, things don’t scale properly, etc.

I’d love to hear about any open source tools that can do the steps of “make it pretty”, “edit it easily”, and end with “save slices out to individual files.” Ideally with “write all the XHTML and css files for you in a sane and reliable way.”

Tagged with:   | 0 comments

D-Link VTA-VR with Asterisk

Posted on June 05, 2009 by Michael

While walking around a local store, I came upon the clearance isle and found a D-Link VTA-VR in an opened package. Seeing the price was $25 (which read as $15 at the register) I thought I’d give it a try. The goal is to rip it from its Vonage branding and make it speak to my Asterisk server.

It worked, mostly. There are many guides on the net on how to deal with this device, but it turns out the default password was already open, so perhaps someone had already cracked it for me. Thanks, if so.

The box has two phone jacks, but my first goal was to get just one of them working. This turns out to be trivial – just configure the username, password, and the proxy and away it goes.

The second line turned out to be a problem. This client, like so many other cheap devices, seems to break the SIP protocol. For one, both lines share a single UDP agent port (defaults to 10000, I set it to 5060 for packet capture filter sanity.) This is ok but, when registering at least, Asterisk would often (but not always) report “expired nonce.”

What is an “expired nonce” you ask? It is part of the registration protocol. Basically, the SIP device sends a REGISTER requeest without any login information, and it receives an UNAUTHORIZED response. In that response, however, is some information which can be combined with a username and a password that the client and server know, to generate an authentication token. Part of this is called a “nonce” which is really just a little bit of random data that prevents certain security attacks.

What I was seeing is this:

VTA:  REGISTER for line 1
Asterisk:  UNAUTHORIZED retry with nonce [123456]
VTA:  REGISTER for line 2
Asterisk:  UNAUTHORIZED retry with nonce [abcdef]
VTA:  REGISTER for line 1 with nonce [123456]
Asterisk:  Expired nonce [123456] retry with [deadbeef]
VTA:  REGISTER for line 2 with nonce [abcdef]
Asterisk:  Expired nonce [abcdef] retry with [feedme]
...

The VTA would never recover from this. A bit of random jitter between registration attempts, or only attempting one at a time, would fix this. Using different ports on the VTA device would fix this, but there is no configuration option for that. If the VTA used a different Call-ID for each registration request, it would work.

Unfortunately, there seems to be no way to get both lines working reliably with a single Asterisk server.

There are several hacks I can think of, one is to install a SIP proxy that can register the second line, but would really just proxy the connection to the Asterisk server. Another would be to cause Asterisk to listen on more than one port, and use different ports per line. I don’t think that’s easy or even possible.

Another option would be to hack Asterisk to make it understand that a very popular firmware is rather broken, and it should just expire a nonce based on more criteria than it uses now. Right now, for a given Call-ID, there can be but one nonce. If a new one is issued, the old goes away. If Asterisk were to maintain a linked list of possible values and expire them all when one is found to be working, this might work. It would delay line 2 registration, but that’s quite acceptable.

Myth Busting

I’d also like to dispel some myths that many VTA-VR hacking pages are saying, usually referring to each other in the process.

The VTA-VR uses four UDP ports: One for the shared “agent port”, one for each line’s SIP proxy (usually both are 5060, this is the port the Asterisk server listens on), and one for TLS (which I do not use, but it defaults to 5061.) You cannot set the proxy port to something other than what the asterisk server is listening on, so any comments like “you need to use different ports” is just wrong, as unless your Asterisk server is accepting connections on more than one port, it won’t work.

Setting the “user agent” port to 5060 is handy. If you do this, you can set the defaultip=10.42.1.2 in sip.conf for that line, and even if the device is not registered, Asterisk will still send calls there. This is somewhat scary, but it does seem to work. Sometimes.

Changing the timers does not help a great deal. They are defaults, it’s best to leave them alone.

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

A Real XML API and Rails

Posted on March 06, 2009 by Michael

I recently implemented an XML API that I intended to be used outside of a web browser. Much of the words others have written on the topic are ways to get a Javascript framework to use the authentication_token magic. Some others show the GET side and mention the DELETE but omit the PUT and POST methods.

Here are things I’ve learned:

  • To ensure XML data is returned, use the correct HTTP header: Accept: application/xml
  • Rails assumes that multipart forms and url-encoded forms are from browsers. You can’t use them in a default Rails setup if you want to avoid the authentication_token check.
  • To POST or PUT, use a header of Content-Type: application/xml and include XML data.

It is rather unfortunate that Rails assumes that the encoding ties into a browser. It should be possible to use any encoding so long as the XML data types in the headers are correct. This is probably a bug, but it might be that if you receive XML through an API, you should send XML too.

Example

1
2
3
4
5
6
7
8

curl --user user:password -H 'Accept: application/xml' \
  -H 'Content-Type: application/xml' \
  -X POST -d '<?xml version="1.0" encoding="UTF-8"?>
<item>
  <name>foo</name>
  <description>bar</description>
  <price>100</price>
</item>' http://localhost:3000/items/create

Tagged with:    | 0 comments

Dell and World Record Customer Support

Posted on March 03, 2009 by Michael

Recently a message started appearing that told me my A/C adapter was not recognized by the system. Knowing that the A/C adapter and laptop were covered by a next-day on-side warranty, I called Dell. I carefully explained the problem: the batteries don’t charge any longer, the boot-time error message, and that the green LED on the adapter seems dimmer than it should be.

The technician at Dell decided that, with this set of problems, the motherboard needs to be replaced. I asked if perhaps an adapter should be tried first… No, he assured me, that would not be the problem.

Two days later (which is service contract for “next business day service”) a very friendly and helpful technician arrived, and replaced the motherboard. Same problem. He and I had a good old laugh at Dell for that, and he asked Dell to send a replacement A/C adapter.

Two days later, it arrived. No warning message! Success! But wait… now when I move the laptop it looses power for a brief moment. If there is no battery, it turns off. This was not happening before…

Calling Dell resulted in a mess. The first technician at Dell I spoke with was, to be as kind as possible, a moron. He had me run hardware tests. He had me set the brightness on the laptop LCD to full on and off battery, and since it didn’t flicker anymore was fully prepared to declare the problem resolved. I slowly and carefully explained that this is a physical problem and that moving the computer at all causes it to turn off if the batteries are too low or removed.

True to the incompetence that I have come to expect from nearly every computer company’s tech support, he declared that since no errors show up in the BIOS self-tests, it MUST be software. I asked to speak to his manager.

The manager declared that this was indeed a problem, part of the ongoing issue, but that I needed to ship the computer to Dell. This is because, in the mean time, my service contract has expired. They admit that while this was an on-going problem, and they will fix it, it won’t be done with on-site, and that I have to ship the laptop. I called bullsh*t.

After about 20 minutes of hold-time, two agents, and over 45 minutes of on-the-phone time they finally decided that it was indeed still covered under the same warranty replacement terms as when the problem started, and they will ship another motherboard out and have it replaced again, next-day (in service-contract terms), which means to the rest of us 3 days.

So, so far, to replace a $100 A/C adapter, Dell has wasted five hours of my time and probably two to three times what the laptop is worth in service calls.

Tagged with:   | 0 comments