Hello,

on Jan 9, 2012, at 12:16 AM, Diego Biurrun wrote:

> Just a couple of nits,

Admit it: you love it! ;-)

> I don't know that part of the code well.

Welcome to the club. :-)

> This sentence explains why starting a sentence with "this sentence" is
> silly; same for "this function" in Doxygen function documentation.

Ack!

>> +struct hip_hadb_state *hip_hadb_find_by_srchit_destip_port(hip_hit_t *src_hit, const struct in6_addr *dst_ip, in_port_t dst_port);
>> +
> 
> Break this long line.

Oh yes. It got a bit worse still in the meantime, so I went a little radical now when it came to breaking it.

Ack!

>> + * Gets called in case of an intended opportunistic base exchange - checks
>> + * first whether a host association is already established, and won't perform
>> + * the base exchange if so.  [...]
> 
> Please start by explaining what the function does, not why and avoid the
> third person singular.

Ack!

>> + * @param  msg  the message from hipconf
>> + * @return zero on success and non-zero on error
> 
> It returns -1.

You may be right. But then again, I tend to trust the documentation of the functions that I'm calling. And there it says the same. Hmmm. :)

>> +int hip_netdev_trigger_opp_bex_msg(const struct hip_common *const msg)

>> +    /* Find out: 1. our own hit, */

>> +    /* 2. the IP address the base exchange should be sent to, */

> These two conditions could be merged.

Ah, good catch. Ack.

>> +    const in_port_t ha_peer_port = (hip_nat_status ? hip_get_peer_nat_udp_port() : 0);
> 
> pointless parentheses

Ack!

>> +    if (entry != NULL) {
> 
> if (entry) {

Ack!

>> + * Handles the <tt> hipconf daemon trigger bex </tt> command.
> 
> Handle ..

Ack!

>> + *                   the action and type.
> 
> We don't generally use that extra space indentation.

Okay, I'll try to do without it in the function headers then. Let me know please if you spot more of that kind that I may have overlooked.

>> +static int conf_handle_trigger_bex(struct hip_common *const msg,
>> +                                                  const int action,
>> +                                                const char *opt[],
>> +                                                  const int optc,
>> +                                                 UNUSED int send_only)
> 
> Ugh..

Please elaborate - are you referring more to the formatting, or to the multitude of consts, or ... ?

>> +    if (action != ACTION_TRIGGER) {
>> +        return -1;
>> +    }
>> +
>> +    if (optc > 1) {
>> +        HIP_ERROR("Too many arguments for trigger bex command.\n");
>> +        return -1;
>> +    }
> 
> Could be merged.

Umm, I'm a little slow right now, maybe. I only want that HIP_ERROR() output in one of the two cases.

>> +    if (ai_err == 0) {
> 
> maybe
> 
>  if (!ai_err) {

Looks okay to me. Ack, also to the other, similar instances. I basically like more explicit comparisons, but as far as functionality is concerned, it doesn't matter, and as long as it doesn't lead to bad obfuscation, I don't mind really.

>> +    const int uh_err = hip_build_user_hdr(msg, HIP_MSG_TRIGGER_OPP_BEX, 0);
>> +
>> +    if (uh_err) {
>> +        HIP_ERROR("Failed to build user message header: %s\n", strerror(uh_err));
>> +        return -1;
>> +    }

> The variable indirection with uh_err looks inconsistent to me, you don't
> do it in the other cases.

I guess I was keen to keep things as const correct as possible. One might argue, of course, that over such a small stretch of code, nothing bad can happen to uh_err, since one can see with a blink of the eye that, past initialization, it's not written to any more.

Hmmm. I tend to leaving it that way, but if you feel const correctness is not of paramount importance here, one could of course change it, making it a little more compact.


Many thanks for your thorough review!


Cheers,

  Christoph

-- 
 <email address hidden>