review needs-fixing

On Thu, Nov 03, 2011 at 03:01:41PM +0000, Miika Komu wrote:
> Miika Komu has proposed merging lp:~hipl-core/hipl/ecdsa-redhat into lp:hipl.
> 
> NEW: I believe I have resolved the comments from Diego.

sort of :)

> --- firewall/rule_management.c	2011-08-15 14:11:56 +0000
> +++ firewall/rule_management.c	2011-11-03 15:00:34 +0000
> @@ -82,7 +82,6 @@
>  #define RSA_FILE "_rsa_"
>  #define DSA_FILE "_dsa_"
>  #define ECDSA_FILE "_ecdsa_"
> -
>  #define MAX_LINE_LENGTH 512

stray unrelated change

> @@ -444,6 +443,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 +479,8 @@
>      return err;
>  }
>  
> +#endif /* HAVE_EC_CRYPTO */
> +
>  /**
>   * load a public key from a file and convert it to a hip_host_id structure
>   *

Please drop the empty line befor the #ifdef.

same elsewhere

> --- lib/core/builder.h	2011-08-15 14:11:56 +0000
> +++ lib/core/builder.h	2011-11-03 15:00:34 +0000
> @@ -26,19 +26,22 @@
>  #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>
> +#ifdef HAVE_EC_CRYPTO
>  #include <openssl/ec.h>
> -
> -#include "config.h"
> +#endif /* HAVE_EC_CRYPTO */
> +
> +
>  #include "certtools.h"
>  #include "debug.h"
>  #include "icomm.h"
>  #include "state.h"
>  
> -
>  /* Removed in 2.6.11 - why ? */
>  extern struct hip_cert_spki_info hip_cert_spki_info;

one stray empty line inserted, one removed

> --- lib/core/crypto.h	2011-10-30 11:54:44 +0000
> +++ lib/core/crypto.h	2011-11-03 15:00:34 +0000
> @@ -92,25 +96,33 @@
>  uint16_t hip_get_dh_size(uint8_t hip_dh_group_type);
>  DSA *create_dsa_key(const int bits);
>  RSA *create_rsa_key(const int bits);
> +#ifdef HAVE_EC_CRYPTO
>  EC_KEY *create_ecdsa_key(const int nid);
> +#endif /* HAVE_EC_CRYPTO */
>  int save_dsa_private_key(const char *const filenamebase, DSA *const dsa);
>  int save_rsa_private_key(const char *const filenamebase, RSA *const rsa);
> +#ifdef HAVE_EC_CRYPTO
>  int save_ecdsa_private_key(const char *const filenamebase, EC_KEY *const ecdsa);
> +#endif /* HAVE_EC_CRYPTO */
>  int load_dsa_private_key(const char *const filenamebase, DSA **const dsa);
>  int load_rsa_private_key(const char *const filename, RSA **const rsa);
> +#ifdef HAVE_EC_CRYPTO
>  int load_ecdsa_private_key(const char *const filename, EC_KEY **const ec);
> +#endif /* HAVE_EC_CRYPTO */
>  int impl_dsa_sign(const unsigned char *const digest,
>                    DSA *const dsa,
>                    unsigned char *const signature);
>  int impl_dsa_verify(const unsigned char *const digest,
>                      DSA *const dsa,
>                      const unsigned char *const signature);
> +#ifdef HAVE_EC_CRYPTO
>  int impl_ecdsa_sign(const unsigned char *const digest,
>                      EC_KEY *const ecdsa,
>                      unsigned char *const signature);
>  int impl_ecdsa_verify(const unsigned char *const digest,
>                        EC_KEY *const ecdsa,
>                        const unsigned char *const signature);
> +#endif /* HAVE_EC_CRYPTO */
>  int hip_write_hmac(int type, const void *key, void *in, int in_len, void *out);
>  int hip_crypto_encrypted(void *data, const void *iv, int enc_alg, int enc_len,
>                           uint8_t *enc_key, int direction);

I think it's worth grouping this together to save ifdeffery.

> --- lib/core/hostid.c	2011-10-25 21:14:16 +0000
> +++ lib/core/hostid.c	2011-11-03 15:00:34 +0000
> @@ -40,7 +40,6 @@
>  #include <openssl/pem.h>
>  #include <openssl/rsa.h>
>  
> -#include "config.h"
>  #include "lib/tool/pk.h"
>  #include "builder.h"
>  #include "crypto.h"

I very much doubt this is correct.

> --- lib/core/hostid.h	2011-07-18 13:10:26 +0000
> +++ lib/core/hostid.h	2011-11-03 15:00:34 +0000
> @@ -62,11 +66,15 @@
>                           struct hip_ecdsa_keylen *const ret);
>  RSA *hip_key_rr_to_rsa(const struct hip_host_id_priv *const host_id, const int is_priv);
>  DSA *hip_key_rr_to_dsa(const struct hip_host_id_priv *const host_id, const int is_priv);
> +#ifdef HAVE_EC_CRYPTO
>  EC_KEY *hip_key_rr_to_ecdsa(const struct hip_host_id_priv *const host_id, const int is_priv);
> +#endif /* HAVE_EC_CRYPTO */
>  
>  int dsa_to_dns_key_rr(const DSA *const dsa, unsigned char **const buf);
>  int rsa_to_dns_key_rr(const RSA *const rsa, unsigned char **const rsa_key_rr);
> +#ifdef HAVE_EC_CRYPTO
>  int ecdsa_to_key_rr(const EC_KEY *const ecdsa, unsigned char **const ec_key_rr);
> +#endif /* HAVE_EC_CRYPTO */

I think this could be grouped.

> --- lib/tool/pk.c	2011-08-15 14:11:56 +0000
> +++ lib/tool/pk.c	2011-11-03 15:00:34 +0000
> @@ -15,9 +17,11 @@
>  #include <netinet/in.h>
>  #include <openssl/bn.h>
>  #include <openssl/dsa.h>
> -#include <openssl/ecdsa.h>
>  #include <openssl/objects.h>
>  #include <openssl/rsa.h>
> +#ifdef HAVE_EC_CRYPTO
> +#include <openssl/ecdsa.h>
> +#endif /* HAVE_EC_CRYPTO */

I wonder why you move the header.

> --- test/lib/tool/pk.c	2011-07-18 13:10:10 +0000
> +++ test/lib/tool/pk.c	2011-11-03 15:00:34 +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>

Probably missing config.h.

Diego