[Nice] New API and refactor
remi.denis-courmont at nokia.com
Thu May 8 23:42:19 PDT 2008
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
> This should clearly be specific to a usage and not be in the core
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,
> This is about the agent (that does check the retval of g_slist_append()
> 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 = (c >> 1) | ((m >> 6) & 0x3e);
> h = ((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:
The message type field is decomposed further into the following
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|
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.
More information about the Nice