review needs-fixing

On Sun, Oct 30, 2011 at 07:49:28AM +0000, Miika Komu wrote:
> Miika Komu has proposed merging lp:~hipl-core/hipl/ecdsa-redhat into lp:hipl.
> 
> --- firewall/rule_management.c	2011-08-15 14:11:56 +0000
> +++ firewall/rule_management.c	2011-10-30 07:48:24 +0000
> @@ -81,8 +81,9 @@
>  /* filename needs to contain one of these to be valid HI file */
>  #define RSA_FILE "_rsa_"
>  #define DSA_FILE "_dsa_"
> +#ifdef HAVE_EC_CRYPTO
>  #define ECDSA_FILE "_ecdsa_"
> -
> +#endif /* HAVE_EC_CRYPTO */
>  #define MAX_LINE_LENGTH 512

unnecessary

> @@ -444,6 +445,7 @@
>      return err;
>  }
>  
> +#ifdef HAVE_EC_CRYPTO
>  /**
>   * Load an ECDSA public key from a file and convert it into a hip_host_id.
>   *
> @@ -479,6 +481,8 @@
>      return err;
>  }
>  
> +#endif /* HAVE_EC_CRYPTO */
> +
>  /**
>   * load a public key from a file and convert it to a hip_host_id structure
>   *

Drop the empty line before the #endif, same below

> --- lib/core/builder.h	2011-08-15 14:11:56 +0000
> +++ lib/core/builder.h	2011-10-30 07:48:24 +0000
> @@ -26,18 +26,21 @@
>  #ifndef HIP_LIB_CORE_BUILDER_H
>  #define HIP_LIB_CORE_BUILDER_H
>  
> +#include "config.h"
> +
>  #include <stdint.h>
>  #include <netinet/in.h>
>  #include <openssl/rsa.h>
>  #include <openssl/dsa.h>
> -#include <openssl/ec.h>
>  
> -#include "config.h"
>  #include "certtools.h"
>  #include "debug.h"
>  #include "icomm.h"
>  #include "state.h"
>  
> +#ifdef HAVE_EC_CRYPTO
> +#include <openssl/ec.h>
> +#endif /* HAVE_EC_CRYPTO */

We have system headers before local headers for a reason.

> --- lib/core/crypto.h	2011-07-18 13:10:26 +0000
> +++ lib/core/crypto.h	2011-10-30 07:48:24 +0000
> @@ -26,12 +26,16 @@
>  #ifndef HIP_LIB_CORE_CRYPTO_H
>  #define HIP_LIB_CORE_CRYPTO_H
>  
> +#include "config.h"
> +
>  #include <stdint.h>
>  #include <netinet/in.h>
>  #include <sys/types.h>
>  #include <openssl/dsa.h>
>  #include <openssl/rsa.h>
> +#ifdef HAVE_EC_CRYPTO
>  #include <openssl/ec.h>
> +#endif /* HAVE_EC_CRYPTO */
>  #include <openssl/dh.h>
>  #include <openssl/pem.h>

.. like you did here ..

> --- lib/core/hostid.c	2011-10-25 21:14:16 +0000
> +++ lib/core/hostid.c	2011-10-30 07:48:24 +0000
> @@ -28,6 +28,8 @@
>   * @brief Host identifier manipulation functions
>   */
>  
> +#include "config.h"
> +
>  #include <errno.h>
>  #include <stdint.h>
>  #include <stdlib.h>
> @@ -40,7 +42,6 @@
>  #include <openssl/pem.h>
>  #include <openssl/rsa.h>
>  
> -#include "config.h"
>  #include "lib/tool/pk.h"
>  #include "builder.h"
>  #include "crypto.h"

unnecessary / unrelated

> @@ -689,6 +715,12 @@
>      struct endpoint_hip     *endpoint_ecdsa_hip     = NULL;
>      struct endpoint_hip     *endpoint_ecdsa_pub_hip = NULL;
>  
> +    if (ecdsa_nid < 0) {
> +        err = -1;
> +        HIP_ERROR("NID for ECDSA is strange %d\n", ecdsa_nid);
> +        goto out_err;
> +    }

?

