Hi harald,

attached with this email last version of patches and some comments.

Harald Welte wrote:

>Thanks pablo.
>
>I am fine with that patch, and I'd like to put it in patch-o-matic for
>now.  As the changes are fairly obvious, I hope we won't get too much
>trouble with it ;)  However, please see my following comments.
>
>  
>
ok, feedback is always welcome. :-)

>>I finished my patch to add an ip_conntrack_expect_alloc(...) function. I 
>>also patched all the current conntrack helpers available in the stable 
>>branch and patch-o-matic-ng.
>>    
>>
>
>thanks. This means that once we get it into the core kernel, we should
>definitely have  the first pom-ng release out, otherwise people will
>still use the old API.
>
>This makes me think of another issue.  We should either rename the
>function, reorder arguments, do whatever.   We cannot just make all old
>(unpatched) code compile without any warning, and still have it crash :(
>I think it's best to exchange the order of the two arguments in
>ip_conntrack_expect_related().  This way a compiler would complain if
>some old helper was compiled with the new core.
>
>  
>

As you suggested, I modified the order of the arguments in expect_related.

>>BTW, I know that there's a skeleton of a helper in the documention, due 
>>to they mostly look the same but I think that I could add it more details.
>>    
>>
>
>yes, please.  Use the SGML master file, don't edit the HTML
>  
>

ok! I will.

>>- There's a memset in expect_alloc (see expect.patch) after the 
>>allocation is done, I don't like it anyway but I just added to keep 
>>backward compatibility because some helper usually do it. I think that 
>>it's better setting all the fields of the new expectation in the helper 
>>(this implies being a bit more restrictive with the programmer but well, 
>>this is kernel programming anyway, it's restrictive :-)).
>>    
>>
>
>yes, sometimes code like this is included mainly for debugging purpose
>and then left behind.
>
>  
>
>>I could delete that memset and have a look at all the helpers to make 
>>sure that all the fields are correctly set.
>>    
>>
>
>The issue is not very easy in this case.  Assume that tuple->dst.u
>is 32bits wide (like after applying the pptp patch).  This in turn means
>that every helper has to explicitly take care not to initialize
>tuple->dst.u with a 16bit tcp/udp port number, since it could end
>up in the 'other' 16bit half.  Rather, they have to assign
>tuple->dst.u.tcp.port.  Those cases should all be fixed by now.
>
>Now look what tuple_cmp does.  It memcmp's the whole union, not just the
>individual fields.  This means that a tuple always has to be cleanly
>initialized with zero, even in those parts that don't contain any
>useful information.
>
>So for now, I'd like to keep that memset until all the code is audited
>with that regard.
>  
>

ok, i'll study this issue calmly.

>>- I found a condition where expect_related could succesfully insert an 
>>expectaction when removing an old one and return EPERM (see 
>>expect_related-newnat14.patch). I think that this doesn't make sense. Am 
>>I forgetting something? That patch fix that problem.
>>    
>>
>
> 
>  
>
>>- I also have no problem about updating the documentation to add info 
>>about expect_alloc.
>>    
>>
>
>ok, please do so.
>  
>

ok! I will.

>One issue with your patch:  It doesn't seem to use tab's for
>indentation, but rather 8 spaces. Please clean this up.
>  
>

it's true, I think that it was because of some copy&paste issues related 
to vim and the use of cat. I'll pay attention to this kind of stuff in 
future since it seems that they are not sensible to indentation. I think 
these patches don't have this problem.

>Also, we cannot export that symbol GPL only, since it doesn't add a new
>API but rather replaces an old one.
> 
>  
>
also fixed!

If these patches are ok, I'll also prepare the patches for the pom-ng 
helpers.

Pablo

P.D: BTW, so should I patch the amanda helper against patrick patch?