[Nice] New API and refactor

[Rémi Denis-Courmont]

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 
them.

-- 
Rémi Denis-Courmont