<p><b>@henningw</b> commented on this pull request.</p>
<p>Thank you Wolfgang, great contribution.</p>
<p>I have added a few comments, mainly to memory management, and some spelling/formatting fixes. I found nothing serious in my review. Would be great if you could have a look to them, after this the module should be committed to git master.</p><hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2031#discussion_r313089325">src/modules/lost/lost.c</a>:</p>
<pre style='color:#555'>> +static int mod_init(void)
+{
+ LM_DBG("init lost module\n");
+
+ if(httpc_load_api(&httpapi) != 0) {
+ LM_ERR("Can not bind to http_client API \n");
+ return -1;
+ }
+
+ LM_DBG("**** init lost module done.\n");
+
+ return 0;
+}
+
+/* Child initialization function */
+static int child_init(int rank)
</pre>
<p>Maybe just return 0?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2031#discussion_r313090544">src/modules/lost/pidf.c</a>:</p>
<pre style='color:#555'>> + while(cur) {
+ if(xmlStrcasecmp(cur->name, (unsigned char *)name) == 0)
+ return cur;
+ cur = cur->next;
+ }
+ return NULL;
+}
+
+xmlNodePtr xmlNodeGetNodeByName(
+ xmlNodePtr node, const char *name, const char *ns)
+{
+ xmlNodePtr cur = node;
+ while(cur) {
+ xmlNodePtr match = NULL;
+ if(xmlStrcasecmp(cur->name, (unsigned char *)name) == 0) {
+ if(!ns
</pre>
<p>The formatting is a bit broken here in this if case.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2031#discussion_r313091255">src/modules/lost/pidf.c</a>:</p>
<pre style='color:#555'>> + * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version
+ *
+ * Kamailio is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ * History:
+ * --------
+ * 2007-04-14 initial version (anca)
</pre>
<p>Remove the history comment from the source file, also in the header file below.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2031#discussion_r313091694">src/modules/lost/pidf.h</a>:</p>
<pre style='color:#555'>> + *
+ * Kamailio is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ * History:
+ * --------
+ * 2006-08-15 initial version (anca)
+ */
+
+/*! \file
</pre>
<p>Version comment (see above) and the doxygen is also wrong (the one in the .c is correct).</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2031#discussion_r313092589">src/modules/lost/utilities.c</a>:</p>
<pre style='color:#555'>> + * freess a location object
+ */
+void lost_free_loc(p_loc_t ptr)
+{
+ pkg_free(ptr->identity);
+ pkg_free(ptr->urn);
+ pkg_free(ptr->longitude);
+ pkg_free(ptr->latitude);
+ pkg_free(ptr);
+}
+
+/*
+ * lost_new_loc(urn)
+ * creates a new location object in private memory and returns a pointer
+ */
+p_loc_t lost_new_loc(str rurn)
</pre>
<p>Makes it sense to return a invalid pointer if the one or two of the allocations fail? You also could do a memset on an invalid pointer.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2031#discussion_r313092780">src/modules/lost/functions.c</a>:</p>
<pre style='color:#555'>> + pidf.s);
+ goto err;
+ }
+
+ LM_DBG("xml (pidf-lo) recovered\n");
+ }
+
+ root = xmlDocGetRootElement(doc);
+ if(!root) {
+ LM_ERR("empty pidf-lo document\n");
+ goto err;
+ }
+ if((!xmlStrcmp(root->name, (const xmlChar *)"presence"))
+ || (!xmlStrcmp(root->name, (const xmlChar *)"locationResponse"))) {
+ /* get the geolocation: point or circle, urn, ... */
+ loc = lost_new_loc(urn);
</pre>
<p>This can fail</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2031#discussion_r313093443">src/modules/lost/utilities.c</a>:</p>
<pre style='color:#555'>> +
+ ptr->identity = id;
+ ptr->urn = urn;
+ ptr->longitude = NULL;
+ ptr->latitude = NULL;
+ ptr->radius = 0;
+ ptr->recursive = 0;
+
+ return ptr;
+}
+
+/*
+ * lost_get_content(node, name, lgth)
+ * gets a nodes "name" content and returns string allocated in private memory
+ */
+char *lost_get_content(xmlNodePtr node, const char *name, int *lgth)
</pre>
<p>Same comment as lost_new_loc function about memory allocation and checks.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2031#discussion_r313093624">src/modules/lost/utilities.c</a>:</p>
<pre style='color:#555'>> + memset(cnt, 0, len + 1);
+ memcpy(cnt, content, len);
+ cnt[len] = '\0';
+
+ *lgth = strlen(cnt);
+
+ xmlFree(content);
+
+ return cnt;
+}
+
+/*
+ * lost_get_property(node, name, lgth)
+ * gets a nodes property "name" and returns string allocated in private memory
+ */
+char *lost_get_property(xmlNodePtr node, const char *name, int *lgth)
</pre>
<p>see above</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2031#discussion_r313094019">src/modules/lost/utilities.c</a>:</p>
<pre style='color:#555'>> + memcpy(doc, (char *)xmlbuff, buffersize);
+ doc[buffersize] = '\0';
+
+ *lgth = strlen(doc);
+
+ xmlFree(xmlbuff);
+ xmlFreeDoc(request);
+
+ return doc;
+}
+
+/*
+ * lost_find_service_request(loc, lgth)
+ * assembles and returns findService request string (allocated in private memory)
+ */
+char *lost_find_service_request(p_loc_t loc, int *lgth)
</pre>
<p>as above</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2031#discussion_r313094266">src/modules/lost/utilities.c</a>:</p>
<pre style='color:#555'>> + sscanf(content, "%d", &iRadius);
+ loc->radius = iRadius;
+ ret = 0;
+ }
+
+ if(ret < 0) {
+ LM_ERR("could not parse location information\n");
+ }
+ return ret;
+}
+
+/*
+ * lost_held_location_request(id, lgth)
+ * assembles and returns locationRequest string (allocated in private memory)
+ */
+char *lost_held_location_request(char *id, int *lgth)
</pre>
<p>see above - and are the XML operations working always as well?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2031#discussion_r313095057">src/modules/lost/utilities.c</a>:</p>
<pre style='color:#555'>> + memset(cnt, 0, len + 1);
+ memcpy(cnt, content, len);
+ cnt[len] = '\0';
+
+ *lgth = strlen(cnt);
+
+ xmlFree(content);
+
+ return cnt;
+}
+
+/*
+ * lost_get_childname(name, lgth)
+ * gets a nodes child name and returns string allocated in private memory
+ */
+char *lost_get_childname(xmlNodePtr node, const char *name, int *lgth)
</pre>
<p>see above</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2031#discussion_r313098911">src/modules/lost/functions.c</a>:</p>
<pre style='color:#555'>> +
+ pvurl.flags = PV_VAL_STR;
+ psurl = (pv_spec_t *)_url;
+ psurl->setf(_m, &psurl->pvp, (int)EQ_T, &pvurl);
+
+ pverr.rs = err;
+ pverr.rs.s = err.s;
+ pverr.rs.len = err.len;
+
+ pverr.flags = PV_VAL_STR;
+ pserr = (pv_spec_t *)_err;
+ pserr->setf(_m, &pserr->pvp, (int)EQ_T, &pverr);
+
+ return (err.len > 0) ? LOST_SERVER_ERROR : LOST_SUCCESS;
+
+err:
</pre>
<p>You are basically leaking memory here, in case one of the memory allocation success initial and a later one (or another library call) is jumping into err. It is probably ncessary to add some (if XXX.s) { pkg_free(XXX.s) } calls here.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2031#discussion_r313099520">src/modules/lost/lost.c</a>:</p>
<pre style='color:#555'>> +/*
+ * Fix 4 lost_held_query params: con (string/pvar)
+ * and pidf, url, err (writable pvar).
+ */
+static int fixup_lost_held_query(void **param, int param_no)
+{
+ if(param_no == 1) {
+ return fixup_spve_null(param, 1);
+ }
+ if((param_no == 2) || (param_no == 3) || (param_no == 4)) {
+ if(fixup_pvar_null(param, 1) != 0) {
+ LM_ERR("failed to fixup result pvar\n");
+ return -1;
+ }
+ if(((pv_spec_t *)(*param))->setf == NULL) {
+ LM_ERR("result pvar is not writeble\n");
</pre>
<p>you meant probably not "writable" - copy and paste error also below</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2031#discussion_r313099811">src/modules/lost/README</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,238 @@
+LOST Module
</pre>
<p>Remove the README - it is autogenerated after you commited the XML doc files.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/kamailio/kamailio/pull/2031?email_source=notifications&email_token=ABO7UZN47H6P2TAKIOY2JITQEG56FA5CNFSM4ILBBGJKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBJWIRA#pullrequestreview-273900612">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABO7UZMFFRME4YT64Z6DEC3QEG56FANCNFSM4ILBBGJA">mute the thread</a>.<img src="https://github.com/notifications/beacon/ABO7UZKTC3BVSRJONITGSVDQEG56FA5CNFSM4ILBBGJKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBJWIRA.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/kamailio/kamailio/pull/2031?email_source=notifications\u0026email_token=ABO7UZN47H6P2TAKIOY2JITQEG56FA5CNFSM4ILBBGJKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBJWIRA#pullrequestreview-273900612",
"url": "https://github.com/kamailio/kamailio/pull/2031?email_source=notifications\u0026email_token=ABO7UZN47H6P2TAKIOY2JITQEG56FA5CNFSM4ILBBGJKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBJWIRA#pullrequestreview-273900612",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>