[Nice] TURN RFC 5766 support in libnice

[Youness Alaoui]

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


-------------- 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/20100929/d01d195f/attachment.pgp>