@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

@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,

@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.

@xadhoom pushed 1 commit.

40282006e6c548bfd35af27ce4ea6715bc4cdbec presence_reginfo: Add option for aggregating presentities

Thanks for the update. Lets wait a few more days for other feedback, and then it could be probably merged.

Thanks, merged