[Nice] TURN RFC 5766 support in libnice

Youness Alaoui youness.alaoui at collabora.co.uk
Wed Dec 15 17:57:53 PST 2010 

Hello again Marcus,

I'm having some time now to continue reviewing your RFC branch, I'll post my
comments here, but I think I will also just fix them as I see them, because I
want to get rid of this once and for all instead of going through multiple back
and forth review emails. So you don't have to do anything now, but I'm still
sending you the code review comments. The reason I'm sending this is for you to
learn from your mistakes, and for you to let me know if I said something with
which you disagree, so I don't 'fix' something that shouldn't be fixed.

- in socket_close, you don't free the priv->sent_permissions, only priv->permissions
- in socket_close, you don't destroy/free the tick_source_create_permission
- in socket_close, you destroy the permission_timeout_source but not the
binding_timeout_source
- in priv_remove_peer_from_list, when you call nice_address_free, you could use
'address' instead of re-casting iter->data
- you initialize i = priv->channels, then later you do : for (; i; i = i->next)...
this makes it hard to read because you need to go look elsewhere at what i was
initialized with.
- when you set a channel bind to active=FALSE, you call send_channel_bind which
set current_binding_msg, but doesn't set current_binding, and when you receive
the binding response, you check if current_binding != NULL, then you set
current_binding->active = TRUE, which means that expired bindings will have
active=FALSE but will never be reset to active=TRUE...
- You only resend a binding request when the user wants to send data, you should
probably send it as soon as it expires (-30seconds) otehrwise if it takes too
long before the user sends data (the call was on 'hold'), then data will fail/be
dropped until the binding is recreated.
- You assume the permission succeeded if the response isn't an "unauthorized
error", what if it's a different error?
- You don't check if you already sent an 'authorized' create_permission when you
receive 'unauthorized', if the user/pass is invalid, it will loop forever
resending the create_permission with the wrong credentials..
- in send_create_permission, you start the timer and schedule_tick and set
current_create_permission_msg without verifying if nice_socket_send returned
TRUE or not.
- You copied the code for the retransmissions and timeouts of the channel bind
for the permissions stuff, but you should minimize code duplication, and it
needs to be refactored to have a single code take care of both types of timeouts.
- the retransmissions_*_tick_unlocked returns FALSE after calling schedule_tick,
but schedule_tick sets the new timer, and the retransmissions_*_tick function
when it receives FALSE from the *_tick_unlocked then destroys the timer, so
we'll never resend more than once and we'll never get a 'retransmission
timeout'. (This was copied from my own code and yes that bug was in my code
too.. which is why it needs to be fixed twice now.. avoid code duplication
because it also duplicates bugs :) )

- in stun/usages/turn.c, a permission can't work without a XOR_PEER_ADDRESS, so
I think instead of doing a if (peer) { append.. } it should be if (!peer) return 0;


I think that's all I can see for now, I fixed a few things, I'll finish fixing
these comments from the review, then i'll refactor that retransmissions stuff to
avoid code duplication, then hopefully will make a release early next week
(finally!).

Youness.



