[Nice] New API and refactor

[Youness Alaoui]

Rémi Denis-Courmont wrote:
> Le Saturday 21 June 2008 00:13:59 ext Youness Alaoui, vous avez écrit :
>   
>> Other changes include the fact that the NONCE/REALM/USERNAME attributes
>> should be added by the user of the library, and not by the library
>> itself.. the STUN module will only add the message-integrity and the
>> fingerprint attributes if necessary.
>>
>> The main problem I see now is with the stun 'bind' and 'stun-ice'
>> usages.. those should not be in the stun module, they should be part of
>> the libnice core functions and should be a lot simpler.
>>     
>
> They used to be simple, until we found half a dozen different cases that need 
> to be handled in different ways for different reasons. The "large" number of 
> API mostly comes from the fact that we want a non-blocking API, so we have to 
> expose the timer and the transaction state (completed, failed, resending, 
> waiting for answer...) to the caller. In fact, the blocking API is way 
> simpler... but it is obviously not usable within nice/
>   
Yes, of course, the "should be simpler" wasn't much about the code 
there, but actually about the API. the transport, the timers, etc.. 
those shouldn't be in the stun module. I probably should have explained 
my views about this earlier.
I think the stun module should be as simple as possible, it should just 
be a "create and parse stun messages" library.
The bind usage should be as simple as :
stun_agent_init_request(agent, msg, STUN_BINDING);
stun_agent_finish_message(agent, msg, NULL, 0);
----
stun_agent_validate(agent, msg, buf, buf_len, NULL, NULL);
if (stun_message_find_addr(msg, STUN_ATTRIBUTE_MAPPED_ADDRESS, &addr) != 
0) {
   stun_message_find_addr(msg, STUN_ATTRIBUTE_XOR_MAPPED_ADDRESS, &addr);
}
----
That's it (a bit simplified of course), we don't really need much more 
than that, the whole transport and timer, etc.. are not something that 
should be part of a simple library like STUN, it's really the job of the 
user of the lib.
The bind usage is useful here for the stunbdc tool and I think it should 
be kept as part of that tool and that's it. Also, libnice doesn't seem 
to use much of the bind stuff, since it does everything itself, so the 
code is really only used by stunbdc.

About the ICE usage, all of that is not stun specific, it's really about 
ICE and it's specific to libnice.. if anyone wanted to create an ICE 
library and use our libstun, they will probably implement their own ICE 
usage code.. Also, having the ice usage there is not something that 
helps our modularity because the usages still have a lot of calls with 
the compat argument for gtalk compatibility.

If we want ICE and bind usages, I suggest something as simple as this :
stun_usage_ice_create(agent, msg, buffer, buffer_len, use_candidate, 
controlling, etc...)
stun_usage_ice_parse(agent, msg, &use_candidate, &controlling, etc..)
stun_usage_bind_create(agent, msg, buffer, buffer_len)
stun_usage_bind_parse(agent, msg, &addr);

and let the library using those functions pass whatever argument they 
want (with a NULL or '-1' if we don't want some attribute (controlling 
for example) to be added to the message).
Simply helper functions for the usages instead of the overly complex 
system we currently have.

> Then it is also true that, for instance, stun_conncheck_start has an horrific 
> prototype. But unless we're to create even more APIs, or expose STUN message 
> formatting upstream, we simply cannot remove them. In all three cases (more 
> API, expose STUN messages format, change nothing), I'm afraid it will remain 
> intricate because it intrinsically is.
>
>   
Well, apart from its horrific prototype (which is ok when justified), 
there's the fact that stun_conncheck_start is in bind.c and needs a 
stun_bind_t context to work.. while stun_conncheck_reply is in 
stun-ice.c and doesn't need it.. it's all very confusing.. is the ice 
conncheck stuff a bind usage thing or an ice usage thing.. hehe.. 
(stun_conncheck_start adds ICE_CONTROLLING and other ice specific 
attributes btw..)
> As for being part of the core - I have to disagree. Binding discovery is part 
> of STUN. Logically: it avoids exposing the STUN message formats to ICE. 
> Architecturally: it is part of the STUN spec, not the ICE one. In real life: 
> it can be used independent of ICE. For testing purpose: it is a lot easier to 
> test in-depth without booting the full ICE layers.
>
>   
As I said before, the binding discovery is just a usage... STUN was 
"Simple Traversal of UDP over NAT", but it was renamed as "Session 
Traversal Utilities for NAT", so it's purpose is to be a utility, not do 
the traversal itself.. anyways, that's just semantics. As I said before, 
the binding usage is very simple, it's just two lines for sending, two 
or three lines for receiving.
The bind stuff with transport and timer are useful only for stunbdc. I 
had at some point moved those files into the tools/ directory and made 
stunbdc link to their .o without them being in libstun.a... I also made 
the test-bind link with the .o files..
In the latest revision, I've put the usages back into a usages/ 
directory to make it easier to port libnice to the new API.

> The STUN-ICE usage is a corner case. It is of course ICE-specific. We put it 
> to the STUN part because it had a lot of common code with the binding 
> discovery, and because we did not want to expose (as far as possible) the ICE 
> agent to the STUN messages.
>
>   
yeah, it's ICE specific and should be in libnice, at least, that's what 
I think.
To summarize what I just said :
- the usages are just "how to use libstun" and should not be part of 
libstun itself.
- whoever wants to do binding requests, would only need a few lines of 
code, so no need for the bind usage
- whoever wants to do ice will probably implement it himself instead of 
using the ice usage since it's specific to libnice
- we could provide helper functions (1 for creating, 1 for parsing) for 
the usages...

Tell me what you think about all that I said, and if you share my thoughts.
If you're ok with it, I'll then proceed to remove the usages from 
libstun and move/copy that code into libnice, while keeping the whole 
bind usage stack for the stunbdc tool.

Thank you,
KaKaRoTo