[Nice] New API and refactor

[Rémi Denis-Courmont]

Le Friday 09 May 2008 03:10:34 ext Olivier Crête, vous avez écrit :
> On Thu, 2008-05-08 at 09:38 +0300, Rémi Denis-Courmont wrote:
> > Le Wednesday 07 May 2008 21:01:41 ext Olivier Crête, vous avez écrit :
> >
> > 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);
>
> Good example... look at the "stun_server_append" here... This is clearly
> behavior specific.. and it should not be in the generic code...

For any response, it makes sense to include the SERVER name, so it _is_ 
generic. Also, it's a non-mandatory attribute.

> Google Talk ignores stun packets that have unknown attributes, so
> stun_fingerprint or stun-server can NOT be there.... I had to produce
> theses horrors to make it possible to disable them:

libstun implements STUN. As in RFC3489 (client-side) and the latest STUNbis 
draft (both sides, since connectivity checks are peer-to-peer). GTalk fails 
both of these...

If you want to duplicate the stun_append_server calls, but I fail to see how a 
few tweaks for adding more usages grant for a whole redesign. I still don't 
get the big picture as to how the STUN API would be fundamentally broken and 
ugly.

> This should clearly be specific to a usage and not be in the core
> stuff...

No. The problem is a _GTalk_ breakage here. If it had not been for its 
non-standardness, stun_append_server() would work fine. The only standard 
thing it breaks, is legacy RFC3489 server-side (not something we care for, 
really).

(...)
> This is about the agent (that does check the retval of g_slist_append()
> everywhere)...
>
> Frankly, one of the main reasons we're going to rewrite it is that it is
> almost impossible to follow what does what. Its filled with  magic
> numbers without any comment to explain them with stuff like:
>   h[0] = (c >> 1) | ((m >> 6) & 0x3e);
>   h[1] = ((c << 4) & 0x10) | ((m << 1) & 0xe0) | (m & 0x0f);
>
> I have no idea what that does or what its supposed to do.

Any piece of arithmetic will look like magic outside of its context.

void stun_set_type (uint8_t *h, stun_class_t c, stun_method_t m);

Ok, it's not obvious that h is the message Header. hdr if you'd like. As for 
the class and method, if you don't know what it refers to... I am getting 
frightened at the thought that you are trying to rewrite the library without 
even figuring out the protocol spec:

http://tools.ietf.org/html/draft-ietf-behave-rfc3489bis-15#section-6

   The message type field is decomposed further into the following
   structure:

                         0                 1
                         2  3  4 5 6 7 8 9 0 1 2 3 4 5

                        +--+--+-+-+-+-+-+-+-+-+-+-+-+-+
                        |M |M |M|M|M|C|M|M|M|C|M|M|M|M|
                        |11|10|9|8|7|1|6|5|4|0|3|2|1|0|
                        +--+--+-+-+-+-+-+-+-+-+-+-+-+-+

                Figure 3: Format of STUN Message Type Field

...
  
> And I won't mention that amount of seemingly random pointer arithmetic.

This reads too much like bad faith to me that I don't want to reply to that.

The STUN stacks works. It has an extensive test suite, it's extensible, and 
it's been tested against third-party both legacy RFC3489 and modern ICE 
implementations. It's fast, it's memory error-safe, it's portable.

I have no doubt that it needs to be extended and tweaked to accommodate new 
usages. But redesigning it looks like a total waste of time.

-- 
Rémi Denis-Courmont