On 14 December 2014 at 21:40, Daniel-Constantin Mierla <miconda@gmail.com> wrote:

Hello,

looking at the patch, I see that the block for parsing first path uri:

+			if (parse_uri(path_dst.s, path_dst.len, &path_uri) < 0){
+				LM_ERR("failed to parse the Path URI\n");
+				ret = -3;
+				goto done;
+			}


Is done outside of parameter check:


 +			if (path_check_local > 0

But seems to be used only inside it (when the parameter is set >0). It should be moved inside that IF, to avoid parsing the path uri when the parameter is not set, because that happens for each lookup.

Yes, spotted that one already and it has been moved inside the parameter check.

 

Another thing that has to be double-checked: you change the value of ptr->path.s if there is a match of local URI for first path. That can mess the structure from usrloc, if it needs to do a free later or is a pointer inside shared memory that is going to be used later -- practically the pointer is no longer at the beginning of allocated memory. I didn't have time to look deeper in usrloc and all its db modes (iirc, for db-only, the location record is temporarily built in memory and freed). The best and safest for the future is to make a copy of the path str and work with it inside the function where you need to change it

str path_str;

...
path_str = ptr->path;
// and use path_str instead of ptr->path from here on
...

Sorry, oversight on my part, will make a copy and work with that instead.

 

The new parameter has to be documented in the readme as well.

 

Of course :)

Thanks for your time,

Charles