[Nice] New API and refactor
remi.denis-courmont at nokia.com
Wed May 7 23:38:32 PDT 2008
Le Wednesday 07 May 2008 21:01:41 ext Olivier Crête, vous avez écrit :
> 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.
Then it's a different STUN usage. We already have at least two of them
(binding discovery and ICE connectivity checks) that are layered on top of
the common parsing code. I don't see the problem in adding a third one. Plus
we're going to have to add yet another one for TURN anyway. STUN NAT control
could have been a fifth one if it took off.
> Also, Google talk does not support the 3489bis stuff, but the current
> STUN code does not provide any compatibility with RFC3489.
It does. Tested it against public STUN servers even.
> 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.
I don't think so. And if it does, it's a bug. That being noted:
void stun_init_response (uint8_t *ans, size_t msize, const uint8_t *req)
/* For RFC3489 compatibility, we cannot assume the cookie */
memcpy (ans + 4, req + 4, 4);
(void)stun_append_server (ans, msize);
> > >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.
The STUN stack has two layers. The message parsing/formatting API is in
<stun-msg.h>. The usages on top of that have there own headers <bind.h> and
<stun-ice.h> at the moment.
There are a few shortcuts that should possibly be cleaned up, whereby the
agent code calls into the message parsing directly to demux STUN and RTP, but
I don't see the point of redesigning (and loosing a lot of properties we took
great pain for) the whole STUN stack for this.
> 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...).
I can't comment. I don't have <conncheck.h> here. It's been added after I last
had the opportunity to touch the code.
> > >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).
The STUN code does not call the ICE code _anywhere_. At least, it did not when
I last saw it.
The STUN code implements the STUN usages as defined per the relevant document
(currently that's STUN binding discovery and STUN/ICE connectivity checks).
> 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.
GStreamer != the world.
> If you using glib, any allocation that it does will use g_malloc(),
> g_malloc() calls abort() if malloc fails.
Precisely, that's why the STUN code does not use glib.
> So really, having a requirement to verify memory allocations for any system
> that uses glib is bollocks.
That's why we were getting rid of glib allocations instead of adding more of
More information about the Nice