Please don't create that much duplicated code. Make the old API function call the new as a wrapper with a null argument for content type header. That way we keep backwards compatibility and can add new functions.

Please also add new tests to the API test module.

Please don't create that much duplicated code. Make the old API function call the new as a wrapper with a null argument for content type header. That way we keep backwards compatibility and can add new functions.

Please also add new tests to the API test module.

Hi Olle, i think you are referring to the http_client part of this pull request? Which one in particular, not sure if you know that you can also add comments to individual functions in the review mode.

@wkampich pushed 3 commits.

2f48d0cdc84eaa583a2f2b6837e6251e62abc901 lost: DOM level count fix 5f50cc0ec7017a325d25249e6e66e6a2feb4dd08 http_client: duplicated code removed 25378c0388bb4365e8d4f1a4e874381fe339cd3b lost: README update

All good. Thanks for fixing!

Moving code to the core from existing module must ensure it can be relocated under BSD license.

Regarding the cloning of some str values, they seem to be required due to used functions that require zero-terminated strings, like ipstr that is passed to lost_get_nameinfo(), which is using libc inet_pton().

@wkampich commented on this pull request.

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

yes true `pkg_str_dup(..)` would be an option; basically I only use the function when a pointer to a string is necessary and requires zero termination

@wkampich commented on this pull request.

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

curl requires a zero terminated string as url, which is not added by the http_client API query function `http_client_query_c`

@wkampich commented on this pull request.

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

curl requires a zero terminated string as url, which is not added by the http_client API query function `http_client_query_c`

@wkampich commented on this pull request.

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

yes, I should definitely correct that - thanks for pointing it out

@wkampich commented on this pull request.

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

thanks, have incorporated it

@wkampich commented on this pull request.

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

thanks, have incorporated it

@wkampich commented on this pull request.

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

done

@sergey-safarov commented on this pull request.

• 	    </itemizedlist>
  

+ <para> + The return value is 200 on success, 400 if an internal error occured, or 500 if an + error code is returned in the HELD response. + </para> + <para> + This function can be used from REQUEST_ROUTE, + ONREPLY_ROUTE, FAILURE_ROUTE, and BRANCH_ROUTE. + </para> + <example> + <title><function>lost_held_dereference()</function> usage</title> + <programlisting format="linespecific"> +... +# HELD location dereference +if ($hdr(Geolocation)=~"^&lt;http.*$") { + $var(url) = $(hdr(Geolocation){s.select,0,;});

I can suggest use ``` $var(url) = @hf_value.geolocation[1].uri; ```

@miconda commented on this pull request.

• 	    </itemizedlist>
  

+ <para> + The return value is 200 on success, 400 if an internal error occured, or 500 if an + error code is returned in the HELD response. + </para> + <para> + This function can be used from REQUEST_ROUTE, + ONREPLY_ROUTE, FAILURE_ROUTE, and BRANCH_ROUTE. + </para> + <example> + <title><function>lost_held_dereference()</function> usage</title> + <programlisting format="linespecific"> +... +# HELD location dereference +if ($hdr(Geolocation)=~"^&lt;http.*$") { + $var(url) = $(hdr(Geolocation){s.select,0,;});

Note that `@...` were SER specific variables, still available, but not with an active maintainer. I would rather recommend using `$...` instead of `@...`, so better examples with $hdr(...) when gives what's needed.

Use case when the call contains two `Geolocation` headers.

Example ``` Geolocation: http://example.org/held-link Geolocation: cid:7607331117@10.3.90.20 ```

Technically such a call contains Geolocacation by_value and does not require deference first Geolocation header by_reference.

From my point of view.

When used `emergencyRouting` routing need to prefer Geolocation by_value. This alow delivered call with minimal delays but maybe with low accuracy. When used `emergencyDispatch` routing needs to prefer Geolocation by_reference. This alow send HELD request to a mobile carrier and get latest mobile device location.

Technically on Kamailio maybe used `emergencyRouting` and on `PSAP` used `emergencyDispatch`.

So as idea, in lost request check the number of present Geolocation headers, if present more than one header: 1. and used `emergencyRouting` then try search `by_value` Geolocation; 2. and used `emergencyDispatch` then try to search and dereference `by_reference` Geolocation.

@wkampich commented on this pull request.

• 	    </itemizedlist>
  

+ <para> + The return value is 200 on success, 400 if an internal error occured, or 500 if an + error code is returned in the HELD response. + </para> + <para> + This function can be used from REQUEST_ROUTE, + ONREPLY_ROUTE, FAILURE_ROUTE, and BRANCH_ROUTE. + </para> + <example> + <title><function>lost_held_dereference()</function> usage</title> + <programlisting format="linespecific"> +... +# HELD location dereference +if ($hdr(Geolocation)=~"^&lt;http.*$") { + $var(url) = $(hdr(Geolocation){s.select,0,;});

@sergey-safarov: you may use the following to dereference multiple header fields: ``` ## response time in ms: 20000, or 'emergencyRouting' or 'emergencyDispatch' #!substdef "!MY_LOC_TIME!emergencyDispatch!g" ## location type: 'any', 'civic', 'geodetic', or 'civic geodetic' #!substdef "!MY_LOC_TYPE!any!g" ``` ... ``` if(is_present_hf("Geolocation")) { $var(count) = 0; while($var(count) < $hdrc(Geolocation)) { $var(geo) = $(hdr(Geolocation)[$var(count)]); if ($var(geo)=~"^<http.*$") { $var(url) = $(var(geo){s.select,0,;}); xlog("L_INFO", "### HELD dereferencing location at: $(var(url){s.unbracket})\n"); $var(res) = lost_held_dereference("$(var(url){s.unbracket})", "MY_LOC_TIME", "MY_LOC_TYPE", "$var(pidf)", "$var(err)"); if ($var(res) == 200) { xlog("L_INFO", "### HELD dereference request returned loc:\n$var(pidf)\n"); } else { xlog("L_INFO", "### held request failed with $var(err)\n"); } } } } ```

