Merge lp:~stefan.goetz-deactivatedaccount/hipl/lib-core-fixes into lp:hipl
- lib-core-fixes
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 5091 |
Proposed branch: | lp:~stefan.goetz-deactivatedaccount/hipl/lib-core-fixes |
Merge into: | lp:hipl |
Prerequisite: | lp:~stefan.goetz-deactivatedaccount/hipl/unittests |
Diff against target: |
951 lines (+231/-470) 9 files modified
hipd/cert.c (+9/-13) hipd/hidb.c (+1/-1) lib/core/conf.c (+18/-26) lib/core/hit.c (+43/-70) lib/core/hit.h (+3/-6) lib/core/straddr.c (+48/-150) lib/core/straddr.h (+3/-6) test/lib/core/hit.c (+26/-56) test/lib/core/straddr.c (+80/-142) |
To merge this branch: | bzr merge lp:~stefan.goetz-deactivatedaccount/hipl/lib-core-fixes |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
HIPL core team | Pending | ||
Review via email: mp+38825@code.launchpad.net |
Commit message
Description of the change
This branch fixes the files lib/core/hit.c and lib/core/straddr.c so that they 1) no longer fail the unit tests in the unittests branch and 2) are generally saner and easier to work, including much improved documentation.
This branch complements the branch lp:~stefan.goetz/hipl/unittests
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : | # |
- 5003. By Stefan Götz
-
In response to https:/
/code.launchpad .net/~stefan. goetz/hipl/ lib-core- fixes/+ merge/38825/ comments/ 87700:
Removed NULL constants from validity checks as they were felt as being unnecessarily verbose. - 5004. By Stefan Götz
-
Pulled lp:~stefan.goetz/hipl/unittests for mergeability
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : | # |
Revision 5003 addresses the concerns about NULL pointer checks raised by Diego. It should thus fix out all outstanding merge blockers. Revision 5004 ensures mergeability on top of lp:~stefan.goetz/hipl/unittests. The branch merges cleanly into the current trunk revision 5028. Unless there are further issues, I herewith re-request this branch to be merged into trunk :-)
- 5005. By Stefan Götz
-
Removed unnecessarily included header files from lib/core/straddr.c
- 5006. By Stefan Götz
-
Removed unnecessarily included header files from lib/core/hit.c
- 5007. By Stefan Götz
-
Pulled lp:~stefan.goetz/hipl/unittests for mergeability
Diego Biurrun (diego-biurrun) wrote : | # |
On Tue, Oct 19, 2010 at 04:26:03PM +0000, Stefan Götz wrote:
>
> > This [the branch] looks like it could be split into two parts, one for the
> > fixes and one for the unit tests...
>
> Technically, it could. Logically however, the intention is to start
> the 'don't commit things that break the unit tests' policy.
Let that policy take effect one commit later. I'm sure nobody will be
able to slip a mickey into HIPL between REV_FIXES and REV_FIXES + 1.
> >> --- lib/core/hit.c 2010-10-18 17:44:31 +0000
> >> +++ lib/core/hit.c 2010-10-19 12:14:42 +0000
> >> @@ -42,71 +42,48 @@
> >>
> >> + if (hit != NULL && hit_str != NULL) {
> >> + if (hip_in6_ntop(hit, hit_str) != NULL) {
> >
> > I don't think the '!= NULL' expressions aid readability.
>
> As opposed to writing "if (hit && hit_str)"?
Yes.
> > In any case these two if clauses can be combined.
>
> Technically, they could. Logically, there are two different checks
> here. First, I check my input (i.e. hit and hit_str), then I do
> the work (hip_in6_ntop()). In my eyes, these two steps are worth
> separating 1) for comprehensibility and 2) if changes become
> necessary. I concede that this is a purely philosophical point :)
Whatever you prefer.
Notice that I always consider my review comments suggestions to be
followed or ignored at the coder's discretion. Unless I'm pointing
out bugs of course :)
Diego
Preview Diff
1 | === modified file 'hipd/cert.c' |
2 | --- hipd/cert.c 2010-10-15 15:29:14 +0000 |
3 | +++ hipd/cert.c 2010-10-20 08:51:06 +0000 |
4 | @@ -194,11 +194,11 @@ |
5 | /* clearing signature field just to be sure */ |
6 | memset(cert->signature, '\0', sizeof(cert->signature)); |
7 | |
8 | - HIP_IFEL(!(digest_b64 = base64_encode((unsigned char *) sha_digest, |
9 | - (unsigned int) sizeof(sha_digest))), |
10 | + HIP_IFEL(EVP_EncodeBlock(digest_b64, (unsigned char *) sha_digest, |
11 | + (unsigned int) sizeof(sha_digest)) > 0, |
12 | -1, "Failed to encode digest_b64\n"); |
13 | - HIP_IFEL(!(signature_b64 = base64_encode((unsigned char *) signature, |
14 | - (unsigned int) sig_len)), |
15 | + HIP_IFEL(EVP_EncodeBlock(signature_b64, (unsigned char *) signature, |
16 | + (unsigned int) sig_len) > 0, |
17 | -1, "Failed to encode signature_b64\n"); |
18 | /* create (signature (hash sha1 |digest|)|signature|) */ |
19 | sprintf(cert->signature, "(signature (hash sha1 |%s|)|%s|)", |
20 | @@ -218,7 +218,7 @@ |
21 | -1, |
22 | "Error in converting public exponent from BN to bin\n"); |
23 | |
24 | - HIP_IFEL(!(n_b64 = base64_encode((unsigned char *) n_bin, RSA_size(rsa))), |
25 | + HIP_IFEL(EVP_EncodeBlock(n_b64, (unsigned char *) n_bin, RSA_size(rsa)) > 0, |
26 | -1, |
27 | "Failed to encode n_b64\n"); |
28 | |
29 | @@ -244,26 +244,22 @@ |
30 | */ |
31 | HIP_IFEL(!(BN_bn2bin(dsa->p, p_bin)), -1, |
32 | "Error in converting public exponent from BN to bin\n"); |
33 | - HIP_IFEL(!(p_b64 = base64_encode((unsigned char *) p_bin, |
34 | - BN_num_bytes(dsa->p))), |
35 | + HIP_IFEL(EVP_EncodeBlock(p_b64, (unsigned char *) p_bin, BN_num_bytes(dsa->p)) > 0, |
36 | -1, "Failed to encode p_b64\n"); |
37 | |
38 | HIP_IFEL(!(BN_bn2bin(dsa->q, q_bin)), -1, |
39 | "Error in converting public exponent from BN to bin\n"); |
40 | - HIP_IFEL(!(q_b64 = base64_encode((unsigned char *) q_bin, |
41 | - BN_num_bytes(dsa->q))), |
42 | + HIP_IFEL(EVP_EncodeBlock(q_b64, (unsigned char *) q_bin, BN_num_bytes(dsa->q)) > 0, |
43 | -1, "Failed to encode q_64"); |
44 | |
45 | HIP_IFEL(!(BN_bn2bin(dsa->g, g_bin)), -1, |
46 | "Error in converting public exponent from BN to bin\n"); |
47 | - HIP_IFEL(!(g_b64 = base64_encode((unsigned char *) g_bin, |
48 | - BN_num_bytes(dsa->g))), |
49 | + HIP_IFEL(EVP_EncodeBlock(g_b64, (unsigned char *) g_bin, BN_num_bytes(dsa->g)) > 0, |
50 | -1, "Failed to encode g_b64\n"); |
51 | |
52 | HIP_IFEL(!(BN_bn2bin(dsa->pub_key, y_bin)), -1, |
53 | "Error in converting public exponent from BN to bin\n"); |
54 | - HIP_IFEL(!(y_b64 = base64_encode((unsigned char *) y_bin, |
55 | - BN_num_bytes(dsa->pub_key))), |
56 | + HIP_IFEL(EVP_EncodeBlock(y_b64, (unsigned char *) y_bin, BN_num_bytes(dsa->pub_key)) > 0, |
57 | -1, "Failed to encode y_b64\n"); |
58 | |
59 | sprintf(cert->public_key, "(public_key (dsa-pkcs1-sha1 (p |%s|)(q |%s|)" |
60 | |
61 | === modified file 'hipd/hidb.c' |
62 | --- hipd/hidb.c 2010-10-15 15:29:14 +0000 |
63 | +++ hipd/hidb.c 2010-10-20 08:51:06 +0000 |
64 | @@ -355,7 +355,7 @@ |
65 | |
66 | list_for_each(item, hip_local_hostid_db, c) { |
67 | id_entry = (struct hip_host_id_entry *) list_entry(item); |
68 | - if (hip_hit_are_equal(&id_entry->lhi.hit, our)) { |
69 | + if (memcmp(&id_entry->lhi.hit, our, sizeof(*our)) == 0) { |
70 | memcpy(our_lsi, &id_entry->lsi, sizeof(hip_lsi_t)); |
71 | return 0; |
72 | } |
73 | |
74 | === modified file 'lib/core/conf.c' |
75 | --- lib/core/conf.c 2010-10-20 03:38:26 +0000 |
76 | +++ lib/core/conf.c 2010-10-20 08:51:06 +0000 |
77 | @@ -872,7 +872,6 @@ |
78 | int index_of_hit = 0, index_of_ip = 0, opp_mode = 0;; |
79 | uint8_t lifetime = 0, *reg_types = NULL; |
80 | time_t seconds_from_lifetime = 0; |
81 | - char lowercase[30]; |
82 | |
83 | memset(&hit, 0, sizeof(hit)); |
84 | memset(&ipv6, 0, sizeof(ipv6)); |
85 | @@ -883,6 +882,8 @@ |
86 | err = -1; |
87 | goto out_err; |
88 | } else if (action == ACTION_ADD) { |
89 | + char *tail_ptr = NULL; |
90 | + |
91 | if (optc < 4) { |
92 | if (optc < 3) { |
93 | HIP_ERROR("Missing arguments.\n"); |
94 | @@ -903,20 +904,12 @@ |
95 | index_of_ip = optc - 2; |
96 | } |
97 | |
98 | - HIP_IFEL(hip_string_is_digit(opt[optc - 1]), -1, |
99 | + seconds = strtoul(opt[optc - 1], &tail_ptr, 10); |
100 | + HIP_IFEL(*tail_ptr == '\0' && seconds > 0 && seconds <= 15384774, -1, |
101 | "Invalid lifetime value \"%s\" given.\n" \ |
102 | "Please give a lifetime value between 1 and " \ |
103 | "15384774 seconds.\n", opt[optc - 1]); |
104 | |
105 | - seconds = atoi(opt[optc - 1]); |
106 | - |
107 | - if (seconds <= 0 || seconds > 15384774) { |
108 | - HIP_ERROR("Invalid lifetime value \"%s\" given.\n" \ |
109 | - "Please give a lifetime value between 1 and " \ |
110 | - "15384774 seconds.\n", opt[optc - 1]); |
111 | - goto out_err; |
112 | - } |
113 | - |
114 | HIP_IFEL(hip_get_lifetime_value(seconds, &lifetime), -1, |
115 | "Unable to convert seconds to a lifetime value.\n"); |
116 | |
117 | @@ -994,19 +987,18 @@ |
118 | goto out_err; |
119 | } |
120 | |
121 | - hip_string_to_lowercase(lowercase, opt[i], strlen(opt[i]) + 1); |
122 | - if (strcmp("rvs", lowercase) == 0) { |
123 | + if (strcasecmp("rvs", opt[i]) == 0) { |
124 | reg_types[i] = HIP_SERVICE_RENDEZVOUS; |
125 | - } else if (strcmp("relay", lowercase) == 0) { |
126 | + } else if (strcasecmp("relay", opt[i]) == 0) { |
127 | reg_types[i] = HIP_SERVICE_RELAY; |
128 | - } else if (strcmp("full-relay", lowercase) == 0) { |
129 | + } else if (strcasecmp("full-relay", opt[i]) == 0) { |
130 | reg_types[i] = HIP_SERVICE_FULLRELAY; |
131 | } /* To cope with the atoi() error value we handle the 'zero' |
132 | * case here. */ |
133 | - else if (strcmp("0", lowercase) == 0) { |
134 | + else if (strcasecmp("0", opt[i]) == 0) { |
135 | reg_types[i] = 0; |
136 | } else { |
137 | - reg_type = atoi(lowercase); |
138 | + reg_type = atoi(opt[i]); |
139 | if (reg_type <= 0 || reg_type > 255) { |
140 | HIP_ERROR("'%s' is not a valid service name " \ |
141 | "or service number.\n", opt[i]); |
142 | @@ -1207,13 +1199,13 @@ |
143 | |
144 | HIP_IFEL((optc != 2 && optc != 3), -1, "Missing arguments\n"); |
145 | |
146 | - HIP_IFEL(convert_string_to_address(opt[0], &hit), -1, |
147 | - "string to address conversion failed\n"); |
148 | - |
149 | - HIP_IFEL((err = convert_string_to_address(opt[1], &ip6)), -1, |
150 | - "string to address conversion failed\n"); |
151 | - |
152 | - if ((err && !convert_string_to_address_v4(opt[1], &aux))) { |
153 | + HIP_IFEL(hip_convert_string_to_address(opt[0], &hit), -1, |
154 | + "string to address conversion failed\n"); |
155 | + |
156 | + HIP_IFEL((err = hip_convert_string_to_address(opt[1], &ip6)), -1, |
157 | + "string to address conversion failed\n"); |
158 | + |
159 | + if ((err && inet_pton(AF_INET, opt[1], &aux) != 1)) { |
160 | HIP_IFEL(IS_LSI32(aux.s_addr), -1, "Missing ip address before lsi\n"); |
161 | } |
162 | |
163 | @@ -1243,7 +1235,7 @@ |
164 | "build param hit failed\n"); |
165 | |
166 | if (optc == 3) { |
167 | - HIP_IFEL(convert_string_to_address_v4(opt[2], &lsi), -1, |
168 | + HIP_IFEL(inet_pton(AF_INET, opt[2], &lsi) == 1, -1, |
169 | "string to address conversion failed\n"); |
170 | HIP_IFEL(!IS_LSI32(lsi.s_addr), -1, "Wrong LSI value\n"); |
171 | HIP_IFEL(hip_build_param_contents(msg, &lsi, |
172 | @@ -2011,7 +2003,7 @@ |
173 | if (!strcmp("all", opt[0])) { |
174 | hip_conf_print_info_ha(ha); |
175 | } else { |
176 | - HIP_IFE(convert_string_to_address(opt[0], &hit1), -1); |
177 | + HIP_IFE(hip_convert_string_to_address(opt[0], &hit1), -1); |
178 | |
179 | if ((ipv6_addr_cmp(&hit1, &ha->hit_our) == 0) || |
180 | (ipv6_addr_cmp(&hit1, &ha->hit_peer) == 0)) |
181 | |
182 | === modified file 'lib/core/hit.c' |
183 | --- lib/core/hit.c 2010-10-18 17:44:31 +0000 |
184 | +++ lib/core/hit.c 2010-10-20 08:51:06 +0000 |
185 | @@ -30,83 +30,56 @@ |
186 | * @author Miika Komu <miika@iki.fi> |
187 | */ |
188 | |
189 | -#include <stdint.h> |
190 | -#include <string.h> |
191 | +#include <string.h> // strcpy() |
192 | |
193 | -#include "config.h" |
194 | -#include "builder.h" |
195 | -#include "debug.h" |
196 | -#include "prefix.h" |
197 | -#include "protodefs.h" |
198 | -#include "straddr.h" |
199 | +#include "debug.h" // HIP_ASSERT() |
200 | +#include "prefix.h" // ipv6_addr_cmp() |
201 | +#include "straddr.h" // hip_in6_ntop() |
202 | #include "hit.h" |
203 | |
204 | /** |
205 | - * convert a binary HIT into a string |
206 | + * Convert a binary HIT to a hexadecimal string representation of the form |
207 | + * 0011:2233:4455:6677:8899:AABB:CCDD:EEFF terminated by a null character. |
208 | * |
209 | - * @param hit a binary HIT |
210 | - * @param prefix an optional HIT prefix as a string |
211 | - * @param hit_str the HIT as a string with the given prefix |
212 | - * @return zero on success and negative on error |
213 | + * @param hit a pointer to a binary HIT. |
214 | + * @param suffix an optional null-terminated string suffix to be appended to |
215 | + * the HIT. If suffix is NULL or the empty string, no suffix is appended. If |
216 | + * suffix is not null-terminated, the result is undefined. |
217 | + * @param hit_str a pointer to a buffer to write the HIT and the suffix to. The |
218 | + * result of passing a buffer that is too short to hold the string |
219 | + * representation plus the suffix is undefined. |
220 | + * @return 0 if the HIT was successfully converted. Returns a negative value if |
221 | + * hit is NULL or hit_str is NULL. |
222 | */ |
223 | -int hip_convert_hit_to_str(const hip_hit_t *hit, |
224 | - const char *prefix, |
225 | - char *hit_str) |
226 | +int hip_convert_hit_to_str(const hip_hit_t *const hit, |
227 | + const char *const suffix, |
228 | + char *const hit_str) |
229 | { |
230 | - int err = 0; |
231 | - |
232 | - HIP_ASSERT(hit); |
233 | - |
234 | - memset(hit_str, 0, INET6_ADDRSTRLEN); |
235 | - err = !hip_in6_ntop(hit, hit_str); |
236 | - |
237 | - if (prefix) { |
238 | - memcpy(hit_str + strlen(hit_str), prefix, strlen(prefix)); |
239 | + if (hit && hit_str) { |
240 | + if (hip_in6_ntop(hit, hit_str)) { |
241 | + if (suffix && *suffix != '\0') { |
242 | + strcpy(hit_str + strlen(hit_str), suffix); |
243 | + } |
244 | + return 0; |
245 | + } |
246 | } |
247 | |
248 | - return err; |
249 | -} |
250 | -/** |
251 | - * compare two HITs to check which HIT is "bigger" |
252 | - * |
253 | - * @param hit1 the first HIT to be compared |
254 | - * @param hit2 the second HIT to be compared |
255 | - * |
256 | - * @return 1 if hit1 was bigger than hit2, or else 0 |
257 | - */ |
258 | -int hip_hit_is_bigger(const struct in6_addr *hit1, |
259 | - const struct in6_addr *hit2) |
260 | -{ |
261 | - return ipv6_addr_cmp(hit1, hit2) > 0; |
262 | -} |
263 | - |
264 | -/** |
265 | - * compare two HITs to check which if they are equal |
266 | - * |
267 | - * @param hit1 the first HIT to be compared |
268 | - * @param hit2 the second HIT to be compared |
269 | - * |
270 | - * @return 1 if the HITs were equal and zero otherwise |
271 | - */ |
272 | -int hip_hit_are_equal(const struct in6_addr *hit1, |
273 | - const struct in6_addr *hit2) |
274 | -{ |
275 | - return ipv6_addr_cmp(hit1, hit2) == 0; |
276 | -} |
277 | - |
278 | -/** |
279 | - * calculate a hash from a HIT |
280 | - * |
281 | - * @param ptr pointer to a HIT |
282 | - * |
283 | - * Returns value in range: 0 <= x < range |
284 | - */ |
285 | -unsigned long hip_hash_hit(const void *ptr) |
286 | -{ |
287 | - uint8_t hash[HIP_AH_SHA_LEN]; |
288 | - |
289 | - hip_build_digest(HIP_DIGEST_SHA1, (const uint8_t *)ptr + sizeof(uint16_t), |
290 | - 7 * sizeof(uint16_t), hash); |
291 | - |
292 | - return *((unsigned long *) hash); |
293 | + return -1; |
294 | +} |
295 | + |
296 | +/** |
297 | + * Determine whether a HIT is numerically greater than another. |
298 | + * |
299 | + * @param hit_gt a pointer to a HIT. When passing a NULL pointer, the result |
300 | + * of this function is undefined. |
301 | + * @param hit_le a pointer to a HIT. When passing a NULL pointer, the result |
302 | + * of this function is undefined. |
303 | + * @return 1 if hit_gt is greater than hit_le, otherwise 0. |
304 | + */ |
305 | +int hip_hit_is_bigger(const struct in6_addr *const hit_gt, |
306 | + const struct in6_addr *const hit_le) |
307 | +{ |
308 | + HIP_ASSERT(hit_gt); |
309 | + HIP_ASSERT(hit_le); |
310 | + return ipv6_addr_cmp(hit_gt, hit_le) > 0; |
311 | } |
312 | |
313 | === modified file 'lib/core/hit.h' |
314 | --- lib/core/hit.h 2010-10-15 15:29:14 +0000 |
315 | +++ lib/core/hit.h 2010-10-20 08:51:06 +0000 |
316 | @@ -30,11 +30,8 @@ |
317 | |
318 | #include "protodefs.h" |
319 | |
320 | -int hip_convert_hit_to_str(const hip_hit_t *hit, const char *prefix, char *str); |
321 | -int hip_hit_is_bigger(const struct in6_addr *hit1, |
322 | - const struct in6_addr *hit2); |
323 | -int hip_hit_are_equal(const struct in6_addr *hit1, |
324 | - const struct in6_addr *hit2); |
325 | -unsigned long hip_hash_hit(const void *hit); |
326 | +int hip_convert_hit_to_str(const hip_hit_t *const hit, const char *const suffix, char *const str); |
327 | +int hip_hit_is_bigger(const struct in6_addr *const hit_gt, |
328 | + const struct in6_addr *const hit_le); |
329 | |
330 | #endif /* HIP_LIB_CORE_HIT_H */ |
331 | |
332 | === modified file 'lib/core/straddr.c' |
333 | --- lib/core/straddr.c 2010-10-18 17:44:31 +0000 |
334 | +++ lib/core/straddr.c 2010-10-20 08:51:06 +0000 |
335 | @@ -32,30 +32,28 @@ |
336 | |
337 | #define _BSD_SOURCE |
338 | |
339 | -#include <ctype.h> |
340 | -#include <errno.h> |
341 | -#include <stdio.h> |
342 | -#include <stdlib.h> |
343 | -#include <arpa/inet.h> |
344 | -#include <netinet/in.h> |
345 | -#include <openssl/evp.h> |
346 | +#include <stdio.h> // sprintf() |
347 | +#include <arpa/inet.h> // inet_pton() |
348 | |
349 | -#include "config.h" |
350 | -#include "debug.h" |
351 | -#include "ife.h" |
352 | -#include "prefix.h" |
353 | +#include "debug.h" // HIP_DEBUG() |
354 | +#include "prefix.h" // IPV4_TO_IPV6_MAP() |
355 | #include "straddr.h" |
356 | |
357 | /** |
358 | - * convert a binary IPv6 address to a string |
359 | + * Convert a binary IPv6 address to a hexadecimal string representation of the |
360 | + * form 0011:2233:4455:6677:8899:AABB:CCDD:EEFF terminated by a null character. |
361 | * |
362 | - * @param in6 the IPv6 address to convert |
363 | - * @param buf a preallocated buffer where the string will be stored |
364 | - * @return a pointer to the buf |
365 | + * @param in6 a pointer to a binary IPv6 address. |
366 | + * @param buf a pointer to a buffer to write the string representation to. The |
367 | + * result of passing a buffer that is too short to hold the string |
368 | + * representation is undefined. |
369 | + * @return The function returns a pointer to the output buffer buf if the |
370 | + * address is successfully converted. It returns a negative value if in6 is |
371 | + * NULL or buf is NULL. |
372 | */ |
373 | -char *hip_in6_ntop(const struct in6_addr *in6, char *buf) |
374 | +char *hip_in6_ntop(const struct in6_addr *const in6, char *const buf) |
375 | { |
376 | - if (!buf) { |
377 | + if (!in6 || !buf) { |
378 | return NULL; |
379 | } |
380 | sprintf(buf, |
381 | @@ -68,137 +66,37 @@ |
382 | } |
383 | |
384 | /** |
385 | - * convert a string into a binary IPv4 address (a wrapper for inet_pton()) |
386 | - * |
387 | - * @param str the string to convert |
388 | - * @param ip an output argument that will contain a binary IPv4 calculated |
389 | - * from the @c str |
390 | - * @return zero on success and negative on error |
391 | - */ |
392 | -int convert_string_to_address_v4(const char *str, struct in_addr *ip) |
393 | -{ |
394 | - int ret = 0, err = 0; |
395 | - |
396 | - ret = inet_pton(AF_INET, str, ip); |
397 | - HIP_IFEL((ret < 0 && errno == EAFNOSUPPORT), -1, |
398 | - "inet_pton: not a valid address family\n"); |
399 | - HIP_IFEL((ret == 0), -1, |
400 | - "inet_pton: %s: not a valid network address\n", str); |
401 | -out_err: |
402 | - return err; |
403 | -} |
404 | - |
405 | -/** |
406 | - * Convert a string to an IPv6 address. This function can handle |
407 | - * also IPv6 mapped addresses. |
408 | - * |
409 | - * @param str the string to convert |
410 | - * @param ip6 An output argument that will contain a binary IPv4 calculated |
411 | - * from the @c str. Possibly in IPv6 mapped format. |
412 | - * @return zero on success or negative on error |
413 | - */ |
414 | -int convert_string_to_address(const char *str, |
415 | - struct in6_addr *ip6) |
416 | -{ |
417 | - int ret = 0, err = 0; |
418 | - struct in_addr ip4; |
419 | - |
420 | - ret = inet_pton(AF_INET6, str, ip6); |
421 | - HIP_IFEL((ret < 0 && errno == EAFNOSUPPORT), -1, |
422 | - "\"%s\" is not of valid address family.\n", str); |
423 | - if (ret > 0) { |
424 | - /* IPv6 address conversion was ok */ |
425 | - goto out_err; |
426 | - } |
427 | - |
428 | - /* Might be an ipv4 address (ret == 0). Lets catch it here. */ |
429 | - err = convert_string_to_address_v4(str, &ip4); |
430 | - if (err) { |
431 | - goto out_err; |
432 | - } |
433 | - |
434 | - IPV4_TO_IPV6_MAP(&ip4, ip6); |
435 | - HIP_DEBUG("Mapped v4 to v6.\n"); |
436 | - HIP_DEBUG_IN6ADDR("mapped v6", ip6); |
437 | - |
438 | -out_err: |
439 | - return err; |
440 | -} |
441 | - |
442 | -/** |
443 | - * convert a string containing upper case characters to lower case |
444 | - * |
445 | - * @param to the result of the conversion (minimum length @c count) |
446 | - * @param from a string possibly containing upper case characters |
447 | - * @param count count |
448 | - * @return zero on success or negative on failure |
449 | - */ |
450 | -int hip_string_to_lowercase(char *to, const char *from, const size_t count) |
451 | -{ |
452 | - unsigned i; |
453 | - |
454 | - if (to == NULL || from == NULL || count == 0) { |
455 | - return -1; |
456 | - } |
457 | - |
458 | - for (i = 0; i < count; i++) { |
459 | - if (isalpha(from[i])) { |
460 | - to[i] = tolower(from[i]); |
461 | + * Convert a string representation of an IPv6 or IPv4 address to a struct |
462 | + * in6_addr. |
463 | + * If the string contains an IPv4 address, it is converted to its |
464 | + * IPv6-compatible mapping. |
465 | + * |
466 | + * @param str points to the string to convert. |
467 | + * @param ip6 points to a buffer where the function stores the binary address |
468 | + * if it could be converted. |
469 | + * @return The return value is 0 if the conversion succeeds. It is a |
470 | + * negative value if str or ip6 are NULL or if str contains neither a |
471 | + * parseable IPv6 or IPv4 address. |
472 | + */ |
473 | +int hip_convert_string_to_address(const char *const str, |
474 | + struct in6_addr *const ip6) |
475 | +{ |
476 | + if (str && ip6) { |
477 | + if (inet_pton(AF_INET6, str, ip6) == 1) { |
478 | + /* IPv6 address conversion was ok */ |
479 | + return 0; |
480 | } else { |
481 | - to[i] = from[i]; |
482 | - } |
483 | - } |
484 | - return 0; |
485 | -} |
486 | - |
487 | -/** |
488 | - * test if a given string contains a positive integer |
489 | - * |
490 | - * @param string the string to test |
491 | - * @return zero if the string is digit or negative otherwise |
492 | - */ |
493 | -int hip_string_is_digit(const char *string) |
494 | -{ |
495 | - if (string == NULL) { |
496 | - return -1; |
497 | - } |
498 | - |
499 | - int i = 0; |
500 | - |
501 | - while (string[i] != '\0') { |
502 | - if (!isdigit(string[i])) { |
503 | - return -1; |
504 | - } |
505 | - i++; |
506 | - } |
507 | - return 0; |
508 | -} |
509 | - |
510 | - |
511 | -/** |
512 | - * encode the given content to Base64 |
513 | - * |
514 | - * @param buf Pointer to contents to be encoded |
515 | - * @param len How long is the first parameter in bytes |
516 | - * |
517 | - * @return Returns a pointer to encoded content or NULL on error |
518 | - */ |
519 | -unsigned char *base64_encode(unsigned char *buf, unsigned int len) |
520 | -{ |
521 | - unsigned char *ret; |
522 | - unsigned int b64_len; |
523 | - |
524 | - b64_len = (((len + 2) / 3) * 4) + 1; |
525 | - ret = malloc(b64_len); |
526 | - if (ret == NULL) { |
527 | - goto out_err; |
528 | - } |
529 | - EVP_EncodeBlock(ret, buf, len); |
530 | - return ret; |
531 | -out_err: |
532 | - if (ret) { |
533 | - free(ret); |
534 | - } |
535 | - return NULL; |
536 | -} |
537 | - |
538 | + struct in_addr ip4; |
539 | + |
540 | + /* Might be an ipv4 address (ret == 0). Lets catch it here. */ |
541 | + if (inet_pton(AF_INET, str, &ip4) == 1) { |
542 | + IPV4_TO_IPV6_MAP(&ip4, ip6); |
543 | + HIP_DEBUG("Mapped v4 to v6.\n"); |
544 | + HIP_DEBUG_IN6ADDR("mapped v6", ip6); |
545 | + return 0; |
546 | + } |
547 | + } |
548 | + } |
549 | + |
550 | + return -1; |
551 | +} |
552 | |
553 | === modified file 'lib/core/straddr.h' |
554 | --- lib/core/straddr.h 2010-10-15 15:29:14 +0000 |
555 | +++ lib/core/straddr.h 2010-10-20 08:51:06 +0000 |
556 | @@ -29,11 +29,8 @@ |
557 | #include <sys/types.h> |
558 | #include <netinet/in.h> |
559 | |
560 | -int convert_string_to_address_v4(const char *str, struct in_addr *ip); |
561 | -int convert_string_to_address(const char *str, struct in6_addr *ip6); |
562 | -char *hip_in6_ntop(const struct in6_addr *in6, char *buf); |
563 | -int hip_string_to_lowercase(char *to, const char *from, const size_t count); |
564 | -int hip_string_is_digit(const char *string); |
565 | -unsigned char *base64_encode(unsigned char *, unsigned int); |
566 | +char *hip_in6_ntop(const struct in6_addr *const in6, char *const buf); |
567 | +int hip_convert_string_to_address(const char *const str, |
568 | + struct in6_addr *const ip6); |
569 | |
570 | #endif /* HIP_LIB_CORE_STRADDR_H */ |
571 | |
572 | === modified file 'test/lib/core/hit.c' |
573 | --- test/lib/core/hit.c 2010-10-20 08:51:06 +0000 |
574 | +++ test/lib/core/hit.c 2010-10-20 08:51:06 +0000 |
575 | @@ -44,18 +44,18 @@ |
576 | START_TEST(test_hip_convert_hit_to_str_null_hit) |
577 | { |
578 | char buf[64]; |
579 | - hip_convert_hit_to_str(NULL, "", buf); |
580 | + fail_unless(hip_convert_hit_to_str(NULL, "", buf) < 0, NULL); |
581 | } |
582 | END_TEST |
583 | |
584 | START_TEST(test_hip_convert_hit_to_str_null_buf) |
585 | { |
586 | hip_hit_t hit; |
587 | - fail_unless(hip_convert_hit_to_str(&hit, "", NULL) == 1, NULL); |
588 | + fail_unless(hip_convert_hit_to_str(&hit, "", NULL) < 0, NULL); |
589 | } |
590 | END_TEST |
591 | |
592 | -START_TEST(test_hip_convert_hit_to_str_null_prefix) |
593 | +START_TEST(test_hip_convert_hit_to_str_null_suffix) |
594 | { |
595 | char buf[64]; |
596 | hip_hit_t hit; |
597 | @@ -70,9 +70,12 @@ |
598 | const unsigned int HIT_LEN = 39; // 16 bytes -> 32 hex chars + 7 ':'s |
599 | const unsigned int SUFFIX_LEN = sizeof(suffix); // includes null char |
600 | const unsigned int AFTER_LEN = 30; |
601 | - char buf[BEFORE_LEN + HIT_LEN + SUFFIX_LEN + AFTER_LEN] = { 1 }; |
602 | - char ones[BEFORE_LEN + HIT_LEN + SUFFIX_LEN + AFTER_LEN] = { 1 }; |
603 | + char buf[BEFORE_LEN + HIT_LEN + SUFFIX_LEN + AFTER_LEN]; |
604 | + char ones[BEFORE_LEN + HIT_LEN + SUFFIX_LEN + AFTER_LEN]; |
605 | hip_hit_t hit; |
606 | + |
607 | + memset(buf, '1', sizeof(buf)); |
608 | + memset(ones, '1', sizeof(ones)); |
609 | memset(&hit.s6_addr, 0x22, sizeof(hit.s6_addr)); |
610 | |
611 | // write the HIT string into the middle of the buffer |
612 | @@ -107,6 +110,20 @@ |
613 | } |
614 | END_TEST |
615 | |
616 | +START_TEST(test_hip_hit_is_bigger_null_first) |
617 | +{ |
618 | + const hip_hit_t hit = IN6ADDR_LOOPBACK_INIT; |
619 | + hip_hit_is_bigger(NULL, &hit); |
620 | +} |
621 | +END_TEST |
622 | + |
623 | +START_TEST(test_hip_hit_is_bigger_null_second) |
624 | +{ |
625 | + const hip_hit_t hit = IN6ADDR_LOOPBACK_INIT; |
626 | + hip_hit_is_bigger(&hit, NULL); |
627 | +} |
628 | +END_TEST |
629 | + |
630 | START_TEST(test_hip_hit_is_bigger_first_null) |
631 | { |
632 | hip_hit_t hit; |
633 | @@ -121,49 +138,6 @@ |
634 | } |
635 | END_TEST |
636 | |
637 | -START_TEST(test_hip_hit_are_equal_equality) |
638 | -{ |
639 | - const hip_hit_t hit1 = IN6ADDR_LOOPBACK_INIT; |
640 | - const hip_hit_t hit2 = IN6ADDR_LOOPBACK_INIT; |
641 | - fail_unless(hip_hit_are_equal(&hit1, &hit2) == 1, NULL); |
642 | -} |
643 | -END_TEST |
644 | - |
645 | -START_TEST(test_hip_hit_are_equal_inequality) |
646 | -{ |
647 | - const hip_hit_t bigger = IN6ADDR_LOOPBACK_INIT; |
648 | - const hip_hit_t smaller = IN6ADDR_ANY_INIT; |
649 | - fail_unless(hip_hit_are_equal(&bigger, &smaller) == 1, NULL); |
650 | -} |
651 | -END_TEST |
652 | - |
653 | -START_TEST(test_hip_hit_are_equal_first_null) |
654 | -{ |
655 | - hip_hit_t hit; |
656 | - hip_hit_are_equal(NULL, &hit); |
657 | -} |
658 | -END_TEST |
659 | - |
660 | -START_TEST(test_hip_hit_are_equal_second_null) |
661 | -{ |
662 | - hip_hit_t hit; |
663 | - hip_hit_are_equal(&hit, NULL); |
664 | -} |
665 | -END_TEST |
666 | - |
667 | -START_TEST(test_hip_hash_hit_valid) |
668 | -{ |
669 | - const hip_hit_t hit = IN6ADDR_ANY_INIT; |
670 | - hip_hash_hit(&hit); |
671 | -} |
672 | -END_TEST |
673 | - |
674 | -START_TEST(test_hip_hash_hit_null) |
675 | -{ |
676 | - hip_hash_hit(NULL); |
677 | -} |
678 | -END_TEST |
679 | - |
680 | // For unknown reasons, this file does not compile with the following, |
681 | // seemingly useless forward declaration |
682 | Suite *lib_core_hit(void); |
683 | @@ -174,20 +148,16 @@ |
684 | |
685 | TCase *tc_core = tcase_create("Core"); |
686 | tcase_add_test(tc_core, test_hip_convert_hit_to_str_valid); |
687 | - tcase_add_exit_test(tc_core, test_hip_convert_hit_to_str_null_hit, 1); |
688 | + tcase_add_test(tc_core, test_hip_convert_hit_to_str_null_hit); |
689 | tcase_add_test(tc_core, test_hip_convert_hit_to_str_null_buf); |
690 | - tcase_add_test(tc_core, test_hip_convert_hit_to_str_null_prefix); |
691 | + tcase_add_test(tc_core, test_hip_convert_hit_to_str_null_suffix); |
692 | tcase_add_test(tc_core, test_hip_convert_hit_to_str_bounds); |
693 | tcase_add_test(tc_core, test_hip_hit_is_bigger_bigger); |
694 | tcase_add_test(tc_core, test_hip_hit_is_bigger_equal_smaller); |
695 | + tcase_add_exit_test(tc_core, test_hip_hit_is_bigger_null_first, 1); |
696 | + tcase_add_exit_test(tc_core, test_hip_hit_is_bigger_null_second, 1); |
697 | tcase_add_exit_test(tc_core, test_hip_hit_is_bigger_first_null, 1); |
698 | tcase_add_exit_test(tc_core, test_hip_hit_is_bigger_second_null, 1); |
699 | - tcase_add_test(tc_core, test_hip_hit_are_equal_equality); |
700 | - tcase_add_test(tc_core, test_hip_hit_are_equal_inequality); |
701 | - tcase_add_exit_test(tc_core, test_hip_hit_are_equal_first_null, 1); |
702 | - tcase_add_exit_test(tc_core, test_hip_hit_are_equal_second_null, 1); |
703 | - tcase_add_test(tc_core, test_hip_hash_hit_valid); |
704 | - tcase_add_exit_test(tc_core, test_hip_hash_hit_null, 1); |
705 | suite_add_tcase(s, tc_core); |
706 | |
707 | return s; |
708 | |
709 | === modified file 'test/lib/core/straddr.c' |
710 | --- test/lib/core/straddr.c 2010-10-20 08:51:06 +0000 |
711 | +++ test/lib/core/straddr.c 2010-10-20 08:51:06 +0000 |
712 | @@ -31,137 +31,83 @@ |
713 | #include <stdlib.h> // free() |
714 | #include "lib/core/straddr.h" |
715 | |
716 | -START_TEST(test_convert_string_to_address_v4_valid) |
717 | -{ |
718 | - const char *str = "127.0.0.1"; |
719 | - struct in_addr ip; |
720 | - |
721 | - fail_unless(convert_string_to_address_v4(str, &ip) == 0, NULL); |
722 | -} |
723 | -END_TEST |
724 | - |
725 | -START_TEST(test_convert_string_to_address_v4_null_str) |
726 | -{ |
727 | - struct in_addr ip; |
728 | - |
729 | - fail_unless(convert_string_to_address_v4(NULL, &ip) < 0, NULL); |
730 | -} |
731 | -END_TEST |
732 | - |
733 | -START_TEST(test_convert_string_to_address_v4_null_addr) |
734 | -{ |
735 | - const char *str = "127.0.0.1"; |
736 | - |
737 | - fail_unless(convert_string_to_address_v4(str, NULL) < 0, NULL); |
738 | -} |
739 | -END_TEST |
740 | - |
741 | -START_TEST(test_convert_string_to_address_v4_invalid) |
742 | -{ |
743 | - const char *str = " 127.0.0.1"; |
744 | - struct in_addr ip; |
745 | - |
746 | - fail_unless(convert_string_to_address_v4(str, &ip) < 0, NULL); |
747 | -} |
748 | -END_TEST |
749 | - |
750 | -START_TEST(test_convert_string_to_address_valid) |
751 | -{ |
752 | - const char *str = "fe80::215:58ff:fe29:9c36"; |
753 | - struct in6_addr ip; |
754 | - |
755 | - fail_unless(convert_string_to_address(str, &ip) == 0, NULL); |
756 | -} |
757 | -END_TEST |
758 | - |
759 | -START_TEST(test_convert_string_to_address_null_str) |
760 | -{ |
761 | - struct in6_addr ip; |
762 | - |
763 | - fail_unless(convert_string_to_address(NULL, &ip) < 0, NULL); |
764 | -} |
765 | -END_TEST |
766 | - |
767 | -START_TEST(test_convert_string_to_address_null_addr) |
768 | -{ |
769 | - const char *str = "fe80::215:58ff:fe29:9c36"; |
770 | - |
771 | - fail_unless(convert_string_to_address(str, NULL) < 0, NULL); |
772 | -} |
773 | -END_TEST |
774 | - |
775 | -START_TEST(test_convert_string_to_address_invalid) |
776 | +START_TEST(test_hip_in6_ntop_valid) |
777 | +{ |
778 | + const int GUARD_SIZE = 32; // arbitrary |
779 | + struct buf_test { |
780 | + char before[GUARD_SIZE]; |
781 | + char addr[39]; // 16 IPv6 bytes -> 32 hex chars + 7 ':'s |
782 | + char null[1]; // terminating null character |
783 | + char after[GUARD_SIZE]; |
784 | + } buf; |
785 | + char ones[GUARD_SIZE]; |
786 | + struct in6_addr in6; |
787 | + |
788 | + memset(&buf, '1', sizeof(buf)); |
789 | + memset(ones, '1', sizeof(ones)); |
790 | + memset(&in6.s6_addr, 0x22, sizeof(in6.s6_addr)); |
791 | + |
792 | + fail_unless(hip_in6_ntop(&in6, buf.addr) == buf.addr, NULL); |
793 | + // is the buffer before the address untouched? |
794 | + fail_unless(memcmp(buf.before, ones, GUARD_SIZE) == 0, NULL); |
795 | + // is the first part of the address correct? |
796 | + fail_unless(buf.addr[0] == '2', NULL); |
797 | + // is the last part of the address correct? |
798 | + fail_unless(buf.addr[sizeof(buf.addr) - 1] == '2', NULL); |
799 | + // is there a terminating null character? |
800 | + fail_unless(buf.null[0] == '\0', NULL); |
801 | + // is the buffer after the address untouched? |
802 | + fail_unless(memcmp(buf.after, ones, GUARD_SIZE) == 0, NULL); |
803 | +} |
804 | +END_TEST |
805 | + |
806 | +START_TEST(test_hip_in6_ntop_null_addr) |
807 | +{ |
808 | + char buf[64]; |
809 | + |
810 | + fail_unless(hip_in6_ntop(NULL, buf) == NULL, NULL); |
811 | +} |
812 | +END_TEST |
813 | + |
814 | +START_TEST(test_hip_in6_ntop_null_buf) |
815 | +{ |
816 | + struct in6_addr in6 = IN6ADDR_LOOPBACK_INIT; |
817 | + |
818 | + fail_unless(hip_in6_ntop(&in6, NULL) == NULL, NULL); |
819 | +} |
820 | +END_TEST |
821 | + |
822 | +START_TEST(test_hip_convert_string_to_address_valid) |
823 | +{ |
824 | + const char *str = "fe80::215:58ff:fe29:9c36"; |
825 | + struct in6_addr ip; |
826 | + |
827 | + fail_unless(hip_convert_string_to_address(str, &ip) == 0, NULL); |
828 | +} |
829 | +END_TEST |
830 | + |
831 | +START_TEST(test_hip_convert_string_to_address_null_str) |
832 | +{ |
833 | + struct in6_addr ip; |
834 | + |
835 | + fail_unless(hip_convert_string_to_address(NULL, &ip) < 0, NULL); |
836 | +} |
837 | +END_TEST |
838 | + |
839 | +START_TEST(test_hip_convert_string_to_address_null_addr) |
840 | +{ |
841 | + const char *str = "fe80::215:58ff:fe29:9c36"; |
842 | + |
843 | + fail_unless(hip_convert_string_to_address(str, NULL) < 0, NULL); |
844 | +} |
845 | +END_TEST |
846 | + |
847 | +START_TEST(test_hip_convert_string_to_address_invalid) |
848 | { |
849 | const char *str = " fe80::215:58ff:fe29:9c36"; |
850 | struct in6_addr ip; |
851 | |
852 | - fail_unless(convert_string_to_address(str, &ip) < 0, NULL); |
853 | -} |
854 | -END_TEST |
855 | - |
856 | -START_TEST(test_hip_string_to_lowercase_valid) |
857 | -{ |
858 | - char to[128] = { 1 }; |
859 | - char ones[128] = { 1 }; |
860 | - const char from[] = "TesT"; |
861 | - const size_t count = sizeof(from) - 1; |
862 | - const unsigned int offset = 32; |
863 | - |
864 | - fail_unless(hip_string_to_lowercase(to + offset, from, count) == 0, NULL); |
865 | - // was from correctly converted to lower case? |
866 | - fail_unless(memcmp(to + offset, "test", count) == 0, NULL); |
867 | - // is the beginning of to still intact? |
868 | - fail_unless(memcmp(to, ones, offset) == 0, NULL); |
869 | - // is the rest of to still intact? |
870 | - fail_unless(memcmp(to + offset + count, ones, offset) == 0, NULL); |
871 | -} |
872 | -END_TEST |
873 | - |
874 | -START_TEST(test_hip_string_is_digit_valid) |
875 | -{ |
876 | - fail_unless(hip_string_is_digit("123456789") == 0, NULL); |
877 | - fail_unless(hip_string_is_digit("abc") < 0, NULL); |
878 | -} |
879 | -END_TEST |
880 | - |
881 | -START_TEST(test_hip_string_is_digit_null) |
882 | -{ |
883 | - fail_unless(hip_string_is_digit(NULL) < 0, NULL); |
884 | -} |
885 | -END_TEST |
886 | - |
887 | -START_TEST(test_hip_string_is_digit_empty) |
888 | -{ |
889 | - fail_unless(hip_string_is_digit("") < 0, NULL); |
890 | -} |
891 | -END_TEST |
892 | - |
893 | -START_TEST(test_base64_encode_valid) |
894 | -{ |
895 | - const char b64[] = "VGVzdA=="; |
896 | - unsigned char buf[] = "Test"; |
897 | - unsigned int len = sizeof(buf) - 1; // do not include null character as per doc |
898 | - unsigned char *result = NULL; |
899 | - |
900 | - fail_unless((result = base64_encode(buf, len)) != NULL, NULL); |
901 | - fail_unless(strcmp((char*)result, b64) == 0, NULL); |
902 | - free(result); // note it's not documented that we need to free the returned memory |
903 | -} |
904 | -END_TEST |
905 | - |
906 | -START_TEST(test_base64_encode_null_buf) |
907 | -{ |
908 | - fail_unless(base64_encode(NULL, 42) == NULL, NULL); |
909 | -} |
910 | -END_TEST |
911 | - |
912 | -START_TEST(test_base64_encode_empty_buf) |
913 | -{ |
914 | - unsigned char buf[] = ""; |
915 | - unsigned char *result = NULL; |
916 | - |
917 | - fail_unless((result = base64_encode(buf, 0)) != NULL, NULL); |
918 | - fail_unless(strlen((char *)result) == 0, NULL); |
919 | + fail_unless(hip_convert_string_to_address(str, &ip) < 0, NULL); |
920 | } |
921 | END_TEST |
922 | |
923 | @@ -174,21 +120,13 @@ |
924 | Suite *s = suite_create("lib/core/straddr"); |
925 | |
926 | TCase *tc_core = tcase_create("Core"); |
927 | - tcase_add_test(tc_core, test_convert_string_to_address_v4_valid); |
928 | - tcase_add_test(tc_core, test_convert_string_to_address_v4_null_str); |
929 | - tcase_add_test(tc_core, test_convert_string_to_address_v4_null_addr); |
930 | - tcase_add_test(tc_core, test_convert_string_to_address_v4_invalid); |
931 | - tcase_add_test(tc_core, test_convert_string_to_address_valid); |
932 | - tcase_add_test(tc_core, test_convert_string_to_address_null_str); |
933 | - tcase_add_test(tc_core, test_convert_string_to_address_null_addr); |
934 | - tcase_add_test(tc_core, test_convert_string_to_address_invalid); |
935 | - tcase_add_test(tc_core, test_hip_string_to_lowercase_valid); |
936 | - tcase_add_test(tc_core, test_hip_string_is_digit_valid); |
937 | - tcase_add_test(tc_core, test_hip_string_is_digit_null); |
938 | - tcase_add_test(tc_core, test_hip_string_is_digit_empty); |
939 | - tcase_add_test(tc_core, test_base64_encode_valid); |
940 | - tcase_add_test(tc_core, test_base64_encode_null_buf); |
941 | - tcase_add_test(tc_core, test_base64_encode_empty_buf); |
942 | + tcase_add_test(tc_core, test_hip_in6_ntop_valid); |
943 | + tcase_add_test(tc_core, test_hip_in6_ntop_null_addr); |
944 | + tcase_add_test(tc_core, test_hip_in6_ntop_null_buf); |
945 | + tcase_add_test(tc_core, test_hip_convert_string_to_address_valid); |
946 | + tcase_add_test(tc_core, test_hip_convert_string_to_address_null_str); |
947 | + tcase_add_test(tc_core, test_hip_convert_string_to_address_null_addr); |
948 | + tcase_add_test(tc_core, test_hip_convert_string_to_address_invalid); |
949 | suite_add_tcase(s, tc_core); |
950 | |
951 | return s; |
Hi Diego!
Thanks for taking the time to review!
> This [the branch] looks like it could be split into two parts, one for the
> fixes and one for the unit tests...
Technically, it could. Logically however, the intention is to start the 'don't commit things that break the unit tests' policy.
>> --- lib/core/hit.c 2010-10-18 17:44:31 +0000
>> +++ lib/core/hit.c 2010-10-19 12:14:42 +0000
>> @@ -42,71 +42,48 @@
>>
>> + if (hit != NULL && hit_str != NULL) {
>> + if (hip_in6_ntop(hit, hit_str) != NULL) {
>
> I don't think the '!= NULL' expressions aid readability.
As opposed to writing "if (hit && hit_str)"? In that case I agree and I would change the code accordingly. Please let me know if it is a different alternative you are suggesting.
> In any case these two if clauses can be combined.
Technically, they could. Logically, there are two different checks here. First, I check my input (i.e. hit and hit_str), then I do the work (hip_in6_ntop()). In my eyes, these two steps are worth separating 1) for comprehensibility and 2) if changes become necessary. I concede that this is a purely philosophical point :)
>> --- test/lib/core/hit.c 2010-10-19 12:14:42 +0000
>> +++ test/lib/core/hit.c 2010-10-19 12:14:42 +0000
>
> This is a separate thing.
True. This is reflected by the fact that on this branch, one commit contains the changes in straddr.c and a different commit those in hit.c.
I hope these explanations adequately address your concerns.
Stefan