<p></p>
<p><b>@henningw</b> commented on this pull request.</p>
<p>Thanks for the update. I just did a quick review, focus on the usual string handling and memory allocation topics.</p><hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2675#discussion_r604644385">src/modules/lost/utilities.c</a>:</p>
<pre style='color:#555'>>
pkg_free(ptr);
- ptr = NULL;
+ *held = NULL;
+}
+
+/*
+ * lost_copy_string(str, int*) {
</pre>
<p>Have you considered using the existing ut.h pkg_str_dup(..) function?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2675#discussion_r604644709">src/modules/lost/functions.c</a>:</p>
<pre style='color:#555'>> + /* send via connection */
+ curl = httpapi.http_connect(_m, &con, NULL, &res, mtheld, &que);
+ } else {
+ /* we have no connection ... do a NAPTR lookup */
+ if(lost_parse_host(did.s, &host, &flag) > 0) {
+
+ LM_DBG("no conn. trying NATPR lookup [%.*s]\n", host.len, host.s);
+
+ /* remove '[' and ']' from string (IPv6) */
+ if(flag == AF_INET6) {
+ host.s++;
+ host.len = host.len - 2;
+ }
+ /* is it a name or ip ... check nameinfo (reverse lookup) */
+ len = 0;
+ ipstr = lost_copy_string(host, &len);
</pre>
<p>Its necessary to make a copy here, can not the host.s be used?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2675#discussion_r604644784">src/modules/lost/functions.c</a>:</p>
<pre style='color:#555'>> + lost_free_string(&idhdr);
+ goto err;
+ }
+ } else {
+ LM_ERR("failed to get location service for [%.*s]\n", did.len,
+ did.s);
+ lost_free_string(&que); /* clean up */
+ lost_free_string(&idhdr);
+ goto err;
+ }
+
+ LM_DBG("NATPR lookup returned [%.*s]\n", url.len, url.s);
+
+ /* curl doesn't like str */
+ len = 0;
+ lisurl = lost_copy_string(url, &len);
</pre>
<p>Its necessary to make a copy here, can not the url.s be used?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2675#discussion_r604645979">src/modules/lost/functions.c</a>:</p>
<pre style='color:#555'>> +
+ /* clean up */
+ if(rtype != NULL) {
+ pkg_free(rtype);
+ }
+
+ if(heldreq != NULL && len == 0) {
+ LM_ERR("could not create POST request\n");
+ goto err;
+ }
+
+ LM_DBG("POST request: [%.*s]\n", len, heldreq);
+
+ /* curl doesn't like str */
+ len = 0;
+ lisurl = lost_copy_string(url, &len);
</pre>
<p>same as above</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2675#discussion_r604646085">src/modules/lost/functions.c</a>:</p>
<pre style='color:#555'>> LM_ERR("lost request failed\n");
goto err;
}
LM_DBG("findService request: [%.*s]\n", req.len, req.s);
/* send findService request to mapping server - HTTP POST */
- curlres = httpapi.http_connect(_m, &con, NULL, &ret, mtlost, &req);
+ if(naptr) {
+ /* copy url */
+ len = 0;
+ urlrep = lost_copy_string(url, &len);
</pre>
<p>same</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2675#discussion_r604646353">src/modules/lost/functions.c</a>:</p>
<pre style='color:#555'>> -
- /* get the error patterm */
- err.s = lost_get_childname(root, errors_element, &err.len);
- LM_DBG("findService error response: [%.*s]\n", err.len, err.s);
- if(err.len == 0) {
- LM_ERR("error pattern element not found: [%.*s]\n", ret.len, ret.s);
- /* free memory */
- lost_free_string(&ret);
- goto err;
+ switch(fsrdata->category) {
+ case RESPONSE:
+ if(fsrdata->uri != NULL) {
+ /* get the first uri element */
+ if((tmp.s = fsrdata->uri->value) != NULL) {
+ tmp.len = strlen(fsrdata->uri->value);
+ uri.s = lost_copy_string(tmp, &uri.len);
</pre>
<p>if you need a str anyway, you could use pkg_str_dup</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2675#discussion_r604646416">src/modules/lost/functions.c</a>:</p>
<pre style='color:#555'>> + case RESPONSE:
+ if(fsrdata->uri != NULL) {
+ /* get the first uri element */
+ if((tmp.s = fsrdata->uri->value) != NULL) {
+ tmp.len = strlen(fsrdata->uri->value);
+ uri.s = lost_copy_string(tmp, &uri.len);
+ }
+ } else {
+ LM_ERR("uri not found: [%.*s]\n", ret.len, ret.s);
+ goto err;
+ }
+ if(fsrdata->mapping != NULL) {
+ /* get the displayName element */
+ if((tmp.s = fsrdata->mapping->name->text) != NULL) {
+ tmp.len = strlen(fsrdata->mapping->name->text);
+ name.s = lost_copy_string(tmp, &name.len);
</pre>
<p>if you need a str anyway, you could use pkg_str_dup</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2675#discussion_r604646494">src/modules/lost/functions.c</a>:</p>
<pre style='color:#555'>> + tmp.len = strlen(fsrdata->mapping->name->text);
+ name.s = lost_copy_string(tmp, &name.len);
+ }
+ } else {
+ LM_ERR("name not found: [%.*s]\n", ret.len, ret.s);
+ goto err;
+ }
+ /* we are done */
+ redirect = 0;
+ break;
+ case ERROR:
+ /* get the errors element */
+ if(fsrdata->errors != NULL) {
+ if((tmp.s = fsrdata->errors->issue->type) != NULL) {
+ tmp.len = strlen(fsrdata->errors->issue->type);
+ err.s = lost_copy_string(tmp, &err.len);
</pre>
<p>if you need a str anyway, you could use pkg_str_dup</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2675#discussion_r604646614">src/modules/lost/functions.c</a>:</p>
<pre style='color:#555'>> + goto err;
+ }
+ /* clean up */
+ tmp.s = NULL;
+ tmp.len = 0;
+ /* check loop */
+ if(oldurl.s != NULL && oldurl.len > 0) {
+ if(str_strcasecmp(&url, &oldurl) == 0) {
+ LM_ERR("loop detected: "
+ "[%.*s]<-->[%.*s]\n",
+ oldurl.len, oldurl.s, url.len, url.s);
+ goto err;
+ }
+ }
+ /* remember the redirect target */
+ oldurl.s = lost_copy_string(url, &oldurl.len);
</pre>
<p>if you need a str anyway, you can use pkg_str_dup</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2675#discussion_r604646979">src/modules/lost/functions.c</a>:</p>
<pre style='color:#555'>> + if(oldurl.s != NULL && oldurl.len > 0) {
+ if(str_strcasecmp(&url, &oldurl) == 0) {
+ LM_ERR("loop detected: "
+ "[%.*s]<-->[%.*s]\n",
+ oldurl.len, oldurl.s, url.len, url.s);
+ goto err;
+ }
+ }
+ /* remember the redirect target */
+ oldurl.s = lost_copy_string(url, &oldurl.len);
+ /* clean up */
+ lost_free_findServiceResponse(&fsrdata);
+ lost_free_string(&ret);
+ /* copy url */
+ len = 0;
+ urlrep = lost_copy_string(url, &len);
</pre>
<p>same</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2675#discussion_r604649091">src/modules/lost/functions.c</a>:</p>
<pre style='color:#555'>> + if(rtp.len == 0) {
+ LM_WARN("no response type found\n");
+ rtype = NULL;
+ } else {
+ len = 0;
+ /* response type string sanity check */
+ rtype = lost_held_type(rtp.s, &exact, &len);
+ if(len == 0) {
+ LM_WARN("cannot normalize [%.*s]\n", rtp.len, rtp.s);
+ rtype = NULL;
+ }
+ }
+ }
+
+ /* default responseTime: emergencyRouting */
+ heldreq = lost_held_post_request(&len, 0, rtype);
</pre>
<p>this will allocate pkg memory to heldreq</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2675#discussion_r604649280">src/modules/lost/functions.c</a>:</p>
<pre style='color:#555'>> + len = 0;
+ /* response type string sanity check */
+ rtype = lost_held_type(rtp.s, &exact, &len);
+ if(len == 0) {
+ LM_WARN("cannot normalize [%.*s]\n", rtp.len, rtp.s);
+ rtype = NULL;
+ }
+ }
+ }
+
+ /* default responseTime: emergencyRouting */
+ heldreq = lost_held_post_request(&len, 0, rtype);
+
+ /* responseTime: milliseconds */
+ if((ltime > 0) && (strlen(ptr) == 0)) {
+ heldreq = lost_held_post_request(&len, ltime, rtype);
</pre>
<p>and this again another chunk of pkg mem to heldreq, but only one is freed later one?</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2675#discussion_r604650181">src/modules/lost/functions.c</a>:</p>
<pre style='color:#555'>> + }
+ }
+ }
+
+ /* default responseTime: emergencyRouting */
+ heldreq = lost_held_post_request(&len, 0, rtype);
+
+ /* responseTime: milliseconds */
+ if((ltime > 0) && (strlen(ptr) == 0)) {
+ heldreq = lost_held_post_request(&len, ltime, rtype);
+ }
+
+ /* responseTime: emergencyRouting|emergencyDispatch */
+ if((ltime == 0) && (strlen(ptr) > 0)) {
+ if(strncasecmp(ptr, HELD_ED, strlen(HELD_ED)) == 0) {
+ heldreq = lost_held_post_request(&len, -1, rtype);
</pre>
<p>see above</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2675#discussion_r604650276">src/modules/lost/functions.c</a>:</p>
<pre style='color:#555'>> +
+ /* default responseTime: emergencyRouting */
+ heldreq = lost_held_post_request(&len, 0, rtype);
+
+ /* responseTime: milliseconds */
+ if((ltime > 0) && (strlen(ptr) == 0)) {
+ heldreq = lost_held_post_request(&len, ltime, rtype);
+ }
+
+ /* responseTime: emergencyRouting|emergencyDispatch */
+ if((ltime == 0) && (strlen(ptr) > 0)) {
+ if(strncasecmp(ptr, HELD_ED, strlen(HELD_ED)) == 0) {
+ heldreq = lost_held_post_request(&len, -1, rtype);
+ }
+ if(strncasecmp(ptr, HELD_ER, strlen(HELD_ER)) == 0) {
+ heldreq = lost_held_post_request(&len, 0, rtype);
</pre>
<p>see above</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2675#discussion_r604654430">src/modules/lost/naptr.c</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,255 @@
+/*
+ * lost module naptr functions
+ * thankfully taken over from the enum module
</pre>
<p>There are several functions (parse_naptr_regexp, naptr_greater, naptr_sort..) copied from the enum module. We could consider moving them to the core. If we want to keep them, the copyright from the author should be present here as well.</p>
<hr>
<p>In <a href="https://github.com/kamailio/kamailio/pull/2675#discussion_r604657968">src/modules/lost/response.h</a>:</p>
<pre style='color:#555'>> +#define PATH_NODE (const char *)"via"
+#define PATH_NODE_VIA (const char *)"via"
+#define MAPP_NODE (const char *)"mapping"
+#define MAPP_NODE_URI (const char *)"uri"
+#define MAPP_PROP_EXP (const char *)"expires"
+#define MAPP_PROP_LUP (const char *)"lastUpdated"
+#define MAPP_PROP_SRC (const char *)"source"
+#define MAPP_PROP_SID (const char *)"sourceId"
+
+#define RED_NODE (const char *)"redirect"
+#define RED_PROP_TAR (const char *)"target"
+#define RED_PROP_SRC (const char *)"source"
+#define RED_PROP_MSG (const char *)"message"
+
+#define ERRORS_NODE (const char *)"errors"
+
</pre>
<p>The section below defines several data structures. In order to prevent overlapping with existing data structures in modules and other libraries you probably want to add a prefix (e.g. lost_list_t, lost_data_t etc..).</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/2675#pullrequestreview-624954455">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABO7UZK3IWNNESUP3XRYRULTGLF3VANCNFSM4ZGSJGLQ">unsubscribe</a>.<img src="https://github.com/notifications/beacon/ABO7UZNIRKPKEJ3ZD43H2ALTGLF3VA5CNFSM4ZGSJGL2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOEVAAYVY.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/2675#pullrequestreview-624954455",
"url": "https://github.com/kamailio/kamailio/pull/2675#pullrequestreview-624954455",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>