<p></p>
<p><b>@henningw</b> commented on this pull request.</p>
<p dir="auto">Thanks for the PR. I've done a review and added a few comments, mostly related to memroy management and error handling.</p><hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3240#discussion_r976874692">src/modules/presence_reginfo/notify_body.c</a>:</p>
<pre style='color:#555'>> + 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");
</pre>
<p dir="auto">You could use the PKG_MEM_ERROR here</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3240#discussion_r976875342">src/modules/presence_reginfo/notify_body.c</a>:</p>
<pre style='color:#555'>> + /* 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,
</pre>
<p dir="auto">remove the commented out code if its not needed anymore</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3240#discussion_r976876702">src/modules/presence_reginfo/notify_body.c</a>:</p>
<pre style='color:#555'>> +
+ /* 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));
</pre>
<p dir="auto">What happened to the previously allocated memory in line 97?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3240#discussion_r976876803">src/modules/presence_reginfo/notify_body.c</a>:</p>
<pre style='color:#555'>> + * 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");
</pre>
<p dir="auto">again PKG_MEM_ERROR</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3240#discussion_r976877949">src/modules/presence_reginfo/notify_body.c</a>:</p>
<pre style='color:#555'>> + }
+ } // 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);
</pre>
<p dir="auto">You could also use PKG_MEM_ERROR, to have it everywhere the same<br>
And you probably also want to exit and cleanup the previous allocated memory, right?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3240#discussion_r976880002">src/modules/presence_reginfo/notify_body.c</a>:</p>
<pre style='color:#555'>> + 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);
</pre>
<p dir="auto">you probably also want to check for negative return (encoding error)</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3240#discussion_r976880609">src/modules/presence_reginfo/notify_body.c</a>:</p>
<pre style='color:#555'>> + 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");
</pre>
<p dir="auto">you could use PKG_MEM_ERROR or PKG_MEM_ERROR_FMT</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/3240#discussion_r976880768">src/modules/presence_reginfo/notify_body.c</a>:</p>
<pre style='color:#555'>> + 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");
</pre>
<p dir="auto">see above</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/kamailio/kamailio/pull/3240#pullrequestreview-1115931959">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABO7UZJSP7PE3LAPGPK66ALV7NLHJANCNFSM6AAAAAAQONRSIE">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/ABO7UZMG5UGAVN3PPAYW76LV7NLHJA5CNFSM6AAAAAAQONRSIGWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTSCQPCTO.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><kamailio/kamailio/pull/3240/review/1115931959</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/kamailio/kamailio/pull/3240#pullrequestreview-1115931959",
"url": "https://github.com/kamailio/kamailio/pull/3240#pullrequestreview-1115931959",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>