[OpenSER-Devel] dialog callbacks: mi enhancements

[Dan Pascu]

Hi Ovidiu,

In the end I went through the patches and together with the detailed 
explanations I now understand what you try to do. Since this is only a 
means for the dialog module to display information about modules sitting 
on top of dialog for MI purposes I think it is fine the way it is done. 
Please disregard my previous comments as they were made for a different 
context that I now see it doesn't apply to this case.

There is only one thing I'd do differently. I would not use a dialog 
callback for this as they are meant to inform the modules on top of the 
dialog module that some event has happened, as opposed to a means for the 
dialog module to get feedback from the module on top of it. I would 
rather have a module on top of the dialog module register a MI query 
function into the dialog module if it wishes to provide information that 
is to be displayed in a dialog context as a result of an MI request. This 
is a minor difference, but I think it makes for a cleaner and less 
confusing interface and the change is minimal. In this case it is obvious 
that the MI query function provides a means for the dialog module to 
obtain some information from the modules on top of it, while this intent 
is not obvious if a standard dialog callback is used.

I believe that keeping them separate is for the better because we do not 
intermix operations which have different directions for the information 
flows between the modules under the same umbrella (the callbacks move 
information from the dialog module to the modules on top of it to inform 
them of events, while this query function moves information from the 
modules on top to the dialog module to provide MI feedback).

On Friday 18 April 2008, Ovidiu Sas wrote:
> Forgot to mention, the callback is generic and it can be used by any
> module sitting on top of the dialog module.
> In the extra pointer from the callback structure I expect to find a
> pointer to a mi_node structure (which is not module specific).  Each
> module is responsible of providing it's data in a well known format,
> in this particular case, an mi_node structure.
>
> As an example, I'm working on a module that will track the sdp for
> both caller and callee (using the openser integrated sdp parser).
> Here's the output of the dlg_list mi command with both sst and qos
> (the module that I'm working on) loaded (see both sst and qos nodes
> attached to the mi tree structure):
>
> dialog::  hash=364:1735939007
>         state:: 2
>         timestart:: 0
>         timeout:: 0
>         callid:: 1-30200 at 10.11.10.148
>         from_uri:: sip:sipp at 10.11.10.148:5050
>         from_tag:: 1
>         caller_contact:: sip:sipp at 10.11.10.148:5050
>         caller_cseq:: 1
>         caller_route_set::
>         caller_bind_addr:: udp:10.11.10.63:5060
>         to_uri:: sip:4165555001 at 10.11.10.63:5060
>         to_tag::
>         callee_contact::
>         callee_cseq::
>         callee_route_set::
>         callee_bind_addr::
>         sst::  requester_flags=4 supported_flags=0 interval=2400
>         qos:: negotiated_sdp
>                 sdp::  m_dir=1 m_id=1 method=INVITE cseq=1
> negotiation=1 session:: callee cnt_disp= streams=1
>                                 stream:: 0 media=audio port=18074
> transport=RTP/AVP payloads_num=2
>                                         payload:: 100 codec=X-NSE
> sendrecv= ptime=
>                                         payload:: 0 codec=PCMU
> sendrecv= ptime= session:: caller cnt_disp=session streams=1 stream:: 0
> media=audio port=6002 transport=RTP/AVP payloads_num=3
>                                         payload:: 18 codec= sendrecv=
> ptime= payload:: 1 codec= sendrecv= ptime= payload:: 0 codec=PCMU
> sendrecv=inactive ptime=
>
>
>
> Regards,
> Ovidiu Sas
>
> On Thu, Apr 17, 2008 at 6:23 PM, Ovidiu Sas <osas at voipembedded.com> 
wrote:
> > Hello Dan,
> >
> >  The problem that I'm trying to solve was described at the beginning
> > of this thread, maybe with not enough clarity (please follow also
> > http://sourceforge.net/tracker/index.php?func=detail&aid=1933630&grou
> >p_id=139143&atid=743022).
> >
> >  I will use as an example the sst module.  The sst module has a call
> >  specific context:
> >
> > typedef struct sst_info_st {
> >     enum sst_flags requester;
> >     enum sst_flags supported;
> >     unsigned int interval;
> >  } sst_info_t;
> >
> >  The pointer to the sst call specific context is saved inside the
> >  dialog callbacks.
> >  I would like to be able to interrogate - via the mi interface - the
> >  content of the sst call specific context, but the only way to do it
> > is via the dialog module.  Therefore, I need a callback from the
> > dialog module into the sst module in order to be able to retrieve the
> > sst call specific context.
> >
> >  As an example, here's the output of the dlg_list mi command, with
> > the content of the sst call specific context (see the last line:
> > "sst:: requester_flags=4 supported_flags=0 interval=2400"):
> >  dialog::  hash=898:913256572
> >         state:: 2
> >         timestart:: 0
> >         timeout:: 0
> >         callid:: 1-24613 at 10.11.10.148
> >         from_uri:: sip:sipp at 10.11.10.148:5050
> >         from_tag:: 1
> >         caller_contact:: sip:sipp at 10.11.10.148:5050
> >         caller_cseq:: 1
> >         caller_route_set::
> >         caller_bind_addr:: udp:10.11.10.63:5060
> >         to_uri:: sip:4165555001 at 10.11.10.63:5060
> >         to_tag::
> >         callee_contact::
> >         callee_cseq::
> >         callee_route_set::
> >         callee_bind_addr::
> >         sst::  requester_flags=4 supported_flags=0 interval=2400
> >
> >
> >  Hope this answer your question.
> >
> >
> >  Regards,
> >  Ovidiu Sas
> >
> >  On Thu, Apr 17, 2008 at 5:31 PM, Dan Pascu <dan at ag-projects.com> 
wrote:
> >  > On Thursday 17 April 2008, Ovidiu Sas wrote:
> >  > > Well ... define generic.
> >  > >
> >  >  > This particular callback is meant to be used via the MI
> >  >  > interface. You can define other type of callbacks and reuse the
> >  >  > extra pointer that was defined (see void **x_param inside
> >  >  > struct dlg_cb_params).
> >  >
> >  >  That's exactly my point. I was asking the question with a
> >  > purpose, that is to highlight that the proposed change would
> >  > define a dialog callback that can only be used by one module and
> >  > in a very specific context, callback that is on the other hand
> >  > made public. What is the use of a callback that is advertised as
> >  > public, but can only be used by one module for a very specific
> >  > task?
> >  >
> >  >  I mean, can't that module export a public API that the dialog
> >  > module can use to get the information it needs? That would be more
> >  > clean, yet it would still be an awkward reverse dependency.
> >  >
> >  >  IMO, I still find such a dependency to be very problematic to be
> >  > forged into any kind of generic interface. The direct dependency
> >  > is fine. Any module built on top of the dialog module can use the
> >  > dialog's public API to access the dialog state/events. But when
> >  > you have it the other way around, the dialog module cannot
> >  > possibly know what modules will be built on top of it to be able
> >  > to generically extract information from them. This kind of
> >  > dependency is not only awkward, but it is very narrow in its
> >  > scope, which hardly justifies it being put in a public API that
> >  > none uses, except that module.
> >  >
> >  >  I'm sure that if we would know in more detail what problem you
> >  > are trying to solve (as opposed to how you try to solve it), many
> >  > people here could offer good suggestions that may turn into a
> >  > better design.
> >  >
> >  >  --
> >  >  Dan



-- 
Dan