Code review from Wouter part 1

diff --git a/Changelog b/Changelog
index 778cb470b0c44ae926ae1539743212970da5a9b2..61711d3fd21f997af64eba265aab695edcf1685a 100644 (file)
--- a/Changelog
+++ b/Changelog
@@ -1,4 +1,6 @@
 1.6.14
+       * DANE support (RFC6698), including ldns-dane example tool.
+       * bugfix #473: Dead code removal and resource leak fix in drill
        * bugfix #471: Let ldns_resolver_push_dnssec_anchor accept DS RR's too.
        * Various bugfixes from code reviews from CZ.NIC and Paul Wouters
        * ldns-notify TSIG option argument checking
@@ -13,7 +15,7 @@
        * bugfix #459: Export only symbols defined in ldns_symbols
        * bugfix #458: Track all newly created signatures when signing.
        * bugfix #454: Only set -g and -O2 CFLAGS when no CFLAGS was given.
-       * bugfix #457: Memroy leak fix for ldns_key_new_frm_algorithm.
+       * bugfix #457: Memory leak fix for ldns_key_new_frm_algorithm.
        * pyldns memory handling fixes and the python3/ldns-signzone.py
          examples script contribution from Karel Slany.
        * bugfix #450: Base # bytes for P, G and Y (T) on the guaranteed
@@ -27,7 +29,6 @@
        * New -p option for ldns-read-zone to prepend-pad SOA serial to take
          up ten characters.
        * Return error if printing RR fails due to unknown/null RDATA.
-       * New TLSA support (draft-ietf-dane-protocol).
 
 1.6.13 2012-05-21
        * New -S option for ldns-verify-zone to chase signatures online.

diff --git a/dname.c b/dname.c
index 00b441d9f2c426a66e3384b139d2aa2ab770d5e0..55aba5d65a1317c64e5e4326b1c93b1534f134ad 100644 (file)
--- a/dname.c
+++ b/dname.c
@@ -30,8 +30,12 @@
 #include <arpa/inet.h>
 #endif
 
+/* Returns whether the last label in the name is a root label (a empty label).
+ * Note that it is not enough to just test the last character to be 0,
+ * because it may be part of the last label itself.
+ */
 static bool
