[Nice] New API and refactor

[Youness Alaoui]

Hi,

As some of you know, I'm working on a refactor of libnice. I'm not sure 
yet whether the external API will change much or not, but the internal 
API and code needs major refactoring.

The way I currently see it is : modular, layered, simple and beautiful!

There are of course multiple angle at which we could start designing the 
code, I currently want to concentrate on the modularity of libnice, 
mainly, have the STUN part of it as a completely separate module.
Here are two possible layering designs :

First one :
  APP
    |
    v
  ICE -> STUN
    |
    v
Socket


Second one :
  APP
    |
    v
  ICE
    |
    v
STUN
    |
    v
Socket


let me be clear, do we want to have the ICE agent use the STUN library 
to build messages, then send them on the socket, and when it receives a 
message, try to parse it with the STUN library to see if it's a valid 
stun message, then handle it, otherwise, give it to the application 
(first design)...
Or do we want to have the ICE agent build messages using the STUN 
library, then tell the lib to send the message, and when it receives a 
message, the STUN library handles it, if it's a stun message, it will 
tell the ICE agent that it has an inbound STUN message, if it's not, 
then it tell the ICE agent that a data message was received.
In other words, should the STUN library be put between the socket and 
the ICE agent, or should it only be *used* by the ICE agent to build and 
parse messages.

Now, for the first layering design, I have two different API designs 
which I will first explain :

Since I am better with code than words, let's talk C instead...
I'm currently looking exclusively at the STUN library and API, and 
here's what I have so far for a design (incomplete) :

struct StunMessage {
   guint16 type;
   guint8 id[16];
   GList *attributes; /* StunAttribute * list */
}
struct StunAttribute {
   guint16 type;
   guint16 size;
   guint8 *value;
}

Sending :
StunMessage * msg = stun_message_new(STUN_RFC3489BIS);
stun_message_add_attribute_uint32 (msg, STUN_USE_CANDIDATE, use_cand);
stun_message_add_attribute (msg, STUN_ATTRIBUTE_USERNAME, username, 
strlen(username));
stun_message_add_integrity(msg, password, 
USE_SHORT_TERM_CREDENTIALS_MECHANISM);
stun_message_add_fingerprint(msg);
gchar *buffer = stun_message_pack(msg, &len);
stun_message_free(msg);
sendto(..., buffer, ...);
g_free(buffer);

Receiving :
static const guint16 attribute_list[] = { STUN_USE_CANIDATE, 
STUN_ICE_CONTROLLING, ..., NULL}; /* List of comprehension-attributes 
supported by the client of the lib */
StunMessage *msg = stun_message_create(attribute_list, buffer, len); /* 
NULL if not a valid stun message */
stun_message_has_attribute(msg, STUN_ATTRIBUTE_FINGERPRINT);
gboolean valid = stun_message_check_fingerprint(msg);
gboolean got_attr = stun_message_get_attribute_uint32(msg, 
STUN_ICE_CONTROLING, &value);


my issue is that I would like to have a StunAgent added, so we would 
create one StunAgent with all our parameters (comprehension-attributes 
that we support, RFC compliance (3489 or 3489bis), extensions to use 
(FINGERPRINT), mechanisms (authentication/integrity mechanism to use : 
long-term or short-term), etc... and we would use that agent to handle 
all our stun needs, like for example :
StunAgent *stun = stun_agent_new (STUN_RFC3489BIS, attribute_list, 
USE_FINGERPRINT_EXTENSION, USE_SHORT_TERM_CREDENTIALS_MECHANISM ...);
stun_agent_set_credentials(agent, username, password);

Sending :
StunMessage *msg = stun_agent_new_message (stun);
stun_message_add_attribute_uint32 (msg, STUN_USE_CANDIDATE, use_cand);
gchar *buf = stun_message_pack (msg, &len);

Receiving :
StunMessage *msg = stun_agent_parse_message (stun, buffer, len);
gboolean got_attr = stun_message_get_attribute_uint32(msg, 
STUN_ICE_CONTROLING, &value);



so, the first design (with no StunAgent), is the preferred design chosen 
by Olivier, while I prefer the StunAgent design
The reasoning behind Olivier's decision is (he will confirm this and 
correct me if I'm wrong) is that the stun library should be very simple 
and minimalistic and should only be used for parsing and generating stun 
messages, while I think that the behaviors defined in the STUN RFC 
should be handled by the STUN library itself (the STUN behavior defined 
in ICE would still be handled by the ICE agent) since there are 
different behaviors that are defined in the STUN RFC like the 
authentification and fingerprint behaviors. Also note that I don't want 
the ICE agent to keep track of the list of transaction ids it generated 
for the sent requests to make sure that any STUN response corresponds to 
a STUN request sent (+ any possible security related algorithms).

quickly put, the second layering design would have something like this :
StunAgent *stun = stun_agent_new (STUN_RFC3489BIS, attribute_list, 
USE_FINGERPRINT_EXTENSION, USE_SHORT_TERM_CREDENTIALS_MECHANISM ...);
stun_agent_set_credentials(agent, username, password);
stun_agent_set_inbound_stun_callback(agent, nice_inbound_stun, 
nice_agent);  /* params : stun_agent, callback, user_data */
stun_agent_set_inbound_data_callback(agent, nice_inbound_data, nice_agent);
Sending would not change, apart from stun_message_pack which would 
become stun_message_send, while receiving would only have the change of 
having the code in the nice_inbound_stun and no more need for calling 
stun_agent_parse_message since we would receive a pre-parsed 
StunMessage* as argument.

So now my question to you is : what do you think? How do you think the 
stun library should work and which design+layering do you prefer ?
Do you have some suggestions concerning the API to have?
I will write a more detailed API design (stun.h) if you think that would 
help.

I'm sorry if anything I said is unclear and please don't hesitate to ask 
for any clarifications.

Thanks,
Youness.