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 > <[email protected] <mailto:[email protected]>> > wrote: > > > On 14 December 2014 at 21:40, Daniel-Constantin Mierla > <[email protected] <mailto:[email protected]>> 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
_______________________________________________ sr-dev mailing list [email protected] http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