-ldns_dname_has_root_label(const ldns_rdf* dname)
+ldns_dname_last_label_is_root_label(const ldns_rdf* dname)
 {
        size_t src_pos;
        size_t len = 0;
@@ -61,7 +65,7 @@ ldns_dname_cat_clone(const ldns_rdf *rd1, const ldns_rdf *rd2)
         * rd, by reducing the size with 1
         */
        left_size = ldns_rdf_size(rd1);
-       if (ldns_dname_has_root_label(rd1)) {
+       if (ldns_dname_last_label_is_root_label(rd1)) {
                left_size--;
        }
 
@@ -98,7 +102,7 @@ ldns_dname_cat(ldns_rdf *rd1, ldns_rdf *rd2)
         * rd, by reducing the size with 1
         */
        left_size = ldns_rdf_size(rd1);
-       if (ldns_dname_has_root_label(rd1)) {
+       if (ldns_dname_last_label_is_root_label(rd1)) {
                left_size--;
        }
 
@@ -140,7 +144,7 @@ ldns_dname_reverse(const ldns_rdf *dname)
        
        /* If dname ends in a root label, the reverse should too.
         */
-       if (ldns_dname_has_root_label(dname)) {
+       if (ldns_dname_last_label_is_root_label(dname)) {
                buf[rd_size - 1] = 0;
                rd_size -= 1;
        }

diff --git a/examples/ldns-dane.1 b/examples/ldns-dane.1
index 6b44ec7cd15c0b58077c084d9387deed53d6c7bf..c488ddad2925f58caca856c57626e6f1604caed6 100644 (file)
--- a/examples/ldns-dane.1
+++ b/examples/ldns-dane.1
@@ -30,22 +30,19 @@ ldns-dane \- verify or create TLS authentication with DANE (RFC6698)
 
 .SH DESCRIPTION
 
-In the first two forms ldns-dane will be in \fBverify\fR modus.
-In the third form in \fBcreate\fR modus.
-
 In the first form: 
-A TLS connection to \fIname\fR:\fIport\fR is made. The TLSA resource
-record(s) for \fIname\fR are used to authenticate the connection.
+A TLS connection to \fIname\fR:\fIport\fR is established.
+The TLSA resource record(s) for \fIname\fR are used to authenticate
+the connection.
 
 In the second form:
-The TLSA record(s) are read from \fItlsafile\fR and user to authenticate
+The TLSA record(s) are read from \fItlsafile\fR and used to authenticate
 the TLS service they reference.
 
 In the third form:
-A TLS connection to \fIname\fR:\fIport\fR is made and used to create the TLSA
-resource record(s) that would authenticate the connection.
-The parameters that determine the form of the TLSA records to be created
-are:
+A TLS connection to \fIname\fR:\fIport\fR is established and used to
+create the TLSA resource record(s) that would authenticate the connection.
+The parameters for TLSA rr creation are:
 
 .PD 0
 .I Certificate-usage\fR:
@@ -79,8 +76,8 @@ SHA-512
 .RE
 .PD 1
 
-In case of numbers the first few letters of the intended meaning may be used.
-Except for the hash algorithm names in which need the full name.
+In stead of numbers the first few letters of the value may be used.
+Except for the hash algorithm name, where the full name must be specified.
 
 .SH OPTIONS
 .IP -4
@@ -92,7 +89,7 @@ Don't try to resolve \fIname\fR, but connect to \fIaddress\fR in stead.
 
 This option may be given more than once.
 .IP -b
-print "\fIname\fR\. TYPE52 \\#\fIsize\fR \fIhexdata\fR" form in stead
+print "\fIname\fR\. TYPE52 \\# \fIsize\fR \fIhexdata\fR" form in stead
 of TLSA presentation format.
 .IP "-c \fIcertfile\fR"
 Do not TLS connect to \fIname\fR:\fIport\fR, but authenticate (or make
@@ -139,11 +136,9 @@ are also given, only TLSA records that match the \fIname\fR, \fIport\fR and
 \fItransport\fR are used. Otherwise the owner name of the TLSA record(s)
 will be used to determine \fIname\fR, \fIport\fR and \fItransport\fR.
 .IP -u
-Use UDP transport in stead of TCP to TLS connect.
+Use UDP transport in stead of TCP.
 .IP -v
 Show version and exit.
-.IP "-V \fI[0-5]\fR
-Set verbosity level (defaul 3)
 
 .SH AUTHOR
 Written by the ldns team as an example for ldns usage.

diff --git a/examples/ldns-dane.c b/examples/ldns-dane.c
index 0e5a6c57f2913427e9ab4e06e13c2fd9ac83f254..2f3dd793047a2a525bfa640159ee9d788e61b9fc 100644 (file)
--- a/examples/ldns-dane.c
+++ b/examples/ldns-dane.c
@@ -178,7 +178,6 @@ ldns_err(const char* s, ldns_status err)
                ssl_err(s);
        } else {
                fprintf(stderr, "error: %s\n", ldns_get_errorstr_by_id(err));
-               assert(0);
                exit(EXIT_FAILURE);
        }
 }
@@ -536,7 +535,7 @@ print_rr_as_TYPEXXX(FILE* out, ldns_rr* rr)
        for (i = 0; i < ldns_rr_rd_count(rr); i++) {
                 sz += ldns_rdf_size(ldns_rr_rdf(rr, i));
        }
-       ldns_buffer_printf(buf, "\t\\#%d ", sz);
+       ldns_buffer_printf(buf, "\t\\# %d ", sz);
        for (i = 0; i < ldns_rr_rd_count(rr); i++) {
                s = ldns_rdf2buffer_str_hex(buf, ldns_rr_rdf(rr, i));
                LDNS_ERR(s, "could not ldns_rdf2buffer_str_hex");

diff --git a/ldns/keys.h b/ldns/keys.h
index fe6f4c3e1e35ab5a3e66751183b8eccdf2e9f156..3e156233ba24cdc433bec630db930f60b7aa1014 100644 (file)
--- a/ldns/keys.h
+++ b/ldns/keys.h
@@ -299,7 +299,7 @@ void ldns_key_set_evp_key(ldns_key *k, EVP_PKEY *e);
 
 /**
  * Set the key's rsa data.
- * The rsa data should be freed by the user themselve.
+ * The rsa data should be freed by the user.
  * \param[in] k the key
  * \param[in] r the rsa data
  */
@@ -307,7 +307,7 @@ void ldns_key_set_rsa_key(ldns_key *k, RSA *r);
 
 /**
  * Set the key's dsa data
- * The dsa data should be freed by the user themselve.
+ * The dsa data should be freed by the user.
  * \param[in] k the key
  * \param[in] d the dsa data
  */

diff --git a/ldns/radix.h b/ldns/radix.h
index b586ddda1736e046e190a82e7f4589168d814bf5..f8833eb2cec89d3201995254ddf47b368707691e 100644 (file)
--- a/ldns/radix.h
+++ b/ldns/radix.h
@@ -162,8 +162,6 @@ ldns_radix_node_t* ldns_radix_search(ldns_radix_t* tree, uint8_t* key,
 int ldns_radix_find_less_equal(ldns_radix_t* tree, uint8_t* key,
        radix_strlen_t len, ldns_radix_node_t** result);
 
-/** ldns_radix_find_less_equal */
-
 /**
  * Get the first element in the tree.
  * @param tree: tree.