[Nice] complete example

Fri Feb 1 16:35:53 PST 2013

Thanks!
I've finished cleaning up the code and integrated it into libnice. I've pushed
the changes here :
http://cgit.collabora.com/git/user/kakaroto/libnice.git/log/
First of all, I added a nice_agent_get_selected_pair because I think it's much
more efficient and logical to have that than being forced to listen to the
signal and keep track of the foundations.
I think I will also add a way to get the state without storing it from the
state-changed signal handler.
I think it would be a good idea to make the license match the rest of the code
from libnice. If you don't want to, then it's not a big deal.
So.. here's the changelog :
- Better to use '-4 -t A' options to 'host' command because it seems to freeze
for a long time otherwise. Added that to the comment.
- include <agent.h> instead of <nice/agent.h>, since $includedir/nice is added
to CFLAGS with pkg-config --cflags. (and it caused it to take the agent.h from
my installation rather than the local version)
- I used a table for transforming the candidate_type and state into strings,
much easier, although less robust, but since the data comes from libnice, it's
expected to be correct.
- Reordered the functions to be more linear/sequential.. the main function at
the top, then followed by whichever function gets executed next.
- Removed the thread, and used the mainloop directly and make the code work
asynchronously in the callbacks.
- stun address is now optional, and I check validity of the controlling argument.
- I use a g_io_channel for stdin and listen asynchronously to data written on stdin.
- I use nice_agent_get_selected_pair to print the ip/port used, and do it on the
state-changed signal.
- I exit the app in case there's a failure to connect
- I use g_strsplit_set instead of parse_args (which I removed).
- I use g_strsplit instead of sscanf for parsing the candidate
- I loop through the candidate_type_name instead of doing an if/else of each
possibility (when parsing the candidate)
- I prefered using if (!function) rather than 'ok = function(); if (!ok)'.. it's
a personal choice though.
- Other small coding style changes to better match libnice's coding style (or
what it should be), like indentation, \n{ in a function declaration, etc..

If you could review the code I wrote and let me know if there's anything you
think needs fixing, let me know.

Thanks!
Youness.

On 02/01/2013 04:28 PM, Bryce Allen wrote:
> Patch attached. Found a bug in print_local_data when there is more than
> one local candidate (wasn't printing spaces in between). Also replaced
> the busy waiting with condition variables.
> 
> -Bryce
> 
> On Fri, 1 Feb 2013 11:24:32 -0600
> Bryce Allen <ballen at ci.uchicago.edu> wrote:
>> Hi Rohan,
>>
>> Thanks for testing it out and for the feedback!
>>
>> Re parse_args, I think it is correct as is. The line returned by fgets
>> may not contains '\n' (if it's very long), so it's not safe to only
>> check for '\n' as end of string, and the !isspace(*p) already handles
>> the '\n' as well as other whitespace to make sure it won't be included
>> in the arg:
>>
>>         // find end of current arg
>>         while (*p != '\0' && !isspace(*p)) { p++; }
>>         if (*p == '\0')
>>             break;
>>         *p = '\0';
>>
>> Were you getting a crash or a hang? It works for me when I copy just
>> the single line, but if I include extra empty lines it just hangs.
>> Also if your terminal wraps the input line, it will fail. It would be
>> nice to have a less fragile format - I think that is what Youness was
>> suggesting with the SDP format. I've attached a new version that
>> handles empty lines and re-prompts if the parse fails, and fixes a
>> memory leak in parse_remote_data.
>>
>> Re removing the thread for mainloop, I'm not sure the STUN
>> binding/keep alive requests will continue if the thread is quit after
>> negotiation is complete (during the send/receive phase). It seems
>> nice to manually drive the loop for tests and avoid the complexity of
>> threads, but I'm not sure it's what we want for an example.
>>
>> -Bryce
>>
>>
>> On Fri, 1 Feb 2013 12:37:43 +0530
>> Rohan Garg <rohan16garg at gmail.com> wrote:
>>> Hey everyone :)
>>> I went over the program a bit, and I'm attaching a patch that fixes
>>> a couple of issues, just minor stuff regarding the thread_mainloop
>>> and making the code a wee bit more async.
>>>
>>> There also seems to be a bug on line 241 of the original code where
>>> it only stops at \0 , I believe that should be \n. When using \0 the
>>> program simply crashes at " remote_candidates =
>>> g_slist_reverse(remote_candidates); " in parse_remote_data.
>>>
>>>
>>> On Fri, Feb 1, 2013 at 6:05 AM, Youness Alaoui
>>>> I have a plan of adding a new API call for transforming the
>>>> candidates into SDP and to parse candidates from SDP. I think it
>>>> will help people, and I think it would be a good idea to port your
>>>> code to it. I'll let you know tomorrow what I find!
>>>>
>>> Could you explain this a bit on the list in another thread? :)
>>>
>>> Regards
>>> Rohan Garg
>>>
>>>> Thanks,
>>>> Youness.
>>>>
>>>>
>>>> On 01/31/2013 07:30 PM, Bryce Allen wrote:
>>>>> I expanded my nicetest.c into a complete example, using a simple
>>>>> serialization format and copying/pasting the data between
>>>>> clients. It's inspired by icedemo from pjnath, but I didn't find
>>>>> having separate commands for each phase useful for testing and
>>>>> it made reading the code more difficult, so I just have three
>>>>> phases:
>>>>>
>>>>> 1) gather candidates, print serialized local data
>>>>> 2) prompt user for remote data, parse and set when entered
>>>>> 3) if successful, prompt for data to send to remote end
>>>>>
>>>>> I'm probably missing some failure cases and some error checking,
>>>>> but it seems like it could be a useful addition to the main
>>>>> distribution after a fixup. I can re-license under LGPL/MPL if
>>>>> needed.
>>>>>
>>>>> Also please let me know if I'm doing anything fundamentally wrong
>>>>> here. I'm using pretty much the same flow for a real application,
>>>>> with condition variables to synchronize instead busy waiting.
>>>>>
>>>>> Thanks,
>>>>> Bryce
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> 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
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> 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: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/nice/attachments/20130201/03151a4a/attachment-0001.pgp>