Code review comment for lp:~christof-mroz/hipl/hipfw-esp-speedup

 review abstain

I'm fine with the merge proposal now, some minor comments below.
All you need to pass now is Stefan's review :)

On Mon, Mar 28, 2011 at 02:06:47PM +0000, Christof Mroz wrote:
> Christof Mroz has proposed merging lp:~christof-mroz/hipl/hipfw-esp-speedup into lp:hipl with lp:~christof-mroz/hipl/hipfw-timeout as a prerequisite.
>
> --- firewall/conntrack.c 2011-03-28 14:06:39 +0000
> +++ firewall/conntrack.c 2011-03-28 14:06:40 +0000
> @@ -1984,6 +2114,174 @@
>
> + if ((str_spi = strstr(input, "spi:"))) {
> + // non-UDP
> + if (sscanf(str_spi, "spi:%u", spi) < 1) {
> + HIP_ERROR("Unexpected iptables output (spi): '%s'\n", input);
> + return false;
> + }
> + } else if ((str_spi = strstr(input, u32_prefix))) {
> + // UDP
> + // spi follows u32_prefix string as a hex number
> + // (always host byte order)
> + if (sscanf(&str_spi[sizeof(u32_prefix) - 1], "%x", spi) < 1) {
> + HIP_ERROR("Unexpected iptables output (u32 match): '%s'\n", input);
> + return false;
> + }
> + } else {
> + // no SPI specified, so it's no ESP rule
> + return false;
> + }
> +
> + // grab packet count and destination IP.
> + if (sscanf(input, formats[0], packet_count, ip) < 2) {
> + // retry with alternative format before we give up
> + if (sscanf(input, formats[1], packet_count, ip) < 2) {
> + HIP_ERROR("Unexpected iptables output (number of colums): '%s'\n", input);
> + return false;
> + }
> + }

All the inner if-conditions could be folded into the outer if-conditions.

> + while (fgets(bfr, sizeof(bfr), p)) {
> + if (strncmp(bfr, "Chain", 5) == 0) {
> + chain_ok = (strncmp(bfr, "Chain HIPFW-INPUT", 17) == 0) ||
> + (strncmp(bfr, "Chain HIPFW-OUTPUT", 18) == 0) ||
> + (strncmp(bfr, "Chain HIPFW-FORWARD", 19) == 0);

nit: The () are unnecessary.

> --- firewall/pisa.c 2011-01-14 15:53:02 +0000
> +++ firewall/pisa.c 2011-03-28 14:06:40 +0000
> @@ -311,6 +311,12 @@
> if (t) {
> t->connection->pisa_state = PISA_STATE_ALLOW;
> HIP_INFO("PISA accepted the connection.\n");
> +
> + struct slist *lst = t->esp_tuples;
> + while (lst) {
> + hip_fw_manage_esp_tuple(lst->data, true);
> + lst = lst->next;
> + }
> } else {
> HIP_ERROR("Connection not found.\n");
> }
> @@ -329,6 +335,15 @@
> if (t) {
> t->connection->pisa_state = PISA_STATE_DISALLOW;
> + HIP_INFO("PISA removed the connection.\n");
> +
> + struct slist *lst = t->esp_tuples;
> + while (lst) {
> + hip_fw_manage_esp_tuple(lst->data, false);
> + lst = lst->next;
> + }
> + } else {
> + HIP_ERROR("Connection not found.\n");
> }
> }

This might be worth moving into its own function to avoid the
code duplication.

Diego