[Nice] New API and refactor

Wed May 7 11:01:41 PDT 2008

On Wed, 2008-05-07 at 10:11 +0300, Kai.Vehmanen at nokia.com wrote:
> Hi,
> 
> On 07 May 2008,  Youness Alaoui wrote:
> >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.
> 
> well, could you elaborate why the code needs major refactoring? 
> I can understand you might have some style and API-usage issues (as
> e.g. nice/stun is not using glib), but it does work and is very clean 
> and tested code. What's the purpose of refactoring?

Youness tried to implement Google Talk compatibility. He has had a very
hard time to do it. He added an enum with various compat modes to the
agent. Google mode has some subtle differences (remotelocal instead of
remote:local). Google Talk fails if "username" is not the only attribute
in the message. So you have to pass the compatibility enum everywhere
inside stun, because there is no separation between the stun message
bulding/parsing and the behavior and the ICE-specific stuff. The stun
part adds fingerprint, ice-controlling, use-candidate, etc.. instead of
separating that.

Also, Google talk does not support the 3489bis stuff, but the current
STUN code does not provide any compatibility with RFC3489. Changing that
is a lot of work, because there are assumptions about using the new
draft in many places. For example, the transaction-id is cut to 12bytes
and the magic cookie is inserted in replies without looking at the
compatibility mode.

> >The way I currently see it is : modular, layered, simple and beautiful!
>
> Like the current STUN implementation? ;)

I fail to see what the "public" api of the STUN implementation. That
makes it hard to see what the layers are. Also stuff like
stun_conncheck_start() is defined in both conncheck.h and stun-ice.h
(not what I'd call clean...).

> >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.
> 
> The current implementation follows the latter approach. One design
> goal was to keep the nice/stun module mainloop-independent (to 
> make it easier to use it in other contexts). This had the 
> design side-effect that nice/stun module couldn't wake up and
> deliver incoming messages by itself. Thus the current implementation
> (where nice/agent has to try and parse incoming messages
> with STUN).

I kind of agree with that part of the current implementation. But in
that case, the message parsing and behavior aspects should be clearly
separated (and not call each other in most incestuous ways).

> >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) :
> [...]
> >StunMessage * msg = stun_message_new(STUN_RFC3489BIS);
> 
> Oh please no! :) STUN messages are used on the hotpath and you 
> shouldn't do dynamic memory allocs on it unless you have a pressing 
> need. Some systems (like gstreamer) might provide a real-time safe 
> memory allocator, but in a general purpose library avoiding these 
> allocs is much, much preferred (the client app can do with a normal
> allocator and still use the library). And in the case of STUN, 
> these allocs can be easily minimized (as demonstrated by the current 
> implementation).

GStreamer just does malloc() for allocations, nothing real-time. But
after spending some more time thinking about it this morning, I think
you may be right that not using malloc is probably the way to go for the
stun parsing. I still think that we should have a clean api to isolate
stun parsing from any behavior.

> >So now my question to you is : what do you think? How do you think the 
> 
> In the end I'm not opposed to refactoring, but I want to make
> sure it's made for the right reasons. I'm worried that some
> ugly-by-design code (because of requirements), is now turned 
> beatiful, and in the process we no longer meet the original 
> requirements. But if this worry is unwarrented, then no problem...

If you using glib, any allocation that it does will use g_malloc(),
g_malloc() calls abort() if malloc fails. So really, having a
requirement to verify memory allocations for any system that uses glib
is bollocks.

-- 
Olivier Crête
olivier.crete at collabora.co.uk
Collabora Ltd
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.freedesktop.org/archives/nice/attachments/20080507/41b98640/attachment.pgp 