<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hello,<br>
<br>
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.<br>
<br>
Cheers,<br>
Daniel<br>
<br>
<div class="moz-cite-prefix">On 09/01/15 18:54, Charles Chance
wrote:<br>
</div>
<blockquote
cite="mid:CAOvxgzAzOAAO9JxjKe51-NewREMb4CjTPCXFvv0TmJi1+8kLkA@mail.gmail.com"
type="cite">
<div dir="ltr">Hello,
<div><br>
</div>
<div>Would anyone like to review the final changes, before I
merge them into master?</div>
<div><br>
</div>
<div>I have created personal branch cchance/registrar, with the
relevant commits being:</div>
<div><br>
</div>
<div><a moz-do-not-send="true"
href="https://github.com/kamailio/kamailio/commit/635f23b12eff2431ca9a14bb39f4204dc2a7227b">https://github.com/kamailio/kamailio/commit/635f23b12eff2431ca9a14bb39f4204dc2a7227b</a><br>
</div>
<div><a moz-do-not-send="true"
href="https://github.com/kamailio/kamailio/commit/4ff2c48d66a3bce2c491b44d0f1b5e939e5508ff">https://github.com/kamailio/kamailio/commit/4ff2c48d66a3bce2c491b44d0f1b5e939e5508ff</a><br>
</div>
<div><br>
</div>
<div>Thanks in advance,<br>
</div>
<div><br>
</div>
<div>Charles<br>
</div>
<div><br>
</div>
<div><br>
</div>
<div class="gmail_extra">
<div class="gmail_quote">On 15 December 2014 at 12:58, Charles
Chance <span dir="ltr"><<a moz-do-not-send="true"
href="mailto:charles.chance@sipcentric.com"
target="_blank">charles.chance@sipcentric.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">
<div class="gmail_extra"><br>
<div class="gmail_quote"><span class="">On 14 December
2014 at 21:40, Daniel-Constantin Mierla <span
dir="ltr"><<a moz-do-not-send="true"
href="mailto:miconda@gmail.com"
target="_blank">miconda@gmail.com</a>></span>
wrote:
<blockquote class="gmail_quote" style="margin:0 0
0 .8ex;border-left:1px #ccc
solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> Hello,<br>
<br>
looking at the patch, I see that the block for
parsing first path uri:<br>
<br>
<pre>+ 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</pre>
<br>
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.<br>
<br>
</div>
</blockquote>
<div><br>
</div>
</span>
<div>Yes, spotted that one already and it has been
moved inside the parameter check.</div>
<span class="">
<div><br>
</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0
0 .8ex;border-left:1px #ccc
solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> 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<br>
<br>
str path_str;<br>
<br>
...<br>
path_str = ptr->path;<br>
// and use path_str instead of ptr->path
from here on<br>
...<br>
<br>
</div>
</blockquote>
<div><br>
</div>
</span>
<div>Sorry, oversight on my part, will make a copy
and work with that instead.</div>
<span class="">
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0
0 .8ex;border-left:1px #ccc
solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> <br>
The new parameter has to be documented in the
readme as well.<br>
<br>
</div>
</blockquote>
<div> </div>
</span>
<div>Of course :)</div>
<div><br>
</div>
<div>Thanks for your time,</div>
<div><br>
</div>
<div>Charles</div>
<div><br>
</div>
</div>
</div>
</div>
</blockquote>
</div>
<br>
<br clear="all">
<div><br>
</div>
</div>
</div>
<br>
<font face="Helvetica, Arial, sans-serif"><font size="2"><span
style="font-size:10pt"><a moz-do-not-send="true"
href="http://www.sipcentric.com/"
title="blocked::http://www.sipcentric.com/"
target="_blank">www.sipcentric.com</a><br>
<br>
Follow us on twitter <a moz-do-not-send="true"
href="http://twitter.com/sipcentric"
title="blocked::http://twitter.com/sipcentric"
target="_blank">@sipcentric</a><br>
<br>
<font color="gray">Sipcentric Ltd. Company registered in
England & Wales no. 7365592.</font> <font color="gray">Registered
office: Faraday Wharf, Innovation Birmingham Campus, Holt
Street, Birmingham Science Park, Birmingham B7 4BB.</font></span></font></font>
</blockquote>
<br>
<pre class="moz-signature" cols="72">--
Daniel-Constantin Mierla
<a class="moz-txt-link-freetext" href="http://twitter.com/#!/miconda">http://twitter.com/#!/miconda</a> - <a class="moz-txt-link-freetext" href="http://www.linkedin.com/in/miconda">http://www.linkedin.com/in/miconda</a></pre>
</body>
</html>