[kamailio/kamailio] [WIP] presence_reginfo: Add option for aggregating reginfo presentities (PR #3240)
16 Sep
2022
16 Sep
'22
11:16 a.m.
<!-- 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)
- [ ] 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
Right now the `presence_reginfo` module does not perform any aggregation on presentities
from the same AoR. While this is generally ok for new notifies driven by new publishes,
when a new subscription is received only the most recent presentity is notified. This PR
adds a new parameter, disabled by default to preserve compatibility, which merges all
presentities for the same AoR into a single document.
For example, two registrations from the same AoR will create the following document:
```<?xml version="1.0"?>
<reginfo xmlns="urn:ietf:params:xml:ns:reginfo"
version="23" state="full">
<registration aor="sip:user2@example.com"
id="0x7f9b6bbf9cf8" state="active">
<contact id="0x7f9b6bbf3ea8" state="active"
event="created" expires="300"
callid=&quot;23439394725296-103541137625012(a)192.168.10.130&quot;
cseq="38" received=""
path="&lt;sip:172.23.42.1;lr;received=sip:uac.public.ip:60015;r2=on&gt;,&lt;sip:kamailio.public.ip;lr;received=sip:uac.public.ip:60015;r2=on&gt;"
user_agent="MyUA 1.2.3.4">
<uri>sip:user2@192.168.10.130:5060</uri>
</contact>
</registration>
<registration aor="sip:user2@example.com"
id="0x7f46836f63b8" state="active">
<contact id="0x7f46836f64e0" state="active"
event="refreshed" expires="300"
callid=&quot;15796302815379-26687127665413(a)192.168.10.130&quot;
cseq="38" received=""
path="&lt;sip:172.23.42.1;lr;received=sip:uac.public.ip:5099;r2=on&gt;,&lt;sip:kamailio.public.ip;lr;received=sip:uac.public.ip:5099;r2=on&gt;"
user_agent="MyUA 1.2.3.4">
<uri>sip:user2@192.168.10.130:5099</uri>
</contact>
</registration>
</reginfo>
```
Example of a body with two registrations with different bodies:
```
<?xml version="1.0"?>
<reginfo xmlns="urn:ietf:params:xml:ns:reginfo"
version="21" state="full">
<registration aor="sip:user2@example.com"
id="0x7f9b6bc55540" state="terminated"/>
<registration aor="sip:user2@example.com"
id="0x7f46836f63b8" state="active">
<contact id="0x7f46836f64e0" state="active"
event="refreshed" expires="300"
callid=&quot;15796302815379-26687127665413(a)192.168.10.130&quot;
cseq="36" received=""
path="&lt;sip:172.23.42.1;lr;received=sip:uac.public.ip:5099;r2=on&gt;,&lt;sip:kamailio.public.ip;lr;received=sip:uac.public.ip:5099;r2=on&gt;"
user_agent="MyUA 1.2.3.4">
<uri>sip:user2@192.168.10.130:5099</uri>
</contact>
</registration>
</reginfo>
```
What is maybe missing: if the aor and id are the same on different presentities (is
possible ?) we should not put it twice in the same body, because aor+id identify an unique
registration.
Most of the code has been adapted from `presence_dialoginfo` so I have preserved the
original authors, too.
Since is a bit complex PR (for me, at least) I'm begging for a in deep review ;)
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3240
-- Commit Summary --
* [presence_reginfo] Add option for aggregating presentities
-- File Changes --
M src/modules/presence_reginfo/Makefile (19)
M src/modules/presence_reginfo/add_events.c (17)
M src/modules/presence_reginfo/doc/presence_reginfo_admin.xml (21)
A src/modules/presence_reginfo/notify_body.c (322)
A src/modules/presence_reginfo/notify_body.h (43)
M src/modules/presence_reginfo/presence_reginfo.c (8)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3240.patch
https://github.com/kamailio/kamailio/pull/3240.diff
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3240
You are receiving this because you are subscribed to this thread.
Message ID: &lt;kamailio/kamailio/pull/3240(a)github.com&gt;
16 Sep
16 Sep
11:22 a.m.
@xadhoom pushed 1 commit.
2f2288b3c9f9837d0f0c0f7d17d42db3bf4554f2 presence_reginfo: Add option for aggregating
presentities
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/3240/files/e3631b31440fcdb58aa308…
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3240/push/11046756008(a)github.com>
21 Sep
21 Sep
2:59 p.m.
New subject: [kamailio/kamailio] presence_reginfo: Add option for aggregating reginfo presentities (PR #3240)
@henningw commented on this pull request.
Thanks for the PR. I've done a review and added a few comments, mostly related to
memroy management and error handling.
+ str *body = NULL;
+
+ xmlDocPtr doc = NULL;
+ xmlNodePtr root_node = NULL;
+ xmlNsPtr namespace = NULL;
+ xmlNodePtr p_root = NULL;
+ xmlDocPtr *xml_array;
+ xmlNodePtr node = NULL;
+ xmlNodePtr next_node = NULL;
+
+ LM_DBG("[pres_user]=%.*s [pres_domain]= %.*s, [n]=%d\n", pres_user->len,
+ pres_user->s, pres_domain->len, pres_domain->s, n);
+
+ xml_array = (xmlDocPtr *)pkg_malloc(n * sizeof(xmlDocPtr));
+ if(xml_array == NULL) {
+ LM_ERR("while allocating memory");
You could use the PKG_MEM_ERROR here
+ /* The version must be increased for each new
document and is a 32bit int.
+ * As the version is different for each watcher, we
can not set here the
+ * correct value. Thus, we just put here a placeholder which will be
+ * replaced by the correct value in the aux_body_processing callback.
+ * Thus we have CPU intensive XML aggregation only once and can use
+ * quick search&replace in the per-watcher aux_body_processing callback.
+ * We use 11 chracters as an signed int (although RFC says unsigned int we
+ * use signed int as presence module stores "version" in DB as
+ * signed int) has max. 10 characters + 1 character for the sign
+ */
+ xmlNewProp(root_node, BAD_CAST "version", BAD_CAST "00000000000");
+ xmlNewProp(root_node, BAD_CAST "state", BAD_CAST "full");
+
+ /* loop over all bodies and create the aggregated body */
+ for(i = 0; i < j; i++) {
+ // LM_DBG("[n]=%d, [i]=%d, [j]=%d xml_array[i]=%p\n", n, i, j,
remove the commented out code if its not needed anymore
+
+ /* we do not copy the node, but unlink it
and then add it ot the new node
+ * this destroys the original document but we do not need it anyway.
+ */
+ xmlUnlinkNode(node);
+ if(xmlAddChild(root_node, node) == NULL) {
+ xmlFreeNode(node);
+ LM_ERR("while adding child\n");
+ goto error;
+ }
+ } // end of loop over registration elements
+ }
+ } // end of loop over all bodies
+
+ // convert to string & cleanup
+ xml_array = (xmlDocPtr *)pkg_malloc(n * sizeof(xmlDocPtr));
What happened to the previously allocated memory in line 97?
+ * this destroys the original document but we do
not need it anyway.
+ */
+ xmlUnlinkNode(node);
+ if(xmlAddChild(root_node, node) == NULL) {
+ xmlFreeNode(node);
+ LM_ERR("while adding child\n");
+ goto error;
+ }
+ } // end of loop over registration elements
+ }
+ } // end of loop over all bodies
+
+ // convert to string & cleanup
+ xml_array = (xmlDocPtr *)pkg_malloc(n * sizeof(xmlDocPtr));
+ if(xml_array == NULL) {
+ LM_ERR("while allocating memory");
again PKG_MEM_ERROR
+ }
+ } // end of loop over registration
elements
+ }
+ } // end of loop over all bodies
+
+ // convert to string & cleanup
+ xml_array = (xmlDocPtr *)pkg_malloc(n * sizeof(xmlDocPtr));
+ if(xml_array == NULL) {
+ LM_ERR("while allocating memory");
+ return NULL;
+ }
+ memset(xml_array, 0, n * sizeof(xmlDocPtr));
+
+ body = (str *)pkg_malloc(sizeof(str));
+ if(body == NULL) {
+ ERR_MEM(PKG_MEM_STR);
You could also use PKG_MEM_ERROR, to have it everywhere the same
And you probably also want to exit and cleanup the previous allocated memory, right?
+ LM_ERR("body string too short!\n");
+ return NULL;
+ }
+ version_start = strstr(body->s + 30, "version=");
+ if(!version_start) {
+ LM_ERR("version string not found!\n");
+ return NULL;
+ }
+ version_start += 9;
+
+ /* safety check for placeholder - if it is body not set by the module,
+ * don't update the version */
+ if(strncmp(version_start, "00000000000\"", 12) != 0)
+ return NULL;
+
+ version_len = snprintf(version, MAX_INT_LEN + 2, "%d\"",
subs->version);
you probably also want to check for negative return (encoding error)
+ version_start += 9;
+
+ /* safety check for placeholder - if it is body not set by the module,
+ * don't update the version */
+ if(strncmp(version_start, "00000000000\"", 12) != 0)
+ return NULL;
+
+ version_len = snprintf(version, MAX_INT_LEN + 2, "%d\"",
subs->version);
+ if(version_len >= MAX_INT_LEN + 2) {
+ LM_ERR("failed to convert 'version' to string\n");
+ return NULL;
+ }
+
+ aux_body = (str *)pkg_malloc(sizeof(str));
+ if(aux_body == NULL) {
+ LM_ERR("error allocating memory for aux body str\n");
you could use PKG_MEM_ERROR or PKG_MEM_ERROR_FMT
+ version_len = snprintf(version, MAX_INT_LEN + 2,
"%d\"", subs->version);
+ if(version_len >= MAX_INT_LEN + 2) {
+ LM_ERR("failed to convert 'version' to string\n");
+ return NULL;
+ }
+
+ aux_body = (str *)pkg_malloc(sizeof(str));
+ if(aux_body == NULL) {
+ LM_ERR("error allocating memory for aux body str\n");
+ return NULL;
+ }
+ memset(aux_body, 0, sizeof(str));
+ aux_body->s = (char *)pkg_malloc(body->len * sizeof(char));
+ if(aux_body->s == NULL) {
+ pkg_free(aux_body);
+ LM_ERR("error allocating memory for aux body buffer\n");
see above
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3240#pullrequestreview-1115931959
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3240/review/1115931959(a)github.com>
22 Sep
22 Sep
3:29 a.m.
New subject: [kamailio/kamailio] presence_reginfo: Add option for aggregating reginfo presentities (PR #3240)
@xadhoom pushed 1 commit.
3b87761906b509ee601199793545f5b34d371e05 presence_reginfo: Add option for aggregating
presentities
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/3240/files/2f2288b3c9f9837d0f0c0f…
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3240/push/11100668736(a)github.com>
3:32 a.m.
New subject: [kamailio/kamailio] presence_reginfo: Add option for aggregating reginfo presentities (PR #3240)
@xadhoom commented on this pull request.
+
+ /* we do not copy the node, but unlink it
and then add it ot the new node
+ * this destroys the original document but we do not need it anyway.
+ */
+ xmlUnlinkNode(node);
+ if(xmlAddChild(root_node, node) == NULL) {
+ xmlFreeNode(node);
+ LM_ERR("while adding child\n");
+ goto error;
+ }
+ } // end of loop over registration elements
+ }
+ } // end of loop over all bodies
+
+ // convert to string & cleanup
+ xml_array = (xmlDocPtr *)pkg_malloc(n * sizeof(xmlDocPtr));
This was indeed a typo, no reason to init the array again, sorry for that,
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3240#discussion_r977299122
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3240/review/1116525982(a)github.com>
3:34 a.m.
New subject: [kamailio/kamailio] presence_reginfo: Add option for aggregating reginfo presentities (PR #3240)
@xadhoom commented on this pull request.
+ }
+ } // end of loop over registration
elements
+ }
+ } // end of loop over all bodies
+
+ // convert to string & cleanup
+ xml_array = (xmlDocPtr *)pkg_malloc(n * sizeof(xmlDocPtr));
+ if(xml_array == NULL) {
+ LM_ERR("while allocating memory");
+ return NULL;
+ }
+ memset(xml_array, 0, n * sizeof(xmlDocPtr));
+
+ body = (str *)pkg_malloc(sizeof(str));
+ if(body == NULL) {
+ ERR_MEM(PKG_MEM_STR);
Well, ERR_MEM does implicit goto to error where the memory is cleaned. The 2nd alloc of
xml_array was a typo and has been removed. Now we have a single xml_array that's freed
in the error handling. or not?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3240#discussion_r977300634
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3240/review/1116528090(a)github.com>
3:35 a.m.
New subject: [kamailio/kamailio] presence_reginfo: Add option for aggregating reginfo presentities (PR #3240)
@xadhoom commented on this pull request.
+ LM_ERR("body string too short!\n");
+ return NULL;
+ }
+ version_start = strstr(body->s + 30, "version=");
+ if(!version_start) {
+ LM_ERR("version string not found!\n");
+ return NULL;
+ }
+ version_start += 9;
+
+ /* safety check for placeholder - if it is body not set by the module,
+ * don't update the version */
+ if(strncmp(version_start, "00000000000\"", 12) != 0)
+ return NULL;
+
+ version_len = snprintf(version, MAX_INT_LEN + 2, "%d\"",
subs->version);
Well, does not change the flow but yes, is useful for debugging, added.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3240#discussion_r977301700
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3240/review/1116529583(a)github.com>
3:37 a.m.
New subject: [kamailio/kamailio] presence_reginfo: Add option for aggregating reginfo presentities (PR #3240)
@henningw thanks a lot for the valuable feedback, I've fixed all the points, except
one which seems ok to me (see above).
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3240#issuecomment-1254644247
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3240/c1254644247(a)github.com>
3:46 a.m.
New subject: [kamailio/kamailio] presence_reginfo: Add option for aggregating reginfo presentities (PR #3240)
@xadhoom pushed 1 commit.
40282006e6c548bfd35af27ce4ea6715bc4cdbec presence_reginfo: Add option for aggregating
presentities
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/3240/files/3b87761906b509ee601199…
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3240/push/11100824634(a)github.com>
5:08 a.m.
New subject: [kamailio/kamailio] presence_reginfo: Add option for aggregating reginfo presentities (PR #3240)
@henningw commented on this pull request.
+ }
+ } // end of loop over registration
elements
+ }
+ } // end of loop over all bodies
+
+ // convert to string & cleanup
+ xml_array = (xmlDocPtr *)pkg_malloc(n * sizeof(xmlDocPtr));
+ if(xml_array == NULL) {
+ LM_ERR("while allocating memory");
+ return NULL;
+ }
+ memset(xml_array, 0, n * sizeof(xmlDocPtr));
+
+ body = (str *)pkg_malloc(sizeof(str));
+ if(body == NULL) {
+ ERR_MEM(PKG_MEM_STR);
You are right of course, missed the goto in the macro.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3240#discussion_r977400944
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3240/review/1116672572(a)github.com>
5:09 a.m.
New subject: [kamailio/kamailio] presence_reginfo: Add option for aggregating reginfo presentities (PR #3240)
Thanks for the update. Lets wait a few more days for other feedback, and then it could be
probably merged.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3240#issuecomment-1254744713
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3240/c1254744713(a)github.com>
27 Sep
27 Sep
10:11 a.m.
New subject: [kamailio/kamailio] presence_reginfo: Add option for aggregating reginfo presentities (PR #3240)
Merged #3240 into master.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3240#event-7469038624
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3240/issue_event/7469038624(a)github.com>
10:11 a.m.
New subject: [kamailio/kamailio] presence_reginfo: Add option for aggregating reginfo presentities (PR #3240)
Thanks, merged
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3240#issuecomment-1259569365
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3240/c1259569365(a)github.com>
721
days inactive
732
days old
12 comments
2 participants
participants (2)
-
Henning Westerholt -
Matteo