> @@ -1059,41 +1101,58 @@
>      if (rsa_filenamebase_pub != NULL) {
>          change_key_file_perms(rsa_filenamebase_pub);
>      }
> -    if (ecdsa_filenamebase_pub != NULL) {
> -        change_key_file_perms(ecdsa_filenamebase_pub);
> -    }
> -    if (ecdsa_filenamebase_pub != NULL) {
> -        change_key_file_perms(ecdsa_filenamebase_pub);
> -    }
> +#ifdef HAVE_EC_CRYPTO
> +    if (ecdsa_filenamebase_pub != NULL) {
> +        change_key_file_perms(ecdsa_filenamebase_pub);
> +    }
> +    if (ecdsa_filenamebase_pub != NULL) {
> +        change_key_file_perms(ecdsa_filenamebase_pub);
> +    }
> +#endif /* HAVE_EC_CRYPTO */
>  
>      free(dsa_host_id);
>      free(dsa_pub_host_id);
> +#ifdef HAVE_EC_CRYPTO
>      free(ecdsa_host_id);
>      free(ecdsa_pub_host_id);
> +#endif /* HAVE_EC_CRYPTO */
>      free(rsa_host_id);
>      free(rsa_pub_host_id);
>      DSA_free(dsa_key);
> +#ifdef HAVE_EC_CRYPTO
>      EC_KEY_free(ecdsa_key);
> +#endif /* HAVE_EC_CRYPTO */
>      RSA_free(rsa_key);
>      DSA_free(dsa_pub_key);
> +#ifdef HAVE_EC_CRYPTO
>      EC_KEY_free(ecdsa_pub_key);
> +#endif /* HAVE_EC_CRYPTO */
>      RSA_free(rsa_pub_key);
>      free(dsa_key_rr);
> +#ifdef HAVE_EC_CRYPTO
>      free(ecdsa_key_rr);
> +#endif /* HAVE_EC_CRYPTO */
>      free(rsa_key_rr);
>      free(dsa_pub_key_rr);
> +#ifdef HAVE_EC_CRYPTO
>      free(ecdsa_pub_key_rr);
> +#endif /* HAVE_EC_CRYPTO */
>      free(rsa_pub_key_rr);
>      free(endpoint_dsa_hip);
> +#ifdef HAVE_EC_CRYPTO
>      free(endpoint_ecdsa_hip);
> +#endif /* HAVE_EC_CRYPTO */
>      free(endpoint_rsa_hip);
>      free(endpoint_dsa_pub_hip);
> +#ifdef HAVE_EC_CRYPTO
>      free(endpoint_ecdsa_pub_hip);
> +#endif /* HAVE_EC_CRYPTO */
>      free(endpoint_rsa_pub_hip);

ETOOMANYIFDEFS

> --- lib/core/hostid.h	2011-07-18 13:10:26 +0000
> +++ lib/core/hostid.h	2011-10-30 07:48:24 +0000
> @@ -29,7 +29,9 @@
>  #include <netinet/in.h>
>  #include <openssl/dsa.h>
>  #include <openssl/rsa.h>
> +#ifdef HAVE_EC_CRYPTO
>  #include <openssl/ec.h>
> +#endif /* HAVE_EC_CRYPTO */

This will not work without config.h.

Did you test this on a system with ECC?  Apparently not, this should at
least lead to 'make checkheaders' failures.

> --- lib/tool/pk.h	2011-07-08 11:47:12 +0000
> +++ lib/tool/pk.h	2011-10-30 07:48:24 +0000
> @@ -9,8 +9,10 @@
>  int hip_dsa_sign(void *peer_pub, struct hip_common *msg);
>  int hip_rsa_verify(void *priv_key, struct hip_common *msg);
>  int hip_rsa_sign(void *peer_pub, struct hip_common *msg);
> +#ifdef HAVE_EC_CRYPTO
>  int hip_ecdsa_verify(void *const peer_pub, struct hip_common *const msg);
>  int hip_ecdsa_sign(void *const peer_pub, struct hip_common *const msg);
> +#endif /* HAVE_EC_CRYPTO */
>  int bn2bin_safe(const BIGNUM *a, unsigned char *to, int len);

unnecessary

> === modified file 'test/lib/tool/pk.c'
> --- test/lib/tool/pk.c	2011-07-18 13:10:10 +0000
> +++ test/lib/tool/pk.c	2011-10-30 07:48:24 +0000
> @@ -27,7 +27,9 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <stdio.h>
> +#ifdef HAVE_EC_CRYPTO
>  #include <openssl/ec.h>
> +#endif /* HAVE_EC_CRYPTO */
>  #include <openssl/pem.h>

see above

Diego