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

In src/modules/lost/utilities.c:

>  
 	pkg_free(ptr);
-	ptr = NULL;
+	*held = NULL;
+}
+
+/*
+ * lost_copy_string(str, int*) {

Have you considered using the existing ut.h pkg_str_dup(..) function?

In src/modules/lost/functions.c:

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

In src/modules/lost/functions.c:

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

In src/modules/lost/functions.c:

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

In src/modules/lost/functions.c:

>  		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

In src/modules/lost/functions.c:

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

In src/modules/lost/functions.c:

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

In src/modules/lost/functions.c:

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

In src/modules/lost/functions.c:

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

In src/modules/lost/functions.c:

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

In src/modules/lost/functions.c:

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

In src/modules/lost/functions.c:

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

In src/modules/lost/functions.c:

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

In src/modules/lost/functions.c:

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

In src/modules/lost/naptr.c:

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

In src/modules/lost/response.h:

> +#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, view it on GitHub, or unsubscribe.