Nortel merge: Some open points for merge into trunk

On Wed, Jun 11, 2008 at 10:01:06PM +0200, Zingo Andersen wrote:
> It works for me on the nortel-branch
> 
> Great that this made inte the repo now I can forget everything
> about it  :)
> (and it will show up in my Ubuntu someday in the future)

Hmm, I'd like to let you off that hook not quite yet 

There are a few things that I'd like to review and a few thing that should
probably be changed before I think that a merge would be OK. The following
came to my mind by taking a rather brief look at the diff:

1) There is a set of new dup_ and free_ functions. I assume that they
   are there to fix a memory leak or two, so they might collide with
   Martin's work.  --> Martin: Any comments?

2) Some // style comments, so far only the old style comments are used.

3) Renaming the attributes _RFC_ and _NORTEL_ is not quite correct.
   _D06_ and _D02_ (or _DRAFT06_ and _DRAFT02_) would be the correct
   names. Or maybe just replace NORTEL with _DRAFT02_ and leave the
   other names alone.

4) In at least one place, the code uses 32767 instead of
   IKE_ATTRIB_NORTEL_UNKNOWN new_isakmp_attribute_16(32767, 10, a).
   While we are at it, maybe give the number 10 a symbolic name as
   well.

5) Code style: Some of the new places do someting like this:
   }
   else {
   whereas the remaining code uses
   } else {

6) The code to create the proposals seems to have grown significantly -
   I still have to understand why  ---> Self: Try to understand
   if (opt_vendor == VENDOR_NORTEL) {
          auth = 0;

7) Comments remaining from development work should go
   /* memcpy(l->u.id.data, key_id, strlen(key_id)); */
   // Nortel specific version
   /* removed for NortelVPN
     ...
    */

8) Is a separate make_our_sa_ipsec_nortel really necessary?

9) Same goes here: 
   // Nortel specific
   static int check_transform(struct sa_block *s,struct isakmp_payload *transform)

10) I get the feeling that some of the code needs to be changed to be more
  flexible to be able to handle both cases without so much new code.

11) The xauth functions probably should be suffixed _02 and _06.

I don't expect you to do all that - it's just meant as an indicator that we are
not quite done yet.

Thanks for your work so far!!!

Ciao
      Joerg
--

-- 
Joerg Mayer                                           <jmayer@...>
We are stuck with technology when what we really want is just stuff that
works. Some say that should read Microsoft instead of technology.