tested config

listen=udp:127.0.0.1:5060
loadmodule "xlog.so"
loadmodule "pv.so"
loadmodule "http_client.so"
loadmodule "lost.so"
loadmodule "textopsx.so"

modparam("lost", "exact_type", 1)
modparam("lost", "post_request", 1)
modparam("http_client", "httpcon", "lostsrv=>http://api.example.com/api/lost");


request_route {
  $var(res) = lost_query("lostsrv", "$var(uri)", "$var(name)", "$var(err)");
  xlog("L_INFO", "LOST findService: Result code $var(res)\nUri: $var(uri)\nName: $var(name)\n");
}

Test with one `Geolocation` header. LOST request send with HTTP headers like

POST /api/lost HTTP/1.1
Host: api.example.com
User-Agent: kamailio (5.5.0-pre0 (x86_64/linux))
Accept: */*
Content-Type: application/lost+xml;charset=utf-8
Content-Length: 526

Here may be updated

Accept: application/lost+xml

@sergey-safarov: that looks like another http_client API fix as the API call does not support to set a specific header.

Also in Kamailio console, I can see this error message when `Geolocation` passed as a value

 1(82) ERROR: <core> [core/parser/parse_body.c:510]: part_multipart_headers_cmp(): error code: "-9" error text: "We reached the end of the buffer".

Looks as here here "ERROR" level misslead. My test SIPp script attached.

[test_call.xml.gz](https://github.com/kamailio/kamailio/files/6255191/test_call.xml.gz)

The script may be started using the command

sipp -m 1 -sf test_call.xml -p 2345 127.0.0.1

@sergey-safarov: I see the same ERROR, but I suspect it is more of a WARNING

Regarding:

``` 1(82) ERROR: <core> [core/parser/parse_body.c:510]: part_multipart_headers_cmp(): error code: "-9" error text: "We reached the end of the buffer". ```

Maybe there are white spaces at the end of body that fit inside the Content-Length in the sipp scenario (try to print the $mb enclosed in some delimiters like [ ] to see it completely. If proves to be a problem, then an issue can be created for this case with a minimal config to reproduce and the sipp scenario for it.

Then related this PR, while not familiar with all its aspects, it seems some of the questions are actually about further improvements (e.g., handling two Geolocation headers). They do not have to be implemented as part of this PR, can be done separately. If no problems need to be solved on this PR, then it should be merged in order to get more chances to be tested before branching 5.5.

If it is a valid multipart body, with a header that may not exist, then the log message should be DEBUG. You can make the patch if that's the case.

If there are no further comments, I will merge the pull request today (late afternoon). Just a note about "handling more than one `Geolocation` header": The LoST module creates a list of `Geolocation` headers (be it multiple values in a header or single headers) and can decide which header type to use to retrieve the location for a LoST request based on certain module parameters. This is done via module functions and does not require any additional steps in `kamailio.cfg`. The new module function `lost_held_dereference` can be used when Kamailio is configured as PSAP endpoint, but was intended more for testing purposes. To get the correct `Geolocation` header value (URL) for this function some additional steps in `kamailio.cfg` are required.

@wkampich Thanks for the update. I think the comments regarding the missing original copyright notice in the natptr.[c,h] files and also the response.h naming are not resolved yet. The copyright notice should be added, I can also do it later if you prefer this. Regarding the naming comment, I just wanted to let you know that there might be a risk for overlapping data structures, as the s_info_t, s_data_t, cat_t etc.. are pretty generic and might be also defined in another library or module. The typedefs like LIST, INFO, DATA etc.. are in C also globally visible. For this reasons modules usually use the module prefix as part of the typedef, e.g.:

* typedef enum rms_action_type * typedef struct _sr_lua_env * typedef struct ipsec_pcscf_api

Several of this typedefs seems to be also not used in your code. But if you think its no big deal for your module, fine.

Hello, @wkampich Let me review the change about ERROR waring. I want to try to understand in detail why the error generated. Maybe I can suggest another approach. Please give today and tomorrow.

@sergey-safarov: yes, ok for me - just a hint: the problem is not in the multipart header parsing but in the function that searches for specific header values in several body parts - parsing (successfully) happens beforehand. Just add the following to your sipp example (xml) and you get the error (or debug message) twice:

``` ... a=maxptime:20

--35129 Content-Type: plain/text

HELLO

--35129 Content-Type: application/pidf+xml Content-Id: 7607331117@10.3.90.20 ... ```

@wkampich, @sergey-safarov: the error message from part_multipart_headers_cmp() does not seem related to the code added in this PR, so, even if it is an issue, it is not required to be fixed by this PR. If proves to be a bug, an issue can be opened, or even better, a new PR can be made to fix it.

yes, have checked "ERROR" generation. This happens because `part_multipart_headers_cmp()` try search content by "Content-Id". The first content element (SDP in the example) does not have "Content-Id". Function reaches the end of the content element and generates the error.

Then `part_multipart_headers_cmp()` try search "Content-Id" in second element and fount it and then returns result.

Looks as current `part_multipart_headers_cmp()` implementation handles case when content element do not has "Content-Id" header as error. Looks as provided fix correct for this case.

Merge it with the patch, it is only changing the printing of the log message in that case and should be legit that the header is missing, not being mandatory.