[hipl-dev] Re: [Merge] lp:~cviethen/hipl/pisa-pairing into lp:hipl
- From: Stefan Götz <stefan.goetz@xxxxxxxxxxxxxxxxx>
- To: mp+63577@xxxxxxxxxxxxxxxxxx
- Date: Tue, 07 Jun 2011 06:53:22 -0000
Review: Needs Fixing
Hi Christoph!
Nice feature!
> === modified file 'hipd/netdev.c'
> --- hipd/netdev.c 2011-05-06 11:54:08 +0000
> +++ hipd/netdev.c 2011-06-06 16:15:55 +0000
> @@ -726,6 +726,105 @@
> }
>
> /**
> + * Begin an opportunistic base exchange with the IP address specified in the
> + * user message, creating an appropriate HIP association if necessary.
> + *
> + * @param msg the user message - it needs to contain an IP as its only
> parameter
[M] documentation: Hm - 'user message' might be used elsewhere but it is
confusing as it dates back to the good ol days of HIP residing in the kernel.
How about hipconf message? Which parameter type is used for the IP address? Is
it an IPv4 or IPv6 address? Please be more precise for people who would like to
know how exactly to use your function.
[M] policy: please cover this function with unit tests
> +int hip_netdev_pair(const struct hip_common *msg)
[M] style: please use full const correctness: 'const struct hip_common *const
msg'
> + /* a few simple plausibility checks */
> + if (!(ipv6_addr_is_hit(&our_hit) && ipv6_addr_is_hit(&peer_hit) &&
> hip_hidb_hit_is_our(&our_hit))) {
> + HIP_ERROR("Internal error.\n");
[L] usability: even if no one will try to interpret this message however helpful
it tries to be, it is still painfully useless in its current form :)
[L] would it make sense to use HIP_ASSERT() here?
> + /* if we got an entry in the HADB already, proceed to sending I1 */
> + entry = hip_hadb_find_byhits(&our_hit, &peer_hit);
> + if (entry && !ipv6_addr_any(&entry->our_addr)) {
> + goto send_i1;
[M] cleanliness: urgh, goto programming? I thought we decided that that is a bad
idea like what? 40 years ago? ;-) Please use a simple if block here
> + /* if not, create such an entry first */
> + if (hip_hadb_add_peer_info(&peer_hit, peer_ip, NULL, NULL)) {
> + HIP_ERROR("Creation of HADB entry failed.\n");
> + return -1;
> + }
[M] correctness: in case of a later error, this hadb entry is not removed again.
Would that make sense?
[L] style: overall, this is a very long function. I believe it would be cleaner
to split it at least into the part that parses the message and determines all
other input parameters and the part that actually creates the HADB entry and
sends the I1.
> === modified file 'lib/core/conf.c'
> --- lib/core/conf.c 2011-05-31 13:40:37 +0000
> +++ lib/core/conf.c 2011-06-06 16:15:55 +0000
> @@ -234,6 +236,7 @@
> "shotgun on|off\n"
> "id-to-addr hit|lsi\n"
> "broadcast on|off\n"
> + "pair <ip|ip6|hostname>\n"
[M] documentation/usability: even if the other commands do not do it: please add
a brief description of the effect of this command.
> @@ -2353,6 +2362,95 @@
> }
>
> /**
> + * Handles the hipconf @c pair command.
> + *
> + * @param msg a pointer to the buffer where the message for hipd will
> + * be written.
> + * @param action (currently unused)
> + * @param opt an array of pointers to the command line arguments after
> + * the action and type.
> + * @param optc the number of elements in the array (@b 0).
> + * @param send_only (currently unused)
> + * @return zero on success, or negative error value on error.
> + */
[L] style: please align all parameter descriptions on the same column.
> +static int hip_conf_handle_pair(struct hip_common *msg,
> + UNUSED int action,
> + const char *opt[],
> + int optc,
> + UNUSED int send_only)
[M] policy: please make this function const correct
The rest looks fine.
Cheers,
Stefan
--
https://code.launchpad.net/~cviethen/hipl/pisa-pairing/+merge/63577
Your team HIPL core team is subscribed to branch lp:hipl.
Other related posts:
