[OpenSER-Devel] [ openser-Patches-1872331 ] dialog: get_direction() api enhancement
SourceForge.net
noreply at sourceforge.net
Fri Apr 4 10:37:50 CEST 2008
Patches item #1872331, was opened at 2008-01-15 21:29
Message generated for change (Settings changed) made by bogdan_iancu
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=743022&aid=1872331&group_id=139143
Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: modules
Group: ver devel
>Status: Closed
Resolution: Accepted
Priority: 5
Private: No
Submitted By: Ovidiu Sas (osas)
Assigned to: Bogdan-Andrei Iancu (bogdan_iancu)
Summary: dialog: get_direction() api enhancement
Initial Comment:
Sometimes, modules that are sitting on top of the dialog module needs to know the direction of a message (upstream or downstream).
The following patch will enhance the dialog callback api with get_direction() function.
The prototype:
unsigned int get_direction(struct dlg_cell *dlg, struct sip_msg* msg);
and it will return one of the following values:
DLG_DIR_UPSTREAM # the direction of the initial INVITE
DLG_DIR_DOWNSTREAM # the direction of the first reply
DLG_DIR_NONE # in case of an error
----------------------------------------------------------------------
>Comment By: Bogdan-Andrei Iancu (bogdan_iancu)
Date: 2008-04-04 11:37
Message:
Logged In: YES
user_id=1275325
Originator: NO
Hello Ovidiu,
Following your idea, I did a rework of the patch for a more efficient
implementation - no need for extra computing for detecting the direction of
replies.
Please test the changes and let me know if something is wrong ;).
Thanks and regards,
Bogdan
----------------------------------------------------------------------
Comment By: Bogdan-Andrei Iancu (bogdan_iancu)
Date: 2008-04-03 10:57
Message:
Logged In: YES
user_id=1275325
Originator: NO
I'm still in process of re-viewing it - If ok, I will committed today.
Regards,
Bogdan
----------------------------------------------------------------------
Comment By: Ovidiu Sas (osas)
Date: 2008-04-03 01:47
Message:
Logged In: YES
user_id=1395524
Originator: YES
If there are no objections, I will push the dlg_direction.patch tomorrow
morning.
Regards,
Ovidiu Sas
----------------------------------------------------------------------
Comment By: Ovidiu Sas (osas)
Date: 2008-03-31 15:50
Message:
Logged In: YES
user_id=1395524
Originator: YES
New patch against the latest trunk attached.
Regards,
Ovidiu Sas
File Added: dlg_direction.patch
----------------------------------------------------------------------
Comment By: Bogdan-Andrei Iancu (bogdan_iancu)
Date: 2008-03-31 13:38
Message:
Logged In: YES
user_id=1275325
Originator: NO
Hi Ovidiu,
The API changes look good. Please re-work the get_direction() patch
(original issue) and post it here.
Thanks and regards,
Bogdan
----------------------------------------------------------------------
Comment By: Ovidiu Sas (osas)
Date: 2008-03-27 21:41
Message:
Logged In: YES
user_id=1395524
Originator: YES
If there are no objections, I will push the dialog_api.patch tomorrow
morning.
Regards,
Ovidiu Sas
----------------------------------------------------------------------
Comment By: Ovidiu Sas (osas)
Date: 2008-03-26 22:38
Message:
Logged In: YES
user_id=1395524
Originator: YES
Here's a patch that will update _only_ the API.
Let me know if you are all ok with it, and I will commit it.
Here's the new API for registering a dialog callback:
-int register_dlgcb(struct dlg_cell *dlg, int types, dialog_cb f, void
*param )
+int register_dlgcb(struct dlg_cell *dlg, int types, dialog_cb f, void
*param, param_free ff )
Here's the callback signature:
-typedef void (dialog_cb) (struct dlg_cell* dlg, int type, struct sip_msg*
msg, void** param);
+typedef void (dialog_cb) (struct dlg_cell* dlg, int type, struct
dlg_cb_params * params);
Both 'struct sip_msg* msg' and 'void** param' are passed via a
dlg_cb_params structure:
struct dlg_cb_params {
struct sip_msg* msg;
void **param;
};
The patch is just cosmetic, it doesn't change the existing functionality.
Regards,
Ovidiu Sas
File Added: dialog_api.patch
----------------------------------------------------------------------
Comment By: Bogdan-Andrei Iancu (bogdan_iancu)
Date: 2008-03-26 15:01
Message:
Logged In: YES
user_id=1275325
Originator: NO
Hi guys,
I'm in favour of the last approach (from Dan) - a static structure for
passing all the parameters to the callbacks. For the user parameter, at
callback registration, a free function should be provided.
Regards,
Bogdan
----------------------------------------------------------------------
Comment By: Dan (dan_pascu)
Date: 2008-03-25 20:20
Message:
Logged In: YES
user_id=1296758
Originator: NO
Ovidiu, I was speaking about the user param, not the internal structure
which can be on the stack, but it can't be a static global.
The problem with the current approach is that I need to always add a
callback for the dialog ending notification just to clean up the param,
while I may not be interested in that callback at all. OTOH, we can just
indicate a function to free the param, and the dialog module should free it
if it is still present in the structure (i.e. wasn't freed by the user) at
the time the dialog is ended. This is just a convenience to write more
compact code and prevent accidental memory leaks. So the dialog startup
callback, that sets up the rest of the dialog callbacks and their optional
parameter, can also indicate a free function to be used on that parameter.
If this function is specified, the dialog module will automatically free
the user param when the dialog ends if it wasn't freed before that point.
If the function pointer is NULL, then no attempt to free it is made and
it'll work like now (i.e. you have to free it yourself).
To put this in an example, this would look like:
dlg_api.register_dlgcb(dlg, EVENTS, callback, param, param_free_function)
instead of the current:
dlg_api.register_dlgcb(dlg, EVENTS, callback, param)
----------------------------------------------------------------------
Comment By: Ovidiu Sas (osas)
Date: 2008-03-25 17:47
Message:
Logged In: YES
user_id=1395524
Originator: YES
The tm module is using a static structure for the tmcb_params and no free
is performed inside the tm module with respect to callbacks.
I think that each module should free up it's own allocated memory (just
like dialog - being the caller of tm - is freeing up it's own context).
In this case, the caller of the dialog module should free up it's own
memory.
----------------------------------------------------------------------
Comment By: Dan (dan_pascu)
Date: 2008-03-25 17:10
Message:
Logged In: YES
user_id=1296758
Originator: NO
My idea is to replace the current **void user parameter to the callback
with a structure that contains this **void param and anything extra we may
need. This is why I said the dialog module can free it automatically (now
is the responsibility of the caller as the dialog module doesn't keep track
of it). This structure cannot be global, it must be per callback otherwise
it may have concurency issues.
I don't think we really need 2 pointer arguments to the callback (the
current one and the new structure param). We can embed the existing user
data in this new structure. At least this is how tm callbacks work and I
think we can use the same approach.
----------------------------------------------------------------------
Comment By: Ovidiu Sas (osas)
Date: 2008-03-25 16:48
Message:
Logged In: YES
user_id=1395524
Originator: YES
I don't think that we need to allocate memory for this new structure (at
least for now). If we really need persistence across dialog life, then the
structure should be incorporated into the dialog context.
If we reached an agreement here, I can rework the patch to provide a
pointer to a structure instead of the actual value.
Please let me know your thoughts.
Regards,
Ovidiu Sas
----------------------------------------------------------------------
Comment By: Dan (dan_pascu)
Date: 2008-03-25 15:36
Message:
Logged In: YES
user_id=1296758
Originator: NO
I was thinking of the same thing. A structure can easily be extended in
the future with no impact on existing code. Besides tm callbacks already
use the same approach. The current user param pointer the callbacks receive
can be a member of this structure. If this structure is maintained by the
dialog module, then it could also easily free all the members, including
the user specified param, automatically when the dialog is destroyed. This
would free the user of the need to manually free it in various callbacks.
All that needs to be done for this is to specify which free function to use
(when the param is specified to the initial create_dialog callback).
----------------------------------------------------------------------
Comment By: Bogdan-Andrei Iancu (bogdan_iancu)
Date: 2008-03-25 13:51
Message:
Logged In: YES
user_id=1275325
Originator: NO
Hi Ovidiu, Hi Dan,
I agree with Dan here - The APIs should have a vertical development,
rather than an horizontal one.
On the other hand we need to find a nice (scalable and not using static
global vars) to pass certain data to the callback functions.
My suggestion will be to add a single new param to the callback prototype
- this will be a structure to encapsulate whatever params we need to pass
through...
This is just an idea - any comments or other ideas are welcome :)
Regards,
Bogdan
----------------------------------------------------------------------
Comment By: Ovidiu Sas (osas)
Date: 2008-03-18 21:45
Message:
Logged In: YES
user_id=1395524
Originator: YES
Hello Dan,
I see your point, and I agree with you about maintaining two different
versions of the same module (I have the same situation here).
But in this case, I think the less disturbing solution for the code flow
is with an API change. Maybe there is another solution, but I fail to see
it right now.
As I mentioned before, I would like to collect more opinions on this
matter from other users of the dialog module.
As for non-public modules, I don't think that the project should be
concerned about it. We should be concerned about what is in the project.
Regards,
Ovidiu Sas
----------------------------------------------------------------------
Comment By: Dan (dan_pascu)
Date: 2008-03-18 21:24
Message:
Logged In: YES
user_id=1296758
Originator: NO
There is at least another module, one I'm working on and that will be
pushed to svn soon. I already have this in test for 1.3 and quite frankly I
do not want to maintain 2 versions for it. A backward incompatible change
in the API is never a good thing, no matter how little code depends on it,
especially when it can be avoided. We should also consider that there may
be non-public custom modules that people may have written and such
incompatible changes will make maintaining them more complicated than
necessary. The issue is not as much making a small change, is maintaining 2
versions for the code.
As for the extra memory requirements, I do not think that is really a
concern. The direction can be stored in a 1 bit flag.
I'm not sure about the race issues. Someone more knowledgeable about the
dialog internals can clarify if storing the direction on the dialog is even
poossible in the given context. If it is not possible then we must live
with an API change, but if possible, let's try to find a solution that is
backward compatible.
----------------------------------------------------------------------
Comment By: Ovidiu Sas (osas)
Date: 2008-03-18 20:46
Message:
Logged In: YES
user_id=1395524
Originator: YES
Hello Dan,
So far, there is one single module on top of the dialog that needs to be
updates (the sst module) and the API change is pretty simple.
Right now, the direction is computed for every request, but not computed
for responses.
Storing it into the dialog cell means more memory and special cases for
requests vs responses:
- for requests it is already computed,
- for responses needs to be computed.
So we will need to track the type of the SIP message that is processes in
order to distinguish between the two (and we can have races between a real
SIP message - we have several callbacks for the same SIP reply - and a tm
timer timeout for destroying the transaction).
Let's hear other's opinions on this matter and we will go to the path that
best fits all.
Regards,
Ovidiu Sas
----------------------------------------------------------------------
Comment By: Dan (dan_pascu)
Date: 2008-03-18 20:29
Message:
Logged In: YES
user_id=1296758
Originator: NO
Ovidiu,
I don't like this approach because it is very disruptive. It adds an
unnecessary API change that will force one to go back and fix every module
on top of the dialog module and also maintain distinct versions of them for
distinct versions of openser.
I think a more practical approach would be to store the direction on the
dialog structure (a flag) wherever it processes a message belonging to the
dialog and then offer a function that can be called inside a callback that
returns the direction flag from the dialog structure, thus avoiding an API
change that is not backward compatible and also avoiding to compute it
everytime it is asked for.
----------------------------------------------------------------------
Comment By: Ovidiu Sas (osas)
Date: 2008-03-18 19:47
Message:
Logged In: YES
user_id=1395524
Originator: YES
Hello Bogdan,
Here's another version of the patch.
This one will reuse the already computed dialog, but instead of creating a
new API, it enhances the old one by passing the direction as a parameter to
the callback function (sst module updated).
-void run_dlg_callbacks( int type , struct dlg_cell *dlg, struct sip_msg
*msg);
+void run_dlg_callbacks( int type , struct dlg_cell *dlg, struct sip_msg
*msg, unsigned int direction);
Please let me know your thoughts.
Regards,
Ovidiu Sas
File Added: dialog_direction.patch.gz
----------------------------------------------------------------------
Comment By: Ovidiu Sas (osas)
Date: 2008-03-03 17:52
Message:
Logged In: YES
user_id=1395524
Originator: YES
Hi Bogdan,
That was my initial approach, but I found it kind of intrusive because not
all the modules sitting on top of the dialog will care about the direction,
that's the reason that I went with the current approach.
I can rework the patch to follow that approach.
And if we follow your approach, another option would be to pass the
direction in the callback functions along with the did, type, msg and param
instead of creating this new API method. Let me know your thoughts.
Regards,
Ovidiu Sas
----------------------------------------------------------------------
Comment By: Bogdan-Andrei Iancu (bogdan_iancu)
Date: 2008-03-03 17:34
Message:
Logged In: YES
user_id=1275325
Originator: NO
Hi Ovidiu,
I delayed a bit the patch as I wanted to re-work it a bit - I think it is
not really necessary to detect the direction at each function call, as the
direction is detected at match time. So, more or less we just need to
remember the direction :)
Regards,
Bogdan
----------------------------------------------------------------------
Comment By: Ovidiu Sas (osas)
Date: 2008-03-01 19:30
Message:
Logged In: YES
user_id=1395524
Originator: YES
If there are no objections, I will push this patch in next week.
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=743022&aid=1872331&group_id=139143
More information about the Devel
mailing list