@henningw commented on this pull request.
Thanks for the update. I just did a quick review, focus on the usual string
handling and memory allocation topics.
>
pkg_free(ptr);
- ptr = NULL;
+ *held = NULL;
+}
+
+/*
+ * lost_copy_string(str, int*) {
Have you considered using the existing ut.h pkg_str_dup(..) function?
> + /* 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);
Its necessary to make a copy here, can not the host.s be used?
> + 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);
Its necessary to make a copy here, can not the url.s be used?
> +
+ /* 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);
same as above
> 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);
same
> -
- /* 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);
if you need a str anyway, you could use pkg_str_dup
> + 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);
if you need a str anyway, you could use pkg_str_dup
> + 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);
if you need a str anyway, you could use pkg_str_dup
> + 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);
if you need a str anyway, you can use pkg_str_dup
> + 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);
same
> + 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);
this will allocate pkg memory to heldreq
> + 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);
and this again another chunk of pkg mem to heldreq, but only one is freed later
one?
> + }
+ }
+ }
+
+ /* 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);
see above
> +
+ /* 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);
see above
> @@ -0,0 +1,255 @@
+/*
+ * lost module naptr functions
+ * thankfully taken over from the enum module
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.
> +#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"
+
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..).
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2675#pullrequestreview-624954455_______________________________________________
Kamailio (SER) - Development Mailing List
[email protected]
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
