Code review comment for lp:~rene-hummen/hipl/puzzle-fixes

 review abstain

I have no opinion on this merge request, just some minor comments
that you may wish to ignore or adapt at your discretion...

On Fri, Apr 08, 2011 at 03:41:24PM +0000, René Hummen wrote:
>
> --- lib/core/solve.c 2011-01-24 10:21:17 +0000
> +++ lib/core/solve.c 2011-04-08 15:41:17 +0000
> @@ -28,11 +28,14 @@
>
> #include <errno.h>
> +#include <openssl/rand.h>
> #include <stdint.h>
> #include <string.h>
> +#include <strings.h>

nit:

#include <errno.h>
#include <stdint.h>
#include <string.h>
#include <strings.h>
#include <openssl/rand.h>

> @@ -43,104 +46,170 @@
> +static int hip_calculate_puzzle_solution(const struct puzzle_common *const compute_puzzle,
> + unsigned char sha_digest[SHA_DIGEST_LENGTH],
> + const int difficulty)
> {
> + HIP_IFEL(difficulty > max_puzzle_difficulty,
> + -1, "difficulty exceeds max. configured difficulty\n");
> +
> + HIP_IFEL(hip_build_digest(HIP_DIGEST_SHA1,
> + compute_puzzle,
> + sizeof(struct puzzle_common),
> + sha_digest),
> + -1, "failed to compute hash digest\n");
> +
> + /* check if position of first least significant 1-bit is higher than
> + * difficulty */
> + if (ffs(truncated_digest) > difficulty) {
> + err = 0;
> } else {
> + err = 1;
> + }
> +
> +out_err:
> + return err;
> +}

I would suggest doing that without HIP_IFEL, that would even save two
lines in the if/else clause.

> +/**
> + * Solve a computational puzzle for HIP
> + *
> + * @param puzzle puzzle to be solved
> + * @param difficulty difficulty of the puzzle
> + * @param initiator_hit initiator HIT of the message exchange
> + * @param responder_hit responder HIT of the message exchange
> + * @param solution computed puzzle solution
> + * @return 0 when solution was found, 1 in case no solution was found after
> + * MAX_PUZZLE_SOLUTION_TRIES, -1 in case of an error
> + */
> +int hip_solve_puzzle(const uint8_t puzzle[PUZZLE_LENGTH],
> + const int difficulty,
> + const hip_hit_t initiator_hit,
> + const hip_hit_t responder_hit,
> + uint8_t solution[PUZZLE_LENGTH])

Having solution as third parameter would feel more natural to me.

> +/**
> + * Verify a computational puzzle for HIP
> + *
> + * @param puzzle puzzle to be verified
> + * @param difficulty difficulty of the puzzle
> + * @param initiator_hit initiator HIT of the message exchange
> + * @param responder_hit responder HIT of the message exchange
> + * @param solution puzzle solution to be verified
> + * @return 0 when solution is correct, -1 otherwise
> + */
> +int hip_verify_puzzle_solution(const uint8_t puzzle[PUZZLE_LENGTH],
> + const uint8_t difficulty,
> + const hip_hit_t initiator_hit,
> + const hip_hit_t responder_hit,
> + const uint8_t solution[PUZZLE_LENGTH])

Having solution as third parameter would feel more natural to me.

> + HIP_IFEL(hip_calculate_puzzle_solution(&compute_puzzle,
> + sha_digest,
> + difficulty),
> + -1, "failed to verify puzzle solution\n");
> +
> out_err:
> return err;
> }

I suggest doing this without HIP_IFEL.

> --- test/lib/core/solve.c 1970-01-01 00:00:00 +0000
> +++ test/lib/core/solve.c 2011-04-08 15:41:17 +0000
> @@ -0,0 +1,170 @@
> +
> +#include <check.h>
> +#include <openssl/rand.h>
> +#include <stdint.h>

nit:

#include <check.h>
#include <stdint.h>
#include <openssl/rand.h>

Diego