[hipl-dev] Re: [Merge] lp:~hipl-core/hipl/opp-removal into lp:hipl
- From: Diego Biurrun <diego@xxxxxxxxxx>
- To: hipl-dev@xxxxxxxxxxxxx
- Date: Thu, 13 Jan 2011 20:33:48 +0100
review needs-fixing
On Mon, Jan 10, 2011 at 06:30:49PM +0000, Rene Hummen wrote:
> Rene Hummen has proposed merging lp:~hipl-core/hipl/opp-removal into lp:hipl.
>
> Requested reviews:
> HIPL core team (hipl-core)
>
> For more details, see:
> https://code.launchpad.net/~hipl-core/hipl/opp-removal/+merge/45730
>
> NOTE: While the normal BEX is still fully functional, I did not check the
> opportunistic mode due to not available setup.
Does this leave any code under #ifdef HIP_OPPORTUNISTIC in HIPL?
Did you do a 'grep -ri' for "opportunistic" or even "opp" and did you
grep for the names of the files you deleted? It's all too easy to
overlook some stragglers...
> --- firewall/cache.c 2011-01-09 22:18:11 +0000
> +++ firewall/cache.c 2011-01-10 18:30:48 +0000
> @@ -63,7 +63,7 @@
> *
> * @return the allocated cache entry
> */
> -struct hip_hadb_user_info_state *hip_cache_create_hl_entry(void)
> +static struct hip_hadb_user_info_state *hip_cache_create_hl_entry(void)
This is an unrelated nicety. Keeping such changes out of topic branches
simplifies reviewing and improves the quality of HIPL if you push the
changes sooner through a cleanup branch.
> --- firewall/firewall.c 2011-01-10 10:14:22 +0000
> +++ firewall/firewall.c 2011-01-10 18:30:48 +0000
> @@ -98,15 +98,13 @@
>
> /* packet types handled by the firewall */
> #define OTHER_PACKET 0
> #define HIP_PACKET 1
> #define ESP_PACKET 2
> -#define TCP_PACKET 3
> -#define FW_PROTO_NUM 4 /* number of packet types */
> +#define FW_PROTO_NUM 3 /* number of packet types */
These could be converted into an enum.
> @@ -413,31 +409,9 @@
> }
>
> /**
> - * Initialize packet capture rules for system-based opportunistic mode
> + * Initialize all basic and extended packet capture rules
.
Also looks unrelated.
> @@ -1143,7 +1036,7 @@
> static int hip_fw_handle_hip_output(struct hip_fw_context *ctx){
> int verdict = accept_hip_esp_traffic_by_default;
>
> - HIP_DEBUG("hip_fw_handle_hip_output \n");
> + HIP_DEBUG("\n");
Just drop it.
> @@ -1162,7 +1055,9 @@
>
> /**
> - * Process an ESP packet from the outbound packet queue
> + * Process an ESP packet from the outbound packet queue.
> + *
> + * @note hooks ESP filtering
> *
> * @param ctx the packet context
> *
> @@ -1184,7 +1079,9 @@
>
> /**
> - * Process an ESP packet from the outbound packet capture queue
> + * Process any other packet from the outbound packet capture queue
> + *
> + * @note hooks userspace IPsec and LSI
> *
> * @param ctx the packet context
> *
These can likely be committed separately.
> @@ -1205,14 +1102,11 @@
> // check if this is a reinjected packet
> if (def_hit && IN6_ARE_ADDR_EQUAL(&ctx->dst, def_hit)) {
> // let the packet pass through directly
> - verdict = 1;
> + verdict = ACCEPT;
As can this, IIUC.
> @@ -1221,15 +1115,12 @@
> /* LSI HOOKS */
> if (IS_LSI32(dst_lsi.s_addr) && hip_lsi_support) {
> if (hip_is_packet_lsi_reinjection(&dst_lsi)) {
> - verdict = 1;
> + verdict = ACCEPT;
> } else {
> hip_fw_handle_outgoing_lsi(ctx->ipq_packet,
> &src_lsi, &dst_lsi);
> - verdict = 0; /* Reject the packet */
> + verdict = DROP; /* Reject the packet */
same
> @@ -1239,22 +1130,10 @@
>
> /**
> * Process a HIP packet from the forward packet capture queue
> *
> + * @note hooks middlebox authentication
> + *
> * @param ctx the packet context
> *
> * @return the verdict (1 for pass and 0 for drop)
ditto
More below, but I will spare you the nagging from here on :)
> @@ -1529,10 +1331,9 @@
> /* ip_hl is given in multiple of 4 bytes
> *
> * NOTE: not sizeof(struct ip) as we might have options */
> - ip_hdr_len = (iphdr->ip_hl * 4);
> - // needed for opportunistic TCP
> - ctx->ip_hdr_len = ip_hdr_len;
> - HIP_DEBUG("ip_hdr_len is: %d\n", ip_hdr_len);
> + ctx->ip_hdr_len = (iphdr->ip_hl * 4);
pointless parentheses
> --- hipd/input.c 2011-01-10 17:51:29 +0000
> +++ hipd/input.c 2011-01-10 18:30:48 +0000
> @@ -495,6 +493,62 @@
>
> /**
> + * fetch an hadb entry corresponding to a pseudo HIT
> + *
> + * @param init_hit the local HIT of the Initiator
> + * @param resp_addr the remote IP address of the Responder from
> + * which to calculate the pseudo HIT
> + * @return a host association or NULL if not found
> + */
> +static struct hip_hadb_state *hip_opp_get_hadb_entry(const hip_hit_t * const
> init_hit,
> +
> +/**
> + * find a host association based on I1 or R1 message
> + *
> + * @param msg the I1 or R2 message
> + * @param src_addr the source address of the message
> + * @return the host association or NULL if not found
> + */
> +static struct hip_hadb_state *hip_opp_get_hadb_entry_i1_r1(struct hip_common
> *msg,
Correct spelling and punctuation is a plus.
> +{
> + hip_hdr type = hip_get_msg_type(msg);
> + struct hip_hadb_state *entry = NULL;
> +
> + if (type == HIP_I1) {
> + if (!ipv6_addr_is_null(&msg->hitr)) {
> + goto out_err;
> + }
> + hip_get_default_hit(&msg->hitr);
> + } else if (type == HIP_R1) {
> + entry = hip_opp_get_hadb_entry(&msg->hitr, src_addr);
> + } else {
> + HIP_ASSERT(0);
> + }
> +
> +out_err:
> + return entry;
> +}
Maybe we should rethink all those gotos...
> --- hipd/user.c 2011-01-04 19:22:24 +0000
> +++ hipd/user.c 2011-01-10 18:30:48 +0000
> @@ -490,7 +485,7 @@
> HIP_IFEL(hip_hadb_add_peer_info(dst_hit, dst_ip,
> NULL, NULL),
> -1, "Error on adding server " \
> - "HIT to IP address mapping to the hadb.\n");
> + "HIT to IP address mapping to the hadb.\n");
Unrelated - and the \ is superfluous.
> --- lib/core/conf.c 2011-01-09 22:18:11 +0000
> +++ lib/core/conf.c 2011-01-10 18:30:48 +0000
> @@ -2203,9 +2134,6 @@
> HIP_DEBUG("Executing %s.\n", argv[0]);
> if (type == EXEC_LOADLIB_HIP) {
> libs[0] = strdup("libhiptool.so");
> - } else if (type == EXEC_LOADLIB_OPP) {
> - libs[0] = strdup("libopphip.so");
> - libs[1] = strdup("libhiptool.so");
> }
The first branch of the if is wrong, there is no such thing as libhiptool
anymore. This is unrelated to your branch though.
> --- lib/core/hashtable.c 2010-10-19 02:38:50 +0000
> +++ lib/core/hashtable.c 2011-01-10 18:30:48 +0000
> @@ -41,8 +41,6 @@
> * @brief Hashtable wrappers for OpenSSL lhash implementation
> *
> * @author Miika Komu <miika@xxxxxx>
> - * @see lib/opphip/wrap_db.c for a minimal hash table implementation
> - * example
> */
Unrelated, committed separately :)
Diego
Other related posts:
