hipl-core team mailing list archive

[René Hummen <rene.hummen@xxxxxxxxxxxxxxxxx>]

See comments below.

On Aug 20, 2010, at 5:01 PM, noreply@xxxxxxxxxxxxx wrote:
> ------------------------------------------------------------
> revno: 4908
> committer: Christof Mroz <christof.mroz@xxxxxxxxxxxxxx>
> branch nick: trunk
> timestamp: Fri 2010-08-20 16:59:41 +0200
> message:
>  Clear ctx->error to 0 prior to processing each socket.
> 
>  Maybe that's the wrong place to patch so here's some background info.

As Diego pointed out: if in doubt, ask :-) Feedback in diff.

>  I stumbled upon this while testing hipd via two VMs, i.e. running hipd on
>  both of them and ping6-ing the respective HITs. On some machines it worked
>  right away but on two setups it didn't because R2 packets were perfectly
>  received (i.e. HIT-to-IP resolution worked) but not processed. Turns out
>  ctx->error was set to -1 before any processing even took place, so of
>  course the packet was dropped at hip_run_handle_functions. As far as I can
>  tell, even though ctx is bound per-packet it's a stack-allocated struct in
>  hipd_main that's reused all the time so it must be cleared at one point or
>  another.

So your point is that the previously received or sent packet resulted in an error and that the detection of this error (ctx-error = -1) carried over to handling of the R2 packet. Did you check which error was detected initially? While fixing the side-effect on R2 handling, it might also be necessary to have a look at the original error.

>  Well anyway, this patch solved my problems so far.

This is a good start, but your patch should integrate cleanly with the structure of the handle_functions, in order for other developers to be able to read and understand the code easily. Your patch "hides" the re-initialization somewhere in the code.

> modified:
>  hipd/hip_socket.c

[...]

> === modified file 'hipd/hip_socket.c'
> --- hipd/hip_socket.c	2010-07-07 16:42:17 +0000
> +++ hipd/hip_socket.c	2010-08-20 14:59:41 +0000
> @@ -276,6 +276,7 @@
>             socketfd = ((struct socketfd*) iter->ptr)->fd;
> 
>             if (FD_ISSET(socketfd, read_fdset)) {
> +                ctx->error = 0;
>                 ((struct socketfd*) iter->ptr)->func_ptr(ctx);
>             }
>         }

The error member of ctx provides feedback to the set of handle-functions, whether the previously executed handle-function returned an error. In that case, there should be no further processing on the current packet. As such, ctx->error has a more global scope than mere socket errors and should therefore not be re-initialized in hipd/hip_socket.c. You should move the re-initialization to the beginning of the while loop of hipd/hipd.c:383 (http://bazaar.launchpad.net/%7Ehipl-core/hipl/trunk/annotate/4910/hipd/hipd.c#L383) and re-initialize all members of ctx that are required to be initialized to 0/NULL. This should solve similar bugs with different members of ctx as well.

Ciao,
René



--
Dipl.-Inform. Rene Hummen, Ph.D. Student
Distributed Systems Group
RWTH Aachen University, Germany
tel: +49 241 80 20772
web: http://ds.rwth-aachen.de/members/hummen