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

On Sun, 27 Mar 2011 19:36:27 +0200, Diego Biurrun <email address hidden> wrote:

> review needs-fixing
>
> On Sun, Mar 27, 2011 at 03:51:36PM +0000, Christof Mroz wrote:
>> Christof Mroz has proposed merging lp:~christof-mroz/hipl/hipfw-timeout
>> into lp:hipl.
>>
>> --- firewall/conntrack.c 2011-03-24 17:34:21 +0000
>> +++ firewall/conntrack.c 2011-03-27 15:51:27 +0000
>> @@ -74,8 +74,33 @@
>> +
>> +#define DEFAULT_CONNECTION_TIMEOUT (60 * 5); // 5 minutes
>> +#define DEFAULT_CLEANUP_INTERVAL (60 * 1); // 1 minute
>> +
>> +time_t cleanup_interval = DEFAULT_CLEANUP_INTERVAL;
>> +
>> +time_t connection_timeout = DEFAULT_CONNECTION_TIMEOUT;
>
> The defines seem completely redundant to me, just assign the values to
> the variables without indirection.

I just realized that only a fraction of HIPL does it that way, but it's
not consistent... so given the choice now, I'll drop that indirection.

> Not sure how to handle the rest of this code that you copypasted.
> Fixing it may be out of the scope of this branch, but it sure is not
> pretty...

If the hipfw <-> hipd link is supposed to be purely local, this part
should be rewritten using Domain sockets or FIFOs and proper access
control (also in hipd, of course). Would be nice if someone with more
authority could either decline this idea or file a feature request (or bug
report even?) as a reminder.