[hipl-dev] Re: [Merge] lp:~diego-biurrun/hipl/hipfw-performance into lp:hipl

On Mon, 10 Jan 2011 23:39:58 +0100, Stefan Götz <stefan.goetz@xxxxxxxxxxxxxxxxx> wrote:


If [H] or [M] is followed by a question, that's a point where I need more information to make a vote on the merge. No real need to change the code right away.

I know that this may sound overly formal and a bit full of myself but I hope that it simply helps the branch author to more easily identify important comments, what blocks a merge in my opinion and what is just a remark.

That's greatly appreciated; more focused than the bunch of mails I've kept bookmarked over the months. That said, I've answered some of these points already but I'll restate for reference.

+#include <linux/netfilter_ipv4.h>

[H] license: does the inclusion introduce a GPL infection?

I planned on scrapping this inclusion altogether anyway.

+#ifdef CONFIG_HIP_DEBUG
+// this improves our chances of finding bugs in the timeout code
+#define DEFAULT_CONNECTION_TIMEOUT 20; // 20 seconds
+#define DEFAULT_CLEANUP_INTERVAL   10; // 10 seconds
+#else
+#define DEFAULT_CONNECTION_TIMEOUT (60 * 5);  // 5 minutes
+#define DEFAULT_CLEANUP_INTERVAL   (60 * 60); // 1 minute
+#endif

[M] style & policy: use constants instead of macros

Glad to hear that HIPL finally adopted constants.

@@ -116,7 +149,7 @@
  *
  * @param hiptuple HIP tuple
  */
-static void print_tuple(const struct hip_tuple *hiptuple)
+static void print_tuple(DBG const struct hip_tuple *hiptuple)

[L] style: const correctness for pointer arguments. May apply to other functions. Please check.

Is this a policy already? And if so, does it apply to pointers only or everything? I mean, the difference between

const blah *ptr   versus   const blah *const ptr

is the same as in

int x  versus  const int x,

right?

+ * @todo Test rules using userspace_ipsec, Relay, LSI, sys-opp, midauth,
+ *       light-update and esp_prot configurations.
+ * @todo Test with different byte ordering.

[L] I don't like TODOs in trunk. Especially if they effectively say that the code has poor quality due to lazyness ;-)

Don't worry, I explicitly stated not to merge unless these features are confirmed to survive the changes (or not) in the original proposal, but my HIPL knowledge is probably not good enough to thoroughly test these features myself yet. René expected no complications, but that doesn't warrant removing this TODO yet I think. Anyway, have to merge with trunk first...

Also, explicitly stating things that might get broke could be interpreted as careful foresight and investigation, rather than lazyness ^^

+    if (esp_tuple->esp_prot_tfm > ESP_PROT_TFM_UNUSED) {
+        HIP_DEBUG("ESP Transforms requested; not handled via iptables "
+                  "since we need to inspect packets\n");
+        return 0;
+    }
+
+    if (esp_tuple->tuple->esp_relay) {
+        HIP_DEBUG("ESP Relay requested; not handled via iptables "
+                  "since we need packet rewriting\n");
+        return 0;
+    }

[L] design/style: It seems strange to me that 'success' is returned when the function didn't actually complete successfully. It might make sense to have a return value that says 'you called this function under conditions with which a call does not make sense'.

It's expected behaviour, and because the return value is not checked anyway it probably doesn't warrant multiple error levels.

+out_err:
+    if (err == EXIT_SUCCESS) {
+        total_esp_rules_count += (insert ? 1 : -1);
+ HIP_DEBUG("total_esp_rules_count = %d\n", total_esp_rules_count);
+    }
+    return err;
+}

[L] style: it's counter-intuitive to have non-error code after the label out_err.

Good point, I'll move the label right before the return.

+
+/**
+ * Insert a destination address into an esp_tuple. If same address exists already,
+ * the update_id is replaced with the new value instead.

[M] docs: does this sentence really reflect the service provided by the function? I think the function actually changes iptables rules, right?

Yeah, that should be added.

+    if (total_esp_rules_count > 0) {
+        const size_t sz = sizeof(*snapshot) * total_esp_rules_count;
+        size_t n = 0;
+
+        HIP_IFEL(!(snapshot = malloc(sz)), 1, "Insufficient memory\n");
+

[M] style: Could a local array variable be used instead of malloc()?
[L] style: if possible, use a local array variable instead of heap memory to simplify memory management

Not as-is, but maybe I can get rid of the snapshot altogether when ditching the manual packet counting (no clear plan yet).

+    iter_conn = conn_list;
+    while (iter_conn) {
+        struct connection *conn = (struct connection*) iter_conn->data;

[M] style: could the cast be avoided?
[L] style: remove cast if possible

Currently no, because containers are of the home-brew void* based kind right now.

Not optimal, because even if we switched to unions, as you proposed not long ago in another context, we'll risk cache incoherency because of an additional indirection (note that you can't avoid using pointers because the list bookkeeping code must be generic), and miniscule space overhead per link. Also, AFAIK and on a side note, strictly speaking only the type used by the very last write to an union can be considered "deterministic", that is, void*-based bookkeeping followed by non-void*-based usage is allowed by the compiler (because it must trust you here) but not guaranteed to work on all architectures (don't ask me which).

Would be easy in C++ using templates, but the existing C solutions are heavily macro-based (uthash, for example) and I'm not sure about licensing. No bold claims here anyway, I have no idea if all this matters enough to architectures targeted by hipl.

=== modified file 'hipd/hiprelay.c'
--- hipd/hiprelay.c     2010-11-30 14:50:30 +0000
+++ hipd/hiprelay.c     2010-12-13 21:28:35 +0000
@@ -1015,16 +1015,16 @@
  *
  * @param r the HIP control message to be relayed
  * @param type_hdr message type
- * @param r_saddr the original source address
- * @param r_daddr the original destination address
+ * @param r_saddr (unused) the original source address
+ * @param r_daddr (unused) the original destination address

[M] what is the point of adding unused function arguments to an already crowded signature?

For debug purposes. Touching the function body would be out of the scope of this branch (even this doc change was, sorry).

You forgot one [H]: missing unit tests... was looking forward to do that too.

Other related posts: