Hello,
looks ok now -- I missed the commit in the branch, but seems to include
the changes I pointed in the last review. If yes, then it can be pushed
in master.
Cheers,
Daniel
On 09/01/15 18:54, Charles Chance wrote:
Hello,
Would anyone like to review the final changes, before I merge them
into master?
I have created personal branch cchance/registrar, with the relevant
commits being:
https://github.com/kamailio/kamailio/commit/635f23b12eff2431ca9a14bb39f4204…
https://github.com/kamailio/kamailio/commit/4ff2c48d66a3bce2c491b44d0f1b5e9…
Thanks in advance,
Charles
On 15 December 2014 at 12:58, Charles Chance
<charles.chance(a)sipcentric.com <mailto:charles.chance@sipcentric.com>>
wrote:
On 14 December 2014 at 21:40, Daniel-Constantin Mierla
<miconda(a)gmail.com <mailto: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
www.sipcentric.com <http://www.sipcentric.com/>
Follow us on twitter @sipcentric <http://twitter.com/sipcentric>
Sipcentric Ltd. Company registered in England & Wales no.
7365592. Registered office: Faraday Wharf, Innovation Birmingham
Campus, Holt Street, Birmingham Science Park, Birmingham B7 4BB.