On 09/29/2010 11:52 PM, Youness Alaoui wrote:
> Here is my first (partial) review :
> 
> turn.c :
> - you should comment on what those variables/structs you added are used for
> (yeah, sorry my variables were also lacking comments)
> 
> - I see trailing spaces.. that's very bad.. please, no trailing spaces, no tabs...
> 
> - you do a hash of the nice address, but that's not right... first, if we're
> using ipv4, then there is a bunch of unused data (that is there for ipv6) that
> could be uninitialized or have any value... also, the port is also specified in
> the nice address...  i would suggest you use g_str_hash and do a
> nice_address_to_string... that would also fix the issue of the port (as far as I
> know, the permissions are per peer IP, no matter what port it is, right ?).
> If the port is needed, then it might not be a bad idea to add that
> nice_address-hash function directly to the address.c file and make it part of
> the public API. and make it do a g_str_hash of a :
> g_strdup_printf ("%s:%s", nice_address_to_string(), nice_address_get_port() )
> 
> 
> - In priv_send_data_queue_destroy, there is a slight indentation issue...
> I would suggest you open the files in emacs, and do :
> Ctrl-x h
> Alt-x untabify
> Ctrl-x h
> Alt-x indent-region
> Alt-x set-variable <enter> show-trailing-whitespace <enter> 't
> Then browse the file for any trailing whitespaces in the file (they will appear
> red, so it will annoy you).
> also resize your window to a width of 80 chars and make sure you never have
> lines longer than 80 chars...
> then commit all that as "Fixing tabs, trailing whitespaces and indentation"
> 
> 
> - I see inconsistencies.. for example :
> +               g_hash_table_new_full (priv_nice_address_hash ,
> +                                   (GEqualFunc) nice_address_equal ,
> +                                   (GDestroyNotify) nice_address_free, NULL);
> There is a space between the *_hash and the comma, same for _equal, but not for
> _free... My rule is to always put a space *after* a comma, but not before..
> 
> - oh... you should not use a GHashTable to hold the premissions and
> sent_permissions... a hash table is to link X to Y, not to do an insert (X,
> X)... if you want it just for the g_hash_table_lookup (to) != NULL.. you could
> very well use a GList and do g_list_find (to) != NULL... or if you want it for
> the hash. you could do g_list_prepend (nice_address_hash(to)), then
> g_list_find(nice_address_hash(to))...
> anyways, there's definitely a better way than using a hash table for a something
> that isn't an association table.. or even a table...
> 
> - in socket_dequeue_all_data, you do a while (g_queue_get_length() > 0).. this
> forces it to calculate the length, which you don't need... instead you need to
> use g_queue_is_empty ()
> 
> - The SendData structure has a ->priv field, but it's not set or used anywhere..
> it should be removed.
> 
> - in socket_send, you may want to group the if(compat == RFC) ... with the
> second one a bit lower...
> unless you need to send the channel bind after sending the create_permission...
> because it's a bit unclear, we need to read two separate blocks of code to
> understand one behavior.
> 
> - do you timeout all bindings at once? I see a 'has_binding'.. shouldn't it
> actually be 'binding->active'.. and have the loop in socket_send not return the
> binding if it's active == FALSE ?
> 
> - sent_binding boolean could be a current_binding_msg != NULL, no ?
> 
> 
> I give up... it's getting late, and the whole indentation issue is driving me
> crazy! There are also a lot of added nice_debug messages that should probably
> also be removed... and do a 'git diff origin/master mlundblad/master' (or just
> 'git diff origin/master master') to see your own diff and try reviewing it
> yourself before I continue.. if something annoys you, it will probably annoy me
> too! :)
> I reviewed the other files though, apart from turn.c.. they seem ok, apart from :
> - same indentation/whitespace issue
> - test-turn.c has your ip in there...
> - stunmessage.c and stunagent.c have useless changes which i shouldn't have to
> bother seeing :p
> 
> That's it for now, try cleaning it all up and fix the things I said so far then
> push your changes and let me know, i'll continue/restart the review and
> comment/merge it :)
> 
> 
> Thanks!
> Youness.
> 
> 
> 
> On 08/31/2010 03:48 PM, Marcus Lundblad wrote:
>> Hi.
>>
>> I have implemented RFC 5766 support in libnice. This means correctly
>> sending CreatePermission requests for peers the client relays to.
>> I have also fixed it to send ChannelBind renewals before the 10 min.
>> timeout comes.
>>
>> This code can be found in this git repo:
>>
>> git://github.com/mlundblad/libnice.git
>>
>> in the master branch
>>
>> I also plan to eventually fix up TCP relaying to work in RFC mode
>> (currently it appearantly only supports Google mode).
>> Perhaps I might look into supporting TLS mode too at some point.
>>
>> Cheers!
>>
>> //Marcus
>>
>> _______________________________________________
>> Nice mailing list
>> Nice at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/nice
> 
> 
> 
> 
> _______________________________________________
> Nice mailing list
> Nice at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nice


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/nice/attachments/20101215/bbd3a70b/attachment.pgp>

More information about the Nice mailing list