Code review comment for lp:~martin-lp/hipl/hipfwconf

Hi,

On Tue, Nov 8, 2011 at 9:47 AM, Diego Biurrun <email address hidden> wrote:
> review needs-fixing
>
> On Mon, Nov 07, 2011 at 05:15:33PM +0000, David Martin wrote:
>> David Martin has proposed merging lp:~martin-lp/hipl/hipfwconf into lp:hipl.
>>
>> --- firewall/firewall_control.c 2011-10-25 21:14:16 +0000
>> +++ firewall/firewall_control.c 2011-11-07 17:14:40 +0000
>> @@ -38,10 +38,12 @@
>>
>> +#include "conntrack.h"
>> #include "lib/core/builder.h"
>> #include "lib/core/debug.h"
>> #include "lib/core/ife.h"
>> #include "lib/core/message.h"
>> +#include "lib/core/prefix.h"
>> #include "lib/core/protodefs.h"
>> #include "cache.h"
>> #include "firewall.h"
>
> conntrack.h should come after cache.h.

fixed.

>> --- lib/core/conf.c 2011-11-03 09:21:12 +0000
>> +++ lib/core/conf.c 2011-11-07 17:14:40 +0000
>> @@ -226,6 +234,51 @@
>>
>> +const char *hipfwconf_usage =
>> + HIPCONF_HIPFW_KEYWORD
>> + " <command>\n\n"
>> + "HIP firewall commands:\n"
>> + "get ha <hit> | all\n";
>
> I think this can be static.

yup, fixed.

>> @@ -510,6 +563,25 @@
>>
>> /**
>> + * Map daemon / firewall keyboard to its respective enum.
>
> You are mapping a keyBOARD? Is that maybe a typo? :)

Heaven forbid! That does indeed seem to be a typo. ^^ Fixed.

>> @@ -2525,7 +2604,8 @@
>>
>> if (err) {
>> - HIP_ERROR("(Check syntax for hipconf. Is hipd running or root privilege needed?)\n");
>> + HIP_ERROR("(Check syntax for hipconf. Is hipd or hipfw running or root"
>> + " privilege needed?)\n");
>
> Maybe just say "daemon" instead of enumerating the 12345 programs that
> will become part of HIPL over the next decade.

You are right. Personally I find it a bit strange to call hipd the HIP daemon when they are all daemons. Whatever, fixed this as well.

>
>> --- lib/core/conf.h 2011-08-15 14:11:56 +0000
>> +++ lib/core/conf.h 2011-11-07 17:14:40 +0000
>> @@ -54,6 +54,11 @@
>> #define ACTION_ADD 1
>> #define ACTION_NEW 3
>>
>> +enum daemon_name { HIP_DAEMON, HIP_FIREWALL, UNKNOWN_KEYWORD };
>
> This is still unused outside of conf.c.

You are right. I made it static.