[Nice] API thoughts

[Youness Alaoui]

Dafydd Harries wrote:
> Ar 17/04/2008 am 09:30, ysgrifennodd Olivier Crête:
>   
>> On Thu, 2008-04-17 at 09:53 +0300, Kai.Vehmanen at nokia.com wrote:
>>     
>>> Hi,
>>>
>>> On 15 April 2008,  Dafydd Harries wrote:
>>>       
>>>>> Could we somehow support both models? Or is there a way to run a set 
>>>>> of GMainContexts in a single GMainloop (without writing a custom 
>>>>>           
>>>> Oh, perhaps I wasn't clear. I absolutely want to have a good 
>>>> API for single-threaded applications too.
>>>>         
>>> aa, ok, then we are on the same page! :)
>>>
>>>       
>>>>> Hmm, yes, this should be doable. Although the locking has to be done 
>>>>> very carefully (inbound STUN messages affect agent-wide state 
>>>>> in many ways, see conncheck.c:conn_check_handle_inbound_stun()).
>>>>>           
>>>> Well, I was thinking that the initial approach could just be 
>>>> to make any handling of inbound STUN packets mutually 
>>>> exclusive. We can make the locking finer later, or perhaps 
>>>> look at using some kind of lock-free update algorithm.
>>>>         
>>> Yes, but you also need to lock the state in all public API 
>>> functions (e.g. in case remote peer restarts the ICE
>>> negotiation), plus in the timer callbacks (which are running
>>> in the "shared context"). But agreed, still very much doable.
>>>       
>> Youness already did a branch to do that, its on the monkey.
>>     
>
> Some comments:
>
> I think some of the mutex code would be simpler with gotos, in the case there
> a function has multiple exit points. Or maybe we could use macros:
>
>   #define WITH_LOCK(x, ret) do { \
>       g_mutex_lock (agent->mutex); \
>       *ret = x;
>       g_mutex_unlock (agent->mutex);\
>       }
>
>   static foo_t
>   do_x (NiceAgent, ...)
>   {
>       ...
>   }
>
>   foo_t
>   nice_agent_do_x (NiceAgent, ...)
>   {
>       foo_t ret;
>       WITH_MUTEX(do_x_unlocked, &ret);
>       return ret;
>   }
>
>   

I don't like MACROs especially this kind... it makes the code ugly and I 
want it to be as clean as possible.

> You used something like this for priv_conn_check_tick.
>
>   
yes, that was only because the function was called twice, once from an 
external API function (so the lock was already taken) and once from a 
timeout (so the lock has to be taken), so I made a locked and unlocked 
version.

> Or even:
>
>   #define WITH_LOCK(function, t, params, args) \
>       t nice_agent_##function (params) \
>       { \
>           t ret; \
>           \
>           g_mutex_lock (agent->mutex); \
>           ret = function (args); \
>           g_mutex_unlock (agent->mutex);\
>           return ret; \
>       }
>
> I think the first is probably nicer though.
>
>   

Again, don't like Macros, but if you want, I can make it use gotos 
instead, would make the code cleaner (if used correctly:p).

> "Detach" has only one "t". :)
>
>   
arg.. yeah.. renamed 'deattach' into 'dettach'.. thx
> Could you add a NICE_COMPATIBILITY_LAST = NICE_COMPATIBILITY_MSN, and use that
> in the compatibility property declaration? This will make it easier to add
> other modes later.
>
>   
right, will do, thx
> I don't understand the changes that look like this:
>
>        res = priv_conn_check_tick_unlocked ((gpointer) agent);
>    
>        /* step: schedule timer if not running yet */
>   -    if (res && agent->conncheck_timer_id == 0) 
>   -      agent->conncheck_timer_id = 
>   -»·······g_timeout_add (agent->timer_ta, priv_conn_check_tick, agent);
>   +    if (res && agent->conncheck_timer_id == 0) {
>   +      GSource *source = g_timeout_source_new (agent->timer_ta);
>   +      g_source_set_callback (source, priv_conn_check_tick, agent, NULL);
>   +      agent->conncheck_timer_id = g_source_attach (source, agent->main_context);
>   +      g_source_unref (source);
>   +    }
>
> Could you explain why they're necessary?
>
>   
ok, the thing is that g_timeout_add automatically adds the timeout to 
the main context, but you might not want that, a user might have a 
non-default mainloop he wants the timeout to be run from, so by using 
this magic trick, I make the timeout create itself in the context that I 
specify (the one given to the nice_agent_new).
> Please commit smaller patches. If it has an "and" in the name, it's probably
> too big. :) (Hint, I'm thinking of "Adding compatibility mode and improvements
> on the main loop integration.", which has a bunch of unrelated changes
> together in one patch.)
>
>   
yeah, I spent 1 hour trying to divide the patches but that's as small as 
I could get them... I should have recorded my changes from the start.. I 
tried to separate compatibility and mainloop but I couldn't because the 
mainloop 'set' is done in the nice_agent_new which prototype also 
contains the compatibility thing.. anyways, it's not much changes on the 
compatibility part.

> I'm not sure we should add the main context and the compatibility as
> parameters to nice_agent_new. In fact, I'm not sure we should have a
> nice_agent_new at all; it should default to creating a sane socket factory if
> you didn't give it one, to using ID19 compatibility, and to using NULL for the
> main context (i.e. the default main context). I.e. you can just call
> g_object_new (NICE_TYPE_AGENT, NULL) in most cases.
>
>   
well, you can do g_object_new(NICE_TYPE_AGENT, NULL), in which case it 
will work as you suggested... I initialize the main context to NULL so 
if you don't specify it, it will use the NULL (default) main context.. 
and the compatibility is set to ID19 as default... I also could have 
left the compatibility out of the nice_agent_new() but I thought it 
would be best to put it there so a user can understand what he creates 
exactly...  we'll avoid later  complaints/errors like "oh, I did set the 
compatibility in here but forgot to do it in that other part of the 
code"... (like someone having a lagent+ragent)..
The main loop though is mandatory because I don't want to give access to 
users to change it later, it has to be set when created and never changed.

KaKaRoTo