Subject: Re: [Dime] diameter-api draft comparison

From: "Romascanu, Dan (Dan)" <[email-masked]>
Date: Sun, 2 Nov 2008 12:15:39 +0100

Hi Jan,

I cannot find any comment from you on the list concerning
draft-ietf-dime-diameter-api neither during the month of July or during
the IETF Last Call. If I missed something please point me to your
messages. It certainly does not suffices to just say 'I am in
disagreement' - you need to provide detailed comments and arguments
about what aspects (technical or editorial) you find as problematic.
Please try to do so as soon as possible, as this document is planned to
be submitted for review and approval to the IESG after the publication
of the I-D revision following the IETF Last Call.   

Dan

> -----Original Message-----
> From: [email-masked] [mailto:[email-masked]] On 
> Behalf Of Jan Engelhardt
> Sent: Saturday, November 01, 2008 6:23 AM
> To: [email-masked]
> Cc: [email-masked]
> Subject: [Dime] diameter-api draft comparison
> 
> 
> I have monitored this list before but I did not quite see a 
> reason for taking part until I finished our implementation of 
> Diameter and WebAuth.
> 
> I see that my earlier disagreement mail about
> draft-ietf-dime-diameter-api-06 from July has not been 
> forwarded here; but it probably suffices to say that I am at 
> a disagreement with that draft, both from a technical 
> standpoint, and not less from a cosmetical one. While 
> camelcase is something one can debate infinitely, I have to 
> condemn the silly use of typedefs and long names. Some 
> specified functions like AAAValueFromAVPCode do not make any 
> sense at all.
> 
> Our API, which actually goes in line with an actual 
> implementation around it, is described in its Developers 
> Guide; a web copy is at 
> http://circum.sf.net/circum_devguide.pdf . I especially 
> encourage the draft's authors to compare.
> _______________________________________________
> DiME mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/dime
> 
_______________________________________________
DiME mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/dime

---

From: Jan Engelhardt <[email-masked]>
Date: Sun, 2 Nov 2008 19:34:01 +0100 (CET)

On Sunday 2008-11-02 12:15, Romascanu, Dan (Dan) wrote:

>It certainly does not suffices to just say 'I am in disagreement' - you 
>need to provide detailed comments and arguments about what aspects 
>(technical or editorial) you find as problematic.

Following is quoting the api-07 text document.

>The callbacks implement the application profile processing for
>incoming messages.  For outgoing calls, the C API provides an
>asynchronous model, leaving processing of the return message to the
>callbacks.

This decision has *really* puzzled me. It seems like you are trying
to put in server-specific stuff into the base library, and this
attempt looked wrong to me. Well after rereading, asynchronous
processing seems to have a place. But I still think it is not that
appropriate.

1. You have to create a function for every command you are interested
in.

2. For a server, asynchronous-ity may make sense, but for clients,
this is less of a case. The fact that our course's implementation
fares perfectly well with synchronous exchange. Especially since I
do not see what a client is supposed to asynchronously do after
having sent a packet and waiting for a reply.

3. Callbacks as used in the draft kill any "thread safety variety".
Consider:

	bar() {
		unregister_cb(command=foo)
		do_something..
	}

	/* see to it that we get notified of foo */
	register_cb(command=foo, ptr=bar);
	send_packet();

The moment you register a CB means that the CB will get triggered
whenever a packet happens to come in. This is bad when you only
want to process a "foo" for exactly one specific packet.
(Noting that the above pseudocode snippet is totally MT-race unsafe.)

>For the most part, the API hides the details of establishing peering
>and redirect connections, parsing and creating Diameter messages,
>and other work necessary to set up and maintain a redirect or
>peering session.  The application profile code need only be
>concerned with processing of the AVPs defined in the application
>profile.

I would rather want to have control over connection establishment,
i.e. when it happens. This seems to be absent from this draft.

>2.3.  String Format
>
>C API clients are required to format strings as UTF-8 if the string
>contains 16 bit characters.  Since the ASCII characters and the
>UTF-8 8 bit characters have the same codes, ASCII can be used for
>UTF-8 if no wide characters are in the string.  All strings passed
>through the C API are standard null-terminated C strings. 
>Processing to remove the null terminator for transmission on the
>wire is done by the library.

This sounds bogus. C strings (in common systems at least) never are
16 bit (but more like CHAR_BIT=8). So if my string is in ISO-8859-15
encoding, I do not have any 16-bit characters in it, and per the
draft, I am not required to format as UTF-8. — See?

>3.1.  Constant Types
>
>3.1.1.  IP Address and Port
>
>   typedef struct sockaddr_storage AAA_IP_ADDR;
>
>   AAA_IP_ADDR provides a way of referring to an IPv4 address, IPv6
>   address, and IP port.  The default implementation (shown here) is
>   defined in the Basic Socket Interface Extensions for IPv6 [RFC3493]

Typedefs are IMHO pointless for simple types (when they do not convey a
special purpose like the Linux kernel's pmd_t), and structs. It also creates
unnecessary compilation delays.

See http://lkml.org/lkml/2006/11/21/34

(Function pointers are not considered "simple".)

Just write "struct sockaddr_storage".

If the name should be AAA_IP_ADDR, either use "struct AAA_IP_ADDR" or write
C++/Java instead. Do not try to be smart in hiding types - C is not meant for
this. Just because one is allowed to do whatever strange things can be done
with C does not mean it should be done.

(The AAA prefix is... mh - could not have come up with something
better?)

Also this camel-case is something you would see in C++-, Java- or
Microsoft-centric stuff. All the good ones, many a Linux, BSD and
Solaris code does without that. When we encounter AAA_AVP, I almost
thinking I get screamed at; it is upper case letters throughout —
very reminiscient of legal disclaimers and <windows.h> source.

Together with the typedefs this is the main reason I would never want
to use this API. Code like that just ain't readable for me.

>3.1.5.  Attribute/Value Pair Code
>
>   typedef uint32_t AAA_AVPCode;

What is the point here? If the typedef is used because you think you
could transparently change the underlying implementation, then this
is a wrong belief. The ABI would change when moving to e.g. uint64_t,
and the required access syntax changes when moving to e.g. a struct.

The naming scheme is inconsistent at best. Underscore goes between
AAA and AVP (also in other places like enum AAA_AVPFlag), but not for
AAA_Server?

>3.1.7.  Value Type Identifier
>
>   typedef void AAAServer;
>
>   AAAServer is an identifier for a particular serving peer.  It is used
>   in the server access functions.

Such a typedef is *so* misleading. Someone who tries to declare

	AAAServer foo;

will only get "storage size of foo is not known" compiler error.
Especially considering this:

	AAAServer my_function(char *do_something)
	{
		/* return something or not? */
		/* it's just not obvious whether it is void */
	}

Because I would rather assume it was typedefed to a struct or some
integral type.

>3.1.11.  Application Identifier
>
>   typedef void* AAAApplicationId;

Pointers in typedefs are so frowned upon. Microsoft did that with
their LPCSTR typedef for example. It confuses the hell out of
everyone, since you do not know, by just looking at the name, whether
it is a pointer or not - and I'd rather assume it was the latter, and
let's say I wanted to increase the id by one:

	void foo(AAAA/*how many As was that?*/pplicationId bar)
	{
		++bar;
	}

but that would do the wrong thing instead of what I intended. Hence
please do not use indirections in typedefs (the one exception is a
function pointer where this is necessary).

>3.1.12.  API Return Codes
>
>   The following status codes are returned by functions in the AAA API:
>
>           typedef enum {
>                 AAA_ERR_NOT_FOUND =       -2,
>                 AAA_ERR_FAILURE =         -1,
>                 AAA_ERR_SUCCESS =          0,
>                 AAA_ERR_NOMEM,
>                 AAA_ERR_PROTO,

It would have been preferable to reuse errno as much as possible.

>3.1.14.  AVP Data Type Codes
>
>   The following are AVP data type codes.  They correspond directly to
>   the AVP data types outline in the Diameter specification [RFC3588]:
>
>           typedef enum {
>                AAA_AVP_STRING_TYPE,
>                AAA_AVP_ADDRESS_TYPE,
>                AAA_AVP_OCTET_STRING_TYPE,
>                AAA_AVP_INTEGER32_TYPE,
>                AAA_AVP_INTEGER64_TYPE,
>                AAA_AVP_UNSIGNED32_TYPE,
>                AAA_AVP_UNSIGNED64_TYPE,
>                AAA_AVP_FLOAT32_TYPE,
>                AAA_AVP_FLOAT64_TYPE,
>           } AAA_AVPDataType;

Where is the GROUP and the "unspecified" type — or are groups of type
STRING?

>3.1.16.  Domain Interconnection Types
>
>           typedef enum {
>              AAA_DOMAIN_LOCAL,
>              AAA_DOMAIN_PROXY,
>              AAA_DOMAIN_BROKER,
>              AAA_DOMAIN_FORWARD
>           } AAADomainInterconnect;

(Detected marketspeak intrusion ("broker"). Is not there some
research/.edu-like term for that?)

>3.1.19.  Search Direction Type
>
>   The following type allows the client to specify which direction to
>   search for an AVP in the AVP list:
>
>           typedef enum {
>              AAA_FORWARD_SEARCH = 0,
>              AAA_BACKWARD_SEARCH
>           } AAASearchType;

Though there is a particular AVP order mandated by the RFC, this
exposes implementation internals — something to be usually avoided.

>3.2.1.
>
>           typedef struct dictionaryEntry {
>              AAA_AVPCode     avpCode;
>              char*           avpName;
>              AAA_AVPDataType avpType;
>              AAAVendorId     vendorId;
>              AAA_AVPFlag     flags;
>           } AAADictionaryEntry;

If you want to hide the fact that something is of a particular type,
do not use C. That being said, *iff* you go down the typedef route,
why give the struct a name? It creates confusion on the developers -
some will use "struct dictionaryEntry" in their programs, some will
use "AAADictionaryEntry", and what you have gained is a double-sided
API, and a potential name clash in the struct namespace. And we all
agree that an API should only be "as big as necessary but as small as
possible", should not it? Not to mention "dictionaryEntry" is real
namespace pollution.

>3.2.2.  AVP Definition
>
>   The following structure contains a message AVP in parsed form:
>
>            typedef struct avp {
>                    enum {
>                            AAA_RADIUS,
>                            AAA_DIAMETER
>                    } packetType;
>                    AAA_AVPCode code;
>                    uint16_t length;

<sarcasm>Oh noes. An (AAA-provided) typedef is
missing for "length"!</sarcasm>

>3.2.3.  AVP List
>            typedef struct avpList{
>                    AAA_AVP *head;
>                    AAA_AVP *tail;
>            } AAA_AVP_LIST;

All-uppercase is commonly used for preprocessor macros only.
(Perhaps "struct aaa_avp_list" was intended instead!?)

>3.3.  Macros and Preprocessor Definitions
>
>   The following definition reserves the vendor id of 0:
>
>   #define AAA_NO_VENDOR_ID 0

Use an enum { AAA_NO_VENDOR_ID = 0 } instead?

>3.4.2.1.  AAARegisterCommandCallback()
>
>   The following function is used to register command callbacks for
>   processing AAA commands:
>
>         AAACallbackHandle *
>         AAARegisterCommandCallback(AAACommandCode commandCode,
>                                    AAAVendorId vendorId,
>                                    char *commandName,
>                                    AAAExtensionId extensionId,
>                                    AAACallback callback,
>                                    AAACallbackLocation position);

spaghetti-long function names (and types too).

Why do not you use const char *? Are libraries supposed to edit
the command name? That means caller code will have to cast away
constness warnings, and eh, casts itself are bad.

Adding a position is no good, because out-of-order callers might
insert CBs that steal the message. And if not that, there is
a dependency of third-party on the particular position used. How to
say; your libfoo uses reg_cb(pos=10), then the 3rd party libbar will
use reg_cb(pos=9) to get its job done, and then the libfoo developer
decides to use pos=5 instead and everything falls apart because
libbar is still at pos=10 and, let's say, cannot go to pos=4 because
it would also create problems for libbar.

>3.4.2.2.  AAARegisterNoncommandCallback()
>
>   The following callback registers an AAA callback to process all
>   messages.  The callback is not associated with any command, but
>   rather will process all messages as they come down the callback
>   chain:
>
>         AAACallbackHandle
>         AAARegisterNoncommandCallback(AAACallback callback,

Why not reuse aaaregcb with a command number of 0?

>3.4.3.7.  AAAGetDomainInterconnectType()
>
>   The following function returns the domain interconnect type for a
>   particular domain name and type of service:
>
>         AAAResultCode
>         AAAGetDomainInterconnectType(AAAMessage *message,
>                                      char *domainName,
>                                      char *type);

AAAReturnCode --- AAAResultCode, this is a 77% similarity, and
something easily overseen.

>3.4.4.6.  AAAGetCommandCode()
>
>   This function returns the command code and vendor id based on a
>   string:
>
>         boolean_t
>         AAAGetCommandCode(char *commandName,
>                           AAACommandCode *commandCode,
>                           AAAVendorId *vendorId);

I am almost crying when I see this. boolean_t is not even a known
type — in C at least. And I remember this draft was for C, was not
it?

>3.4.5.15.  AAAGetPrevAVP()
>
>   This function returns a pointer to the previous AVP in the list:
>
>         AAA_AVP *
>         AAAGetPrevAVP(AAA_AVP *avp);

Hypothesis: This API is overdesigned.

Overdesign is when a function is added that is _thought_ might be
useful, but actually is not. Unless there is empirical data that
shows there ARE actually users of this function (and just two won't
cut it), it is just bloat.

Also, there is no member in the struct^W, pardon-moi, typedef, that
points to the previous AVP in a list. The same goes for
aaa_get_next_avp. So there is no way to even implement this with the
given draft. Again I looked at SUN's prototype from 2002 and
overlaying it with AAA_XAVP is undescribably ugly and calls for
segfaults.

>3.4.3.1.  AAAStartSession()
>
>   The following function allows a client to start a session and
>   identify it:
>
>         AAAReturnCode
>         AAAStartSession(AAASessionId **sessionId,
>                         AAAApplicationId appHandle,
>                         char *userName,
>                         AAACallback abortCallback);

Why the user name? I do not see it being required by the RFC.

>3.4.3.2.  AAARegisterPeerSession()

It is not clear in what order any of these functions are supposed to
be called.

>3.4.3.5.  AAALookupServer()
>
>The function looks up the AAA server based on IP address and port
>number. Server connections are created from the configuration file:
>
>AAAServer *AAALookupServer(AAA_IP_ADDR ipAddr);
>
>The return value is either a valid server pointer or the NULL if
>the server can't be found.

"Server pointer" is too sparse. It's almost like I could return
&ipaddr again. Also, there is no way to determine what l3proto
the ipaddr is supposed to. (There is, but that seems to be an
implementation detail of libsock.)

And why would a server lookup be needed? The ip address is enough to
connect to it.

>3.4.4.4.  AAAValueFromAVPCode()
>
>This function looks up an AVP value using the AVP id and vendor
>id, and the value name:
>
>AAAValue AAAValueFromAVPCode(AAA_AVPCode avpCode,
>                             AAAVendorId vendorId, char *valueName);

I even had to look into SUN's year-old freediameter to figure out
just what this was supposed to do. This should carry the word AVP
"enum" somewhere at least.

>3.4.4.6.  AAAGetCommandCode()
>         boolean_t [...]
>   The return value is _B_TRUE if the command was found.

There it is again. *There is no boolean_t and no _B_TRUE* in the C
standard!

>3.4.5.1.  AAANewMessage()
>AAAMessage *AAANewMessage(AAACommandCode commandCode,
>                          AAAVendorId vendorId,
>                          AAASessionId *sessionId,
>                          AAAExtensionId extensionId,
>                          AAAMessage *request);
>extensionId:  The extension identifier.

What is an extension identifier?

>3.4.5.2.  AAAFreeMessage()
>
>This function frees a message allocated through AAANewMessage():
>
>AAAReturnCode AAAFreeMessage(AAAMessage **message);
>
>message: A pointer to a pointer to the allocated message.

Just use a single pointer like free() does. The same applies to
aaa_free_avp.

>3.4.5.16.  AAAConvertAVPToString()
>
>This function converts the data in the AVP to a format suitable for
>log or display functions.
>
>char *AAAConvertAVPToString(AAA_AVP *avp, char *dest, size_t destLen);
>
>The return value is the passed in destination buffer.

The return value — which is a "long"-sized pointer (i.e. 2, 4 or 8
bytes, depending on compiler mode). And this pointer is passed in the
destination buffer, uhm, so that sounds like, to get my return value,
I need to use:

	myreturnvalue = *(char **)dest;

Then, what is the return value good for if it is returned in the
buffer anyway.

>3.4.6.1.  AAASetServer()
>
>This function sets the server to which the message is sent:
>
>AAAReturnCode AAASetServer(AAAMessage *message, AAA_IP_ADDR host);

Somehow I feel that the destination for a message is not a message
property... What if I want to send the same message to multiple
destinations? I would have to

	setserver(msg, "localhost");
	my_send(msg);
	setserver(msg, "remotehost");
	my_send(msg);

but it is much smarter to not have it as a per-message property, IOW:

	my_send(msg, "localhost");
	my_send(msg, "remotehost");

For added win, I can even call my_send() asynchronously and have them
run concurrently, e.g. by spawning extra threads, without having to
worry about serialization issues. With the broken AAASetServer, one
would have to.

Preferably though, I have two previously-established connections:

	struct conn *c1, *c2; /* assume initialized */
	send(c1, msg);
	send(c2, msg);

which would IMHO be a sane approach.

>ipVersion: The version number of the IP address.

This parameter does not even appear in the function signature.

>AAA_ERR_NOT_FOUND: If the server was not found.

There is a lack of ECONNREFUSED: server found but connection refused.

>3.6.  Grouped AVPs
>
>In order to create grouped AVPs, an application creates an
>AAA_AVP_LIST that is not attached to an AAAMessage structure (also
>known as an orphaned AAA_AVP_LIST). All of the necessary AVPs within
>the Group are added to the orphaned AAA_AVP_LIST using the existing
>list manipulation functions. Lastly, the grouped AVP is added to the
>AAAMessage structure.
>
>The following is an example that adds a Proxy-State Grouped AVP to
>an existing AAAMessage structure.
>
>addProxyState(AAAMessage *message, ipaddr_t *ourAddress,
>    void *state, size_t stateLen)

Missing return type.

>{
>        AAA_AVP_LIST *avpList = NULL;
>
>        if (AAACreateAndAddAVPToList(&avpList,
>            DIAM_AVP_PROXY_ADDRESS, AAA_AVPI_FLAG_NONE,
>            NO_VENDOR_ID, (char *) ourAddress,
>            sizeof (ipaddr_t))) {
>                return (AAA_ERR_FAILURE);
>        }
>        if (AAACreateAndAddAVPToList(&avpList,
>            DIAM_AVP_PROXY_INFO, AAA_AVPI_FLAG_NONE,
>            NO_VENDOR_ID, state, stateLen)) {
>                return (AAA_ERR_FAILURE);
>        }
>        if (AAACreateAndAddAVPToList(&message->avpList,
>            DIAM_AVP_PROXY_STATE, AAA_AVPI_FLAG_NONE,
>            NO_VENDOR_ID, (char *)avpList,
>            AAA_AVP_GROUPED_LENGTH)) {
>                return (AAA_ERR_FAILURE);
>        }
>
>        return (AAA_ERR_SUCCESS);
>}

What is AAA_AVP_GROUPED_LENGTH? The length usually depends on the
group, you cannot use any static number. Not when it is recorded
as a AAA_AVPTYPE_STRING.

What is AAA_AVPI_FLAG_NONE? It is undocumented.

What is DIAM_AVP_PROXY_STATE? Also undocumented.

And NO_VENDOR_ID? Probably should be AAA_NO_VENDOR_ID.

Just to put that into contrast, this is libcircum (advertised as
being cleaner and using less screaming letters):

bool add_proxy_state(struct a3_packet *msg, const ipaddr_t *ouraddr,
    const void *state, unsigned int statelen)
{
	struct a3_avp *avp =
		a3_pkt_add_avp(msg, "Proxy-Info", NULL, 0);
	if (avp == NULL)
		return false;

	if (a3_pkt_add_avp(avp, "Proxy-Host", ouraddress, sizeof(ipaddr_t)) == NULL)
		return false;
	if (a3_pkt_add_avp(avp, "Proxy-State", state, statelen) == NULL)
		return false;
	return true;
}
_______________________________________________
DiME mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/dime

---

From: "Glen Zorn" <[email-masked]>
Date: Sun, 2 Nov 2008 14:06:45 -0800

Jan Engelhardt [mailto://[email-masked]] writes:

...

> having sent a packet and waiting for a reply.

Diameter IS NOT a client/server protocol; the terms are used only as a convenience.  If you are writing code that assumes that it _is_ a client-server protocol, then a) you're going to a lot more trouble than you need to & b) your design is by definition broken since it is based upon invalid assumptions (irrespective of whether it seems to "work" in certain limited situations).

...

---

From: Jan Engelhardt <[email-masked]>
Date: Mon, 3 Nov 2008 07:34:48 +0100 (CET)

On Sunday 2008-11-02 23:06, Glen Zorn wrote:
>Diameter IS NOT a client/server protocol; the terms are used only as
>a convenience. 

So what IS it? Some freeform packet flooder like netbios broadcasts?
What I want to point out here is that with CBs, it seems impossible
to match an incoming message to its previously corresponding message,
at least in cases like AA.
_______________________________________________
DiME mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/dime

---

From: "Romascanu, Dan (Dan)" <[email-masked]>
Date: Mon, 3 Nov 2008 10:51:01 +0100

Jan,

Thank you for your input. FYI, I am the co-area director of the OPS area responsible for the DIME WG, and not an editor of this document. Frankly it would have been better for such detailed review level and comments to be submitted earlier in the process and not three months after the IETF Last Call, but we shall not be too formal here. I will not submit this document to the IESG review yet and will provide the document editors and the rest of the working group the opportunity to review and discuss your comments. 

Dan

> From: [email-masked] 
> [mailto:[email-masked]] On Behalf Of Jan Engelhardt
> Sent: Sunday, November 02, 2008 8:34 PM
> To: Romascanu, Dan (Dan)
> Cc: [email-masked]; [email-masked]
> Subject: RE: [Dime] diameter-api draft comparison
> 
> 
> >The callbacks implement the application profile processing 
> for incoming 
> >messages.  For outgoing calls, the C API provides an asynchronous 
> >model, leaving processing of the return message to the callbacks.
> 
> This decision has *really* puzzled me. It seems like you are 
> trying to put in server-specific stuff into the base library, 
> and this attempt looked wrong to me. Well after rereading, 
> asynchronous processing seems to have a place. But I still 
> think it is not that appropriate.
> 
> 1. You have to create a function for every command you are 
> interested in.
> 
> 2. For a server, asynchronous-ity may make sense, but for 
> clients, this is less of a case. The fact that our course's 
> implementation fares perfectly well with synchronous 
> exchange. Especially since I do not see what a client is 
> supposed to asynchronously do after having sent a packet and 
> waiting for a reply.
> 
> The moment you register a CB means that the CB will get 
> triggered whenever a packet happens to come in. This is bad 
> when you only want to process a "foo" for exactly one specific packet.
> >and redirect connections, parsing and creating Diameter 
> messages, and 
> >other work necessary to set up and maintain a redirect or peering 
> >session.  The application profile code need only be concerned with 
> >processing of the AVPs defined in the application profile.
> 
> I would rather want to have control over connection 
> establishment, i.e. when it happens. This seems to be absent 
> from this draft.
> 
> >2.3.  String Format
> >
> >Processing to remove the null terminator for transmission on 
> the wire 
> >is done by the library.
> 
> 16 bit (but more like CHAR_BIT=8). So if my string is in 
> ISO-8859-15 encoding, I do not have any 16-bit characters in 
> it, and per the draft, I am not required to format as UTF-8. — See?
> 
> >3.1.  Constant Types
> >
> Typedefs are IMHO pointless for simple types (when they do 
> not convey a special purpose like the Linux kernel's pmd_t), 
> and structs. It also creates unnecessary compilation delays.
> 
> C++/Java instead. Do not try to be smart in hiding types - C is not 
> C++meant for
> this. Just because one is allowed to do whatever strange 
> things can be done with C does not mean it should be done.
> 
> Also this camel-case is something you would see in C++-, 
> Java- or Microsoft-centric stuff. All the good ones, many a 
> Linux, BSD and Solaris code does without that. When we 
> encounter AAA_AVP, I almost thinking I get screamed at; it is 
> upper case letters throughout — very reminiscient of legal 
> disclaimers and <windows.h> source.
> 
> Together with the typedefs this is the main reason I would 
> never want to use this API. Code like that just ain't readable for me.
> 
> What is the point here? If the typedef is used because you 
> think you could transparently change the underlying 
> implementation, then this is a wrong belief. The ABI would 
> change when moving to e.g. uint64_t, and the required access 
> syntax changes when moving to e.g. a struct.
> 
> The naming scheme is inconsistent at best. Underscore goes 
> between AAA and AVP (also in other places like enum 
> AAA_AVPFlag), but not for AAA_Server?
> 
> Because I would rather assume it was typedefed to a struct or 
> some integral type.
> 
> Pointers in typedefs are so frowned upon. Microsoft did that 
> with their LPCSTR typedef for example. It confuses the hell 
> out of everyone, since you do not know, by just looking at 
> the name, whether it is a pointer or not - and I'd rather 
> assume it was the latter, and let's say I wanted to increase 
> the id by one:
> 
> but that would do the wrong thing instead of what I intended. 
> Hence please do not use indirections in typedefs (the one 
> exception is a function pointer where this is necessary).
> 
> >3.1.12.  API Return Codes
> >
> Where is the GROUP and the "unspecified" type — or are groups 
> of type STRING?
> 
> Though there is a particular AVP order mandated by the RFC, 
> this exposes implementation internals — something to be 
> usually avoided.
> 
> >3.2.1.
> >
> If you want to hide the fact that something is of a 
> particular type, do not use C. That being said, *iff* you go 
> down the typedef route, why give the struct a name? It 
> creates confusion on the developers - some will use "struct 
> dictionaryEntry" in their programs, some will use 
> "AAADictionaryEntry", and what you have gained is a 
> double-sided API, and a potential name clash in the struct 
> namespace. And we all agree that an API should only be "as 
> big as necessary but as small as possible", should not it? 
> Not to mention "dictionaryEntry" is real namespace pollution.
> 
> >3.2.2.  AVP Definition
> >
> <sarcasm>Oh noes. An (AAA-provided) typedef is missing for 
> "length"!</sarcasm>
> 
> >3.2.3.  AVP List
> >            typedef struct avpList{
> >                    AAA_AVP *head;
> >                    AAA_AVP *tail;
> >            } AAA_AVP_LIST;
> 
> Why do not you use const char *? Are libraries supposed to 
> edit the command name? That means caller code will have to 
> cast away constness warnings, and eh, casts itself are bad.
> 
> Adding a position is no good, because out-of-order callers 
> might insert CBs that steal the message. And if not that, 
> there is a dependency of third-party on the particular 
> position used. How to say; your libfoo uses reg_cb(pos=10), 
> then the 3rd party libbar will use reg_cb(pos=9) to get its 
> job done, and then the libfoo developer decides to use pos=5 
> instead and everything falls apart because libbar is still at 
> pos=10 and, let's say, cannot go to pos=4 because it would 
> also create problems for libbar.
> 
> AAAReturnCode --- AAAResultCode, this is a 77% similarity, 
> and something easily overseen.
> 
> I am almost crying when I see this. boolean_t is not even a 
> known type — in C at least. And I remember this draft was for 
> C, was not it?
> 
> Overdesign is when a function is added that is _thought_ 
> might be useful, but actually is not. Unless there is 
> empirical data that shows there ARE actually users of this 
> function (and just two won't cut it), it is just bloat.
> 
> Also, there is no member in the struct^W, pardon-moi, 
> typedef, that points to the previous AVP in a list. The same 
> goes for aaa_get_next_avp. So there is no way to even 
> implement this with the given draft. Again I looked at SUN's 
> prototype from 2002 and overlaying it with AAA_XAVP is 
> undescribably ugly and calls for segfaults.
> 
> It is not clear in what order any of these functions are 
> supposed to be called.
> 
> >The return value is either a valid server pointer or the NULL if the 
> >server can't be found.
> 
> "Server pointer" is too sparse. It's almost like I could 
> return &ipaddr again. Also, there is no way to determine what 
> l3proto the ipaddr is supposed to. (There is, but that seems 
> to be an implementation detail of libsock.)
> 
> And why would a server lookup be needed? The ip address is 
> enough to connect to it.
> 
> >This function looks up an AVP value using the AVP id and 
> vendor id, and 
> >the value name:
> >
> I even had to look into SUN's year-old freediameter to figure 
> out just what this was supposed to do. This should carry the 
> word AVP "enum" somewhere at least.
> 
> There it is again. *There is no boolean_t and no _B_TRUE* in 
> the C standard!
> 
> >3.4.5.1.  AAANewMessage()
> Just use a single pointer like free() does. The same applies 
> to aaa_free_avp.
> 
> >This function converts the data in the AVP to a format 
> suitable for log 
> >or display functions.
> >
> The return value — which is a "long"-sized pointer (i.e. 2, 4 
> or 8 bytes, depending on compiler mode). And this pointer is 
> passed in the destination buffer, uhm, so that sounds like, 
> to get my return value, I need to use:
> 
> Then, what is the return value good for if it is returned in 
> the buffer anyway.
> 
> >3.4.6.1.  AAASetServer()
> >
> Somehow I feel that the destination for a message is not a 
> message property... What if I want to send the same message 
> to multiple destinations? I would have to
> 
> For added win, I can even call my_send() asynchronously and 
> have them run concurrently, e.g. by spawning extra threads, 
> without having to worry about serialization issues. With the 
> broken AAASetServer, one would have to.
> 
> AAA_AVP_LIST 
> >that is not attached to an AAAMessage structure (also known as an 
> >orphaned AAA_AVP_LIST). All of the necessary AVPs within the 
> Group are 
> >added to the orphaned AAA_AVP_LIST using the existing list 
> manipulation 
> >functions. Lastly, the grouped AVP is added to the AAAMessage 
> >structure.
> >
> >The following is an example that adds a Proxy-State Grouped 
> AVP to an 
> >existing AAAMessage structure.
> >
> What is AAA_AVP_GROUPED_LENGTH? The length usually depends on 
> the group, you cannot use any static number. Not when it is 
> recorded as a AAA_AVPTYPE_STRING.
> 
> Just to put that into contrast, this is libcircum (advertised 
> as being cleaner and using less screaming letters):
> 
>     const void *state, unsigned int statelen) {
> 	struct a3_avp *avp =

---

From: "Glen Zorn" <[email-masked]>
Date: Mon, 3 Nov 2008 14:26:26 -0800

Jan Engelhardt [mailto://[email-masked]] writes:

> So what IS it? 

Diameter is a peer-to-peer protocol.

> Some freeform packet flooder like netbios broadcasts?

What's wrong w/the Hop-by-Hop Identifier?  BTW, with the exception of this
minor conceptual faux pas, you comments WRT this draft are the first I
recall seeing from somebody actually writing code & are very welcome (though
a little late).

...

---

From: Jan Engelhardt <[email-masked]>
Date: Tue, 4 Nov 2008 00:29:48 +0100 (CET)

On Monday 2008-11-03 23:26, Glen Zorn wrote:
>>[ Some freeform packet flooder like netbios broadcasts? ]
>

I take that as a yes.

>What's wrong w/the Hop-by-Hop Identifier?

Thinking about this, P2P is not truly telling here. P2P merely says
that every node can/will randomly connect to another. Because TCP is
stateful, one can be sure that messages received on a socket
descriptor always come from the same peer. Furthermore, the RFC does
not seem to say anything about (a)synchronous operation, whether 1.
messages may be processed async or must be processes in sync at the
remote side, and 2. whether, independent of (1), replies must be in
the same order as the requests/may be reordered.

So irrespective of the P2P nature, I interpreted the RFC as assuming
synchronous processing and ordering within one connection. — Which
lead to the HBH looking like a redundant field because order is
guaranteed by the stream protocol. (It may have made sense for
RADIUS/UDP or something, but that was not part of the initial
impl.)

>BTW, with the exception of this minor conceptual faux pas, you[r]
>comments WRT this draft are the first I recall seeing from somebody
>actually writing code & are very welcome (though a little late).

I think that is a problem with many a draft/RFC, that there is no
impl. by an external entity that has not otherwise been involved in
the process of crafting the draft/RFC and/or first prototypes.

Diameter seems to go under everyone's radar — it has been around for
years but implementations are on the rare side.
_______________________________________________
DiME mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/dime

---

From: Sebastien Decugis <[email-masked]>
Date: Tue, 04 Nov 2008 12:11:32 +0900

Hello,

I would like to add some precisions here (inlined).

Thanks,
Sebastien.

> RADIUS/UDP or something, but that was not part of the initial
In my understanding, the nature of the messages themselves will enforce
the ordering of the messages: one answer for one request. Anyway,
messages related to different user sessions may be sent concurrently
(especially using multi-stream SCTP) and the answers may not be received
in the same order as the requests. I think the Hop-by-hop ID is far more
efficient way to match an incoming answer with the corresponding sent
request for example at a relay level. I guess that this ID should not be
removed.

About the peer-to-peer Vs client-server model, Diameter is a
peer-to-peer protocol but I believe that most (all?) of the Diameter
applications are based on client-server models. This is probably related
to the nature of AAA itself...

> years but implementations are on the rare side.
For the record, I am also working on a Diameter implementation (
http://aaa.koganei.wide.ad.jp/ ) and I sent a list of comments on the
Diameter API earlier in this list:
http://www.ietf.org/mail-archive/web/dime/current/msg02460.html

This new implementation is not based on the Diameter API, mainly because
that API does not allows to implement a Diameter agent (Proxy, Redirect,
...) and focuses on easing transition from RADIUS which may not be a
good thing IMHO.

Best regards,
Sebastien.

---

From: Jan Engelhardt <[email-masked]>
Date: Tue, 4 Nov 2008 07:52:08 +0100 (CET)

>http://www.ietf.org/mail-archive/web/dime/current/msg02460.html

Mh, some random comments: lots of code, lots of hardcoded data,
values instead of constants, casting malloc's result (can lead to
crashes; I have no idea what many a C book author smoked), redundant
casts, comments that... (No way would I have guessed that
pthread_mutex_destroy destroys the mutex!)

	/* Destroy the mutex */ 
	CHECK_POSIX(    pthread_mutex_unlock( &item->lock )     ); 
	CHECK_POSIX(    pthread_mutex_destroy( &item->lock )    ); 

and for starters:
	make[3]: *** No rule to make target `peer-dpr_dpa.o',
	needed by `waaad'.  Stop.

Still has a way to go.

---

From: Sebastien Decugis <[email-masked]>
Date: Tue, 04 Nov 2008 16:22:59 +0900

> 	make[3]: *** No rule to make target `peer-dpr_dpa.o',
Sure, I never claimed the implementation was complete yet ^^ It's a work
in progress!

About your comments:
- lots of code.
=> can you implement Diameter without having lot of code? You're *very*
good then!

- lots of hardcoded data.
=> can you give precise examples? Except from the dictionary definitions
of the base protocol I mean... I saw that the argument for your
implementation is that you resolve the command code and AVP code from
the name; this can also be done with this implementation. One could
argue that the AVP name *is* hardcoded in any case...

- values instead of constants.
=> there may remain a few such places but I don't think there are
many... Can you point me to these places so that I fix it?

- casting malloc results.
=> Wow, for my culture, can you explain why this is not recommended??? I
never heard this before...

- redundant casts.
=> Why is this a problem?

- too many comments .
=> Well I believe that commented code makes it easier to read.
Especially, it makes it easy to extract the pseudo-code for any
function. But if you prefer to go without comments, it's quite easy to
remove...

- compilation error:
=> bad timing, between two commits... Anyway, as said previously, this
is a work in progress and not expected to work properly yet.
Anyway, if some people want to help the project by contributing (or even
criticizing, as long as it is constructive...) please contact me!

Bests,
Sebastien.

-- 
Sebastien Decugis
Research fellow
Network Architecture Group
NICT (nict.go.jp)

---

From: Jan Engelhardt <[email-masked]>
Date: Tue, 4 Nov 2008 08:58:42 +0100 (CET)

>of the base protocol I mean...

Yes it was the dictionary definitions.

>I saw that the argument for your

The name yes, but 

>many... Can you point me to these places so that I fix it?

dict-base.c:#if ER_DIAMETER_SUCCESS != 2001

Not sure if that qualifies, but I am getting suspicious whenever
I see any such values in .c files. Having it more than once can
possibly lead to an update anomaly, as database people would call it.

>=> Why is this a problem?

1. Because they are, well, redundant.

2. Assume typeof(x) is already of T.
	y = (T)x;
   Now assume that during the lifetime of the code, typeof(x)
   changes. The compiler would normally warn you if typeof(x) cannot
   be implicitly converted to typeof(y), but you made the compiler go
   STFU by use of a cast.

3. Casting to silence the compiler because of the developer's laziness 
   to include a header for a prototype incurs a truncation of the 
   function's return value, leading to unspecified behavior. 
   http://jengelh.medozas.de/2007/0616-nocast.php (example code)
   All C basics really.

>function. But if you prefer to go without comments, it's quite easy to

I did not say without-comments. But it is best explained for example here
http://stackoverflow.com/questions/121945/how-do-you-like-your-comments-best-practices
_______________________________________________
DiME mailing list
[email-masked]
https://www.ietf.org/mailman/listinfo/dime

---

From: Sebastien Decugis <[email-masked]>
Date: Tue, 04 Nov 2008 17:20:38 +0900

Thank you for your detailed comments. I will continue the discussion in
a separate mail since this might be boring the people from the mailing
list...

Sebastien.