Code review comment for lp:~rene-hummen/hipl/midauth-hipd

On 08.08.2011, at 17:01, Christof Mroz wrote:
> Looks good, aside from what Diego mentioned already (I'll approve as soon as the conflict is resolved). One minor point:
>
> +/**
> + * Convert the opaque value in the CHALLENGE_REQUEST to the seed value I of a
> + * HIP puzzle.
> + *
> + * The opaque value plays a dual role in a CHALLENGE_REQUEST:
> + * i) it is a challenge that needs to be echoed back by the responder and
> + * ii) it is used to derive the seed value for a cryptographic puzzle. The
> + * puzzle is defined in RFC5201.
> + *
> + * @param opaque the nonce (challenge) in the CHALLENGE_REQUEST
> + * @param opaque_len the length of the nonce
> + * @param puzzle_value the puzzle value generated from the nonce
> + * @return zero on success, -1 in case of an error
> + */
> +int hip_midauth_puzzle_seed(const uint8_t opaque[],
> + const unsigned int opaque_len,
> + uint8_t puzzle_value[PUZZLE_LENGTH])
> +{
> + unsigned char sha_digest[SHA_DIGEST_LENGTH];
> +
>
> + if (!puzzle_value) {
> + HIP_ERROR("Parameter puzzle_value is not allocated\n");
> + return -1;
> + }
>
> This looks like it should never happen, i.e. an assertion would be more appropriate. Same for the opaque parameter.

You are right. I implemented it as an if statement because in contrast to asserts HIP_ASSERTs are always executed. I changed this to HIP_ASSERT nevertheless in order to make a transition to assert() less painful in the future.

Ciao,
René

--
Dipl.-Inform. Rene Hummen, Ph.D. Student
Chair of Communication and Distributed Systems
RWTH Aachen University, Germany
tel: +49 241 80 20772
web: http://www.comsys.rwth-aachen.de/team/rene-hummen/