Code review comment for lp:~hipl-core/hipl/hipv2-dh-ecdh

On Sun, Sep 16, 2012 at 12:15:22AM +0800, Xin Gu wrote:
> On 09/10/2012 03:19 PM, Diego Biurrun wrote:
> > review needs-fixing
> >You sort of addressed my comments, but I'm really not sure how thorough.
> >In order to check for simple oversights I searched this email for "llips"
> >and immediately caught an error I pointed out but you failed to fix.
>
> oops, I read the merge diff line by line before I commit it, but it
> seems that I still miss some points.
>
> >It seems that all newly-added memory allocations are checked now. This
> >is good; unfortunately unchecked memory allocations still happen left
> >and right in HIPL.
>
> This is not related to this merge right? If we should discuss how to
> fix them, I think it is better to put it into another mail thread,
> or fire a bug?

Yes. While we do have the boyscout rule, we cannot expect you to fix
all unchecked memory allocation problems in HIPL. Of course you should
not make the problem worse by adding new ones though.

> >This whole review business is rather pointless if the comments are not
> >addressed. If I can find mistakes in your revised code literally within
> >seconds then we have serious quality control issues as a project.
>
> People do make mistakes, if you check how many "llips" I have fixed
> and how I fix your other comments, maybe you will feel a sense of
> relief.

I don't mean to imply that you're not making an effort. Your effort and
contribution is very much appreciated. Nonetheless we need to keep up
code quality and doublecheck each other's work.

If I review your code, notice an issue and find it again in the next
iteration you send, then I have not found a bug in the code, but in your
workflow. What we need to fix then is both the bug and the workflow so
that more such bugs don't reappear again. Everybody wins.

As a general rule, bugs appear in clusters. If I or one of my reviewers
notices a bug in some code I work on, I don't just fix that instance of
the bug. I make an effort to doublecheck the rest of the code I work on
and ideally all of the codebase for similar problems. In 99% of the
cases I stumble over more issues in this way.

How you look for similar issues depends on the case. For typos you can
grep, for other issues you might want to use a regexp grep or look through
relevant files manually.

In the case of your typo, you should not just look at my review comments
and fix all of the ones I pointed out. I could have overlooked one,
reviewers make mistakes as well; grep does not. Plus, it could even be
faster, so everybody wins.

> >>--- libhipl/cookie.c 2012-06-03 10:26:55 +0000
> >>+++ libhipl/cookie.c 2012-09-08 08:34:18 +0000
> >>@@ -219,41 +218,93 @@
> >>+struct hip_common *hip_get_r1_v2(struct in6_addr *ip_i, struct in6_addr *ip_r,
> >>+ struct in6_addr *our_hit, const int dh_group_id)
> >>+{
> >>+ /* Create a copy of the found entry */
> >>+ len = hip_get_msg_total_len(&hid->r1_v2[dh_group_id][idx].buf.msg);
> >>+ if (len <= 0) {
> >>+ HIP_ERROR("Invalid r1_v2 entry at %d, %d\n", dh_group_id, idx);
> >>+ return NULL;
> >>+ }
> >>+
> >>+ if((r1 = hip_msg_alloc()) == NULL) {
> >if (
> >
> >Is your uncrustify hook not working?
>
> Yeah, I turned it off because some of code changes will violate it,
> but I should have checked other codes. you have my apology.

What part of your or our code is not compatible with the hook?

Diego