Code review comment for lp:~hipl-core/hipl/libhip

On 08/02/12 20:00, Diego Biurrun wrote:
> review needs-fixing
>
> On Wed, Feb 08, 2012 at 08:43:18AM +0000, Xin wrote:
>> You have been requested to review the proposed merge of lp:~hipl-core/hipl/libhip into lp:hipl.
>>
>> === modified file 'Makefile.am'
>> --- Makefile.am 2012-01-30 12:28:31 +0000
>> +++ Makefile.am 2012-02-07 15:20:56 +0000
>> @@ -64,19 +65,22 @@
>>
>> ### libraries ###
>> lib_LTLIBRARIES = lib/core/libhipcore.la
>> -
>> +lib_LTLIBRARIES += lib/hipdaemon/libhipdaemon.la
>>
>> ### tests ###
> Oh, the poor empty line ...
Fixed, I didn't notice that there is a style for empty lines
>
>> if HIP_UNITTESTS
>> -TESTS = test/check_hipd \
>> - test/check_hipfw \
>> - test/check_lib_core \
>> - test/check_lib_tool \
>> +TESTS = test/check_hipd \
>> + test/check_hipfw \
>> + test/check_hipnetcat \
>> + test/check_lib_core \
>> + test/check_lib_tool \
>> test/check_modules_midauth
>> -check_PROGRAMS = test/check_hipd \
>> - test/check_hipfw \
>> - test/check_lib_core \
>> - test/check_lib_tool \
>> +
>> +check_PROGRAMS = test/check_hipd \
>> + test/check_hipfw \
>> + test/check_hipnetcat \
>> + test/check_lib_core \
>> + test/check_lib_tool \
>> test/check_modules_midauth
>> endif
> Having to realign all those backslashes is suboptimal. I'll move them
> all to a sensible position in trunk.
I merged your newest change, which align to column 72, but some new
contents in libhip are long than 72. I align those longer lines to 84
>
>> @@ -86,55 +90,17 @@
>> test_auth_performance_SOURCES = test/auth_performance.c
>> test_certteststub_SOURCES = test/certteststub.c
>> test_dh_performance_SOURCES = test/dh_performance.c
>> -test_fw_port_bindings_performance_SOURCES = test/fw_port_bindings_performance.c \
>> - hipfw/file_buffer.c \
>> +test_fw_port_bindings_performance_SOURCES = hipfw/file_buffer.c \
>> hipfw/line_parser.c \
>> - hipfw/port_bindings.c
>> + hipfw/port_bindings.c \
>> + test/fw_port_bindings_performance.c
> This is unrelated, push to trunk right away.
Pushed
>> @@ -225,6 +233,8 @@
>> test/hipfw/rewrite.c \
>> $(hipfw_hipfw_sources)
>>
>> +test_check_hipnetcat_SOURCES = test/check_hipnetcat.c
>> +
>> test_check_lib_core_SOURCES = test/check_lib_core.c \
>> test/lib/core/crypto.c \
>> test/lib/core/hit.c \
>> @@ -241,21 +251,35 @@
>> test/modules/midauth/hipd/midauth.c \
>> $(hipd_hipd_sources)
>>
>> -
>> ### static library dependencies ###
>> -
> Oh, those poor empty lines.
Fixed
>> -hipd_hipd_LDADD = lib/core/libhipcore.la
>> -hipfw_hipfw_LDADD = lib/core/libhipcore.la
>> -test_auth_performance_LDADD = lib/core/libhipcore.la
>> -test_check_hipd_LDADD = lib/core/libhipcore.la
>> -test_check_hipfw_LDADD = lib/core/libhipcore.la
>> -test_check_lib_core_LDADD = lib/core/libhipcore.la
>> -test_check_lib_tool_LDADD = lib/core/libhipcore.la
>> -test_check_modules_midauth_LDADD = lib/core/libhipcore.la
>> -test_certteststub_LDADD = lib/core/libhipcore.la
>> -test_dh_performance_LDADD = lib/core/libhipcore.la
>> -test_fw_port_bindings_performance_LDADD = lib/core/libhipcore.la
>> -test_hc_performance_LDADD = lib/core/libhipcore.la
>> +hipd_hipd_LDADD = lib/core/libhipcore.la \
>> + lib/hipdaemon/libhipdaemon.la
>> +hipfw_hipfw_LDADD = lib/core/libhipcore.la \
>> + lib/hipdaemon/libhipdaemon.la
>> +test_auth_performance_LDADD = lib/core/libhipcore.la \
>> + lib/hipdaemon/libhipdaemon.la
>> +test_check_hipd_LDADD = lib/core/libhipcore.la \
>> + lib/hipdaemon/libhipdaemon.la
>> +test_check_hipfw_LDADD = lib/core/libhipcore.la \
>> + lib/hipdaemon/libhipdaemon.la
>> +test_check_hipnetcat_LDADD = lib/core/libhipcore.la \
>> + lib/hipdaemon/libhipdaemon.la
>> +test_check_lib_core_LDADD = lib/core/libhipcore.la \
>> + lib/hipdaemon/libhipdaemon.la
>> +test_check_lib_tool_LDADD = lib/core/libhipcore.la \
>> + lib/hipdaemon/libhipdaemon.la
>> +test_check_modules_midauth_LDADD = lib/core/libhipcore.la \
>> + lib/hipdaemon/libhipdaemon.la
>> +test_certteststub_LDADD = lib/core/libhipcore.la \
>> + lib/hipdaemon/libhipdaemon.la
>> +test_dh_performance_LDADD = lib/core/libhipcore.la \
>> + lib/hipdaemon/libhipdaemon.la
>> +test_fw_port_bindings_performance_LDADD = lib/core/libhipcore.la \
>> + lib/hipdaemon/libhipdaemon.la
>> +test_hc_performance_LDADD = lib/core/libhipcore.la \
>> + lib/hipdaemon/libhipdaemon.la
>> +test_hipnetcat_LDADD = lib/core/libhipcore.la \
>> + lib/hipdaemon/libhipdaemon.la
>> tools_hipconf_LDADD = lib/core/libhipcore.la
>> tools_pisacert_LDADD = lib/core/libhipcore.la
> This cannot be correct. Looks like you just mechanically changed this
> w/o verifying the necessity.
Fixed
>> --- lib/core/message.c 2011-11-08 15:25:41 +0000
>> +++ lib/core/message.c 2012-02-07 15:20:56 +0000
>> @@ -94,9 +94,9 @@
>> #include "hip_udp.h"
>> #include "icomm.h"
>> #include "ife.h"
>> +#include "message.h"
>> #include "prefix.h"
>> #include "protodefs.h"
>> -#include "message.h"
> unrelated and wrong
I revert this change
>> @@ -685,6 +685,72 @@
>>
>> +int hip_read_control_msg_tcp(int sockfd, struct hip_packet_context *ctx)
>> +{
>> + HIP_IFEL(hip_verify_network_header(ctx->input_msg,&src_addr,
>> +&dst_addr, len),
>> + -1, "verifying network header failed\n");
>> +
>> +out_err:
>> + return err;
>> +}
> HIP_IFEL abuse - I already remarked on this.
Fixed.
>
> Please don't ignore my review comments. Either address all of them
> before sending in a new merge request or explain why you do not address
> them.
>
> Diego
>
I check all the changes again, wish this time I don't missing anything.

Xin Gu