[Nice] API thoughts

[Dafydd Harries]

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;
  }

You used something like this for priv_conn_check_tick.

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.

"Detach" has only one "t". :)

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.

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?

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.)

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.

-- 
Dafydd