Re: [Ipsec-tools-devel] WG: Smallel fixes and 2 new features

[Roman H. A. <rh...@op...>]

Hi Wolfgang

I looked through your first bunch of patches.


Patch 1
=======

Nothing to say.


Patch 2
=======

Chunk 2 white space error, see below (p3).

Also chunk 2: removal of rmconf from union looks like cleanup work, not
related to the task of p2. Move it to another patch please.

"@@ -63,27 +63,56 @@": this chunk and another (p3?) introduces comments
about initialisation that are obvious. IMHO comments should be removed.

Again "@@ -63,27 +63,56 @@": remove WSC comment

"@@ -603,6 +603,9 @@": duprmconf_shallow was written by me, so on a
first glance I suspect your change introduces either a double free or a
memleak. But this has to be confirmed by valgrind (or similar method),
using inherited remote confs and reload, just like all the other spots
touched by you for memory management improvements.

I know, you thought it all through, but I don't have the time to wind my
brain again around these data structures. A complete verification with
valgrind won't be feasible (too many execution paths). I'm not sure if
I'll find the time to run a couple of tests myself, once these patches
become formally correct.


Patch 3
=======

First chunk of p3 (patch three) is a cleanup, which is caused by p2.
Please cleanup yourself :-).

The directory structure of the patches is good (patch -p1 conform):
  --- ipsec-tools-0.8.0-p2/src/racoon/cfparse.y	

Also good: the custom #ifdef statements marking your doings are gone.

P3 chunk 6 "@@ -521,16 +569,43 @@": at the end there is white space
cleanup, not related to your task.

Same goes for "@@ -1653,7 +1728,6 @@".

Fishy: @@ -86,7 +86,7 @@ (localconf.c)

Cheers,
Roman