<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [X] Commit message has the format required by CONTRIBUTING guide - [X] Commits are split per component (core, individual modules, libs, utils, ...) - [X] Each component has a single commit (if not, squash them into one commit) - [X] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [ ] Small bug fix (non-breaking change which fixes an issue) - [X] New feature (non-breaking change which adds new functionality) - [X] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [ ] PR should be backported to stable branches - [X] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description I've marked this as a "breaking change" only because it slightly alters the error handling of the RPC dlg.list_match command, to draw attention to it. But the actual functionality is unchanged.
This PR adds a new script function to the dialog module: `dlg_get_matches`. This has the same behaviour as the RPC `dlg.list_match` command, but allows use in scripts. The results are returned in an XAVP of a specified name, as an array of fields.
``` dlg_get_matches(mkey, mop, mval, xavp_name[, max_results]);
Returns number of matches found. Or -1 if there's an error. ```
An example of use would be: ``` xlog("Extension is: $var(ext)\n");
$avp(dlg_count) = dlg_get_matches("turi", "sw", "sip:nexusone$var(ext)@", "dlg_matches");
xlog("Got $avp(dlg_count) matches.\n"); $var(i) = 0; $var(matched) = 0; while ($var(i) < $avp(dlg_count)) { //Skip dialogs that are not early state if ($xavp(dlg_matches[$var(i)]=>state) == 2) { xlog("Found matching dlg[$var(i)]: $xavp(dlg_matches[$var(i)]=>from_uri)\n"); } } ```
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3005
-- Commit Summary --
* dialog: Adding dlg_get_matches function based on RPC dlg.list_match command. * dialog: Adding documentation for dlg_get_matches function. * dialog: Refactored dlg_get_matches and dlg.list_match RPC command to use a shared dlg_list_matches function.
-- File Changes --
M src/modules/dialog/dialog.c (793) M src/modules/dialog/doc/dialog_admin.xml (92)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3005.patch https://github.com/kamailio/kamailio/pull/3005.diff
@rhys-hanrahan pushed 1 commit.
1978981de79dd64e41f39a20a7c88c439856dc37 dialog: Refactored dlg_get_matches and dlg.list_match RPC command to use a shared dlg_list_matches function.
@rhys-hanrahan pushed 1 commit.
8c02b56b91b3ab5560472a60c3e6073deb59854c dialog: Refactored dlg_get_matches and dlg.list_match RPC command to use a shared dlg_list_matches function.
@rhys-hanrahan pushed 1 commit.
cbaa1071f83cab3a99142c449410a9a2cd94efbc dialog: Refactored dlg_get_matches and dlg.list_match RPC command to use a shared dlg_list_matches function.
@rhys-hanrahan pushed 1 commit.
020cef7dad263c3319d9d7b44faea4a20859c883 dialog: Refactored dlg_get_matches and dlg.list_match RPC command to use a shared dlg_list_matches function.
@rhys-hanrahan pushed 1 commit.
d706668935ca9e4e3e7fc7de5a232363e13fed69 dialog: Refactored dlg_get_matches and dlg.list_match RPC command to use a shared dlg_list_matches function.
@rhys-hanrahan pushed 2 commits.
535f6c92c0835fb6246c51fd380e7ae99886f479 dialog: Adding documentation for dlg_get_matches function. aaf2bf7ca535b591e558d30b22b63380345db7d8 dialog: Refactored dlg_get_matches and dlg.list_match RPC command to use a shared dlg_list_matches function.
For some reason I had totally ignored that dlg vars, profiles and callbacks are all referenced by the RPC command (and probably should be by my function). Given I don't know what the call backs might be doing to the original dialogs, it doesn't seem safe anymore to make copies of the original dialogs.
I just tried re-writing the code to return an array of pointers to the original dialogs, to avoid copying them. This would mean I need to perform locking around any results of `dlg_list_matches`. However I've realised that I won't know what "table entry" to lock as this could be different for each match.
I'm feeling a bit stuck right now - I suppose I could use a struct that tracks each dlg ptr match and what table index it belongs to, when returning results. I was trying to avoid creating extra things just for this small bit of code, though.
Any suggestions on this would be appreciated. Maybe it's just easier to not share the matching logic and keep it simple and just duplicate all the matching logic?
I haven't checked the patches of the PR so far, but fyi, you should be able to execute an RPC command with jsonrpcs_exec(), get the output in a variable that you can parse with jansson module.
Having a dedicated function for config is probably better if you need to use it often.
@rhys-hanrahan pushed 1 commit.
513d0aed29d136814d56067337237ec610afc32f dialog: Re-factored dlg_list_matches to use dlg_cell_match_t to return matches.
Thanks Daniel! As it happens, I just finished re-writing this code. I did end up creating a struct to return each matching dlg ptr, along with it's table entry index, to allow for locking. After some basic testing, this seems to be working for me, without copying the dialogs.
There's more cleanup I'd do before merging but I wanted to check if you have any issues with this overall approach before spending more time on it.
Thanks for the tip on jsonrpcs_exec - I was trying to find something like this originally but couldn't. As of right now, the solution in this PR is working well for me. But l will look at jsonrpcs_exec as a plan B.
Thanks!
@rhys-hanrahan pushed 3 commits.
66df632ae92187e37dc2f7f10a1bfdc8c94a1e31 dialog: Adding documentation for dlg_get_matches function. 04279a2939ab4bc95012a6dd8cb555d330b7c4b2 dialog: Refactored dlg_get_matches and dlg.list_match RPC command to use a shared dlg_list_matches function. 30926c2d3c0cbcd157a2622ee68684e5fc26aea0 dialog: Re-factored dlg_list_matches to use dlg_cell_match_t to return matches.
@rhys-hanrahan pushed 2 commits.
6d08a9aca1a83a5d02d5c9c8b79f63933d4ff5a7 dialog: Fixed bug with entry not being unsigned. a2eb95d6eab7a03bc96b969afc99f155d28a65d6 dialog: Added output of dialog vars and profiles to dlg_get_matches xavp output
@rhys-hanrahan pushed 3 commits.
ef81f94f34d1d9ec16dc93ab709f96c6a76b6ea5 dialog: Adding dlg_get_matches function based on RPC dlg.list_match command. d24e51602a13ab4ae06080d7fbd00189df7ff585 dialog: Adding documentation for dlg_get_matches function. 78ba1db463cbb09a758a95ee7d51a056d3ffaa9d dialog: Refactored dlg_get_matches and dlg.list_match RPC command to use a shared dlg_list_matches function.
@rhys-hanrahan pushed 1 commit.
a677eae1ba06c0517c01969ca7510b3f3d441f54 dialog: Refactored dlg_get_matches and dlg.list_match RPC command to use a shared dlg_list_matches function.
@miconda Hi Daniel, just confirming this is cleaned up now and ready for whenever you have a chance to look at it - but let me know if you suggest any other changes. The only thing I haven't done is move the `pv_parse_format` into the fixup function, because as per the notes in my PR I had some weird segfaults there. But I can work on this if needed.
Otherwise, I've left the PR split into two commits in-case you're against sharing the matching logic for the dlg.match RPC command and this new dlg_add_matches, though I am happy with how things ended up with the shared function.
Thanks! Rhys.
Looking looking quickly at the final patch, there seems to be a race that can happen between building the list of matching dialogs and using the items in the list to push to xavps or to rpc output. The slots in the dialog hash table are released after it was walked and matching dialogs were linked in the matching list. It can take time till other slots are walked for matching other dialogs and during that time some dialogs in the previous slots can be destroyed (e.g., bye or timeout). This is one example, but dialogs might be destroyed also during the walk of matching list to convert it to xavp or rpc output.
The clone to xavp or printing to rpc output should happen when the slot of the matching dialog is acquired and before it is released again. If the lock is released after linking to matching list and then re-acquired for cloning to xavp or printing to rpc, then such races can happen and dlg field in matching list may point to invalid memory address.
Thanks Daniel! Really appreciate the feedback, and thanks for catching that race condition. Unfortunately I'm still in a lab setting so haven't come across anything like that.
I will look at changing to use a callback style with variable arguments. For some reason at the time I didn't think this was an option but I can't see why this wouldn't work at the moment? And this would allow me to maintain the same lock all the way through.
I will work on this as soon as I get some time and come back to you. Thanks!
@rhys-hanrahan Any update on this one? Just wanted to ask because the freeze period for the next kamailio release starts soon.
@henningw Sorry for the delayed reply. I'm still planning to re-work this with the feedback, but unfortunately my priorities shifted away from Kamailio for the moment. Hoping to come back to this in the next few weeks but I guess I'll be missing the freeze for next release, unfortunately.
@rhys-hanrahan any update from your side?
Closing this one being now 1 year old. When the code is updated, a new PR can be made if easier, or this one re-opened.
Closed #3005.
-
Daniel-Constantin Mierla -
Henning Westerholt
-
rhys-hanrahan