[Nice] New API and refactor

[Rémi Denis-Courmont]

On Monday 21 July 2008 23:54:34 ext Youness Alaoui, you wrote:
> > No. Binding discovery is part of STUN. So are the timers. You could run
> > ICE with any other binding discovery scheme (TURN, Teredo server, UPnP
> > IGD, NAT-PMP, SOCKS...). This is part of STUN. Both in the spec, and in
> > the architecture. The timers are part of STUN. They simply are.
>
> Yes, that's true, but those timers only apply for UDP, not for TCP or TLS.
> I wouldn't mind having those timers implementation as utility functions
> for the library.

Sure. The timer API is already separated - the timer.h and timer.c only depend 
on libc, not on anything else from the STUN library. Similarly the 
transaction code is independent from the STUN message code only depends on 
libc and the timer code. If I recall correctly, stun-msg.h was included to 
get the buffer size.

I would not think that 400 lines of code grant for a separate shared object 
with "clean opaque" structure types.

> I would like to separate them as two libraries.. stun-format and
> stun-usage, or something like that. Anyways, we're not implementing the
> whole STUN specs as far as I know (I don't see the TCP or TLS
> implementation, or the DNS discovery). Maybe it will be better once
> refactored, but the problem now is that it's not very extensible and
> does too much stuff that it shouldn't be doing, so if we want to keep
> those timers working even for TLS sockets, it needs to be refactored.

The timer code already supports TCP/TLS. The transaction layer only 
partially - because ICE-TCP is in too much of a limbo to know how to deal 
with it.

> Yes, adding timers and error cases, etc.. will add code, but the purpose
> of the library is to make it easy to do STUN, and not "everything in one
> function". It would be nice if glib had a one function "g_do_voip"... no
> it doesn't, but it gives a lot of small functions that makes it easier
> to build something to do that. Or a more realistic example, farsight
> doesn't have a 'farsight_start_audio(ip, port)' function... That's just
> the way I see it.

That's pointless a discussion. libc has the getaddrinfo() function, but you 
can also use the lower-level DNS querying functions. Or you can use the DNS 
formatting function and sockets manually. Or you can even reimplement 
everything from scratch. So what?

The real question is whether we actually NEED to extend this piece of code, 
for some actual use case that does not fit the current model. Maybe we do, 
but I have not seen it so far.

> As I said, I understand your point of view, and I agree with most of
> what you said. The best solution would probably be somewhere in the
> middle. I will refactor the bind usage into something less 'obscure',
> and I'll make the timers as a utility function, so anyone can use them
> for any usage they want (it's not bind usage specific, it's 'sending
> over UDP' specific).

As I said, that's already done at the source code level. You can do it at the 
Makefile level if you so wish.

(...)
> Right now, the bind usage has its own agent in there, it does some
> mallocs, creates sockets, sends data, etc.. We need to keep it simple,
> no malloc, no socket mess, no agent created by the usage itself (I have
> one agent in ice, and another one in the bind usage, and they don't
> share the same list of sent IDs, and one can't validate a response to a
> request sent from the other, so it's confusing).

It only creates socket if the application does not bother doing so. In the ICE 
case, that code is (as far as I remember) not used.

As for mallocs - I don't know. You've lost me. I thought you wanted to layer 
everything über nicely. If you remove mallocs from the STUN you will have to 
expose structures more to the outside.

> Is that all ok with you ?

I would rather not: add mallocs on the fast path, remove memory error 
handling, and/or add more memory copying on the RX/TX path. Other than that, 
I don't really care.

-- 
Rémi Denis-Courmont
Maemo Software, Nokia Devices R&D