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

On Mon, 28 Mar 2011 19:03:34 +0200, Diego Biurrun <email address hidden> wrote:

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

Hooray! On to the final boss.

>> + // 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.

It's more readable this way IMO.

>> --- 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.

OK, Good idea.