henningw commented on this pull request.

Thank you Wolfgang, great contribution.

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.

+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)

Maybe just return 0?

• 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

The formatting is a bit broken here in this if case.

• • 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)

Remove the history comment from the source file, also in the header file below.

+ * 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

Version comment (see above) and the doxygen is also wrong (the one in the .c is correct).

• • 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)

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.

• 			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);

This can fail

+ 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)

Same comment as lost_new_loc function about memory allocation and checks.

• 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)

see above

• 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)

as above

• 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)

see above - and are the XML operations working always as well?

• 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)

see above

+ 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:

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.

+/*

+ * 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");

you meant probably not "writable" - copy and paste error also below

@@ -0,0 +1,238 @@

+LOST Module

Remove the README - it is autogenerated after you commited the XML doc files.