[sr-dev] [kamailio/kamailio] presence_reginfo: Add option for aggregating reginfo presentities (PR #3240)

[Henning Westerholt]

@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 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.kamailio.org/pipermail/sr-dev/attachments/20220921/83d8be7c/attachment-0001.htm>