[sr-dev] Registrar - check for local path

[Daniel-Constantin Mierla]

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/635f23b12eff2431ca9a14bb39f4204dc2a7227b
> https://github.com/kamailio/kamailio/commit/4ff2c48d66a3bce2c491b44d0f1b5e939e5508ff
>
> Thanks in advance,
>
> Charles
>
>
> On 15 December 2014 at 12:58, Charles Chance
> <charles.chance at sipcentric.com <mailto:charles.chance at sipcentric.com>>
> wrote:
>
>
>     On 14 December 2014 at 21:40, Daniel-Constantin Mierla
>     <miconda at gmail.com <mailto:miconda at 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. 

-- 
Daniel-Constantin Mierla
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sip-router.org/pipermail/sr-dev/attachments/20150109/b548036c/attachment-0001.html>