Document: draft-ietf-dime-diameter-api-07.txt
Reviewer: Francis Dupont
Review Date: 2008-08-20
IETF LC End Date: 2008-08-08
IESG Telechat date: unknown

Summary: Not Ready

Comments: my main concern is the API as it is described up to section 3.7 is clearly impossible to implement so I strongly suggest to add soon (in section 2 for instance) some text explaining the document only specifies the visible/public part of some structures.
Another issue is most of the "include" part is missing.

Detailed comments:
 - 1 page 4: code need only -> code needs only
 - 2.2 page 6: All of the functions -> All functions ?
 - 2.5 page 6: a DTD described it -> a DTD describing it
 - 2 page 6: IMHO this is the right place to explain the structs
  defined in the document are the visible/public part of the real/internal
  structs with a reference to section 3.7
 - 3.1.12 page 10: if -> when ?
 - 3.1.15 page 12: the header (six, four) is obviously about a previous
  version...
 - 3.1.15 page 12: IMHO you should explain why, despite the name "flags",
  it is right to use an enumeration here (exclusive values, etc).
 - 3.1.16 page 12: AAAGetDomainInterconnectType() doesn't return
  this type (cf. 3.4.3.7) and it is not AAAGetDomainInternconnectType()
                                                         ^
 - 3.2.3 page 15: this type should be opaque, i.e., the name of the
  fields should be private. This has two consequences:
  * users don't know the names so can't misuse them (they have
   AAAGetFirstAVP() & co)
  * it justifies a bit the *avpList in the next section (in place
   of plain avpList)
 BTW in a traditional double-linked list with tail pointer the  tail is a ** pointer
 - 3.2.4 page 16: extra *Version fields (unneeded with AAA_IP_ADDR)
 - 3.4.3.7 page 24: type should be a AAADomainInterconnect *
 - 3.4.4.6 page 27: AAAGetCommandCode() must return an AAAReturnCode
  and the return value text be updated to the usual one
 - 3.4.5.4 page 29: occured -> occurred
 - 3.4.5.6 page 30: AAACreateAndAddAVPToList() is underspecified:
  * can be the avpList created when it points to NULL?
   (cf next section)
  * what is the position when the list is not empty?
 - 3.4.5.[67] pages 30 & 31: there is no way to free an AAA_AVP_List
  so what to do if something fails? Obviously there are some use
  restrictions so at least a reference to 3.6 is wellcome
 - 3.4.5.1[45] page 34 (comment): obviously AAAGet{Next,Prev}AVP()
  imply link fields in the real AVP struct!
 - 3.4.6.1 page 36: extra ipVersion field
 - 3.4.6.2 page 36 (comment): again the server id is an internal
  AAAMessage field
 - 3.6 page 38: (for uniformity)
  (char *) ourAddress -> (char *)ourAddress
 - 3.7 page 39: this is a key point but one has to read ~40 pages
  to find it. Note there are other ways to handle public/private
  fields in C (but no highly recommended ways):
   * the sub structure way (document's one)
   * private fields at the end of the definition with a comment
    explaining they are private
   * private fields at the end of the definition protected by a #ifdef
  I prefer the second one (no cast, sizeof() works but it relies
  on the programmer's discipline) but you should simply give the
  choice to implementors
 - 3.9 page 41: 3.1.13 specifies there can be only one first/last,
  for instance:
   AAA_APP_INSTALL_FIRST:  Install this callback as the first callback
      in the chain.  If subsequent callbacks are installed with this
      code, then AAA_ERR_ALREADY_REGISTERED is returned.
  so 3.1.13 and 3.9 are in conflict (note I prefer 3.1.13)
 - 7.2 page 45: where are the DIAM_AVP_* & co defined?
  I am afraid you have to add them...
 - Author's Addresses page 46: please add the Contry (USA? :-)
 - I'd like to have a .h in annex (note the names of the .h and
  of the DLL/library/... are part of the API)