I have been selected as the General Area Review Team (Gen-ART)
reviewer for this draft (for background on Gen-ART, please see
http://www.alvestrand.no/ietf/gen/art/gen-art-FAQ.html).
Please resolve these comments along with any other Last Call comments
you may receive.
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)
Regards
[EMAIL PROTECTED]
_______________________________________________
Gen-art mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/gen-art
