[Nice] API thoughts
Youness Alaoui
youness.alaoui at collabora.co.uk
Tue Apr 22 11:21:56 PDT 2008
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
More information about the Nice
mailing list