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.

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
...


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

Cheers,
Daniel

On 11/12/14 14:10, Charles Chance wrote:
Ok, no worries - thanks, Daniel.

Quick note - I spotted that parse_uri() is not necessary unless path_check_local is enabled, so that has been moved now.

Otherwise, it is working nicely in one of our installations.

Best regards,

Charles


On 11 December 2014 at 12:52, Daniel-Constantin Mierla <miconda@gmail.com> wrote:
I didn't get the chance to look at it yet -- it is on my radar.

Cheers,
Daniel


On 11/12/14 11:24, Charles Chance wrote:

Any further thoughts on this, please?

Cheers,

Charles

Hi Daniel,

Please see attached (I have tested already).

Should it be added, or left up to the user to perform via config? Either way, at least the patch is available here as an option, in case the question is asked again.

It would be good to hear others' opinions, too.

Best regards,

Charles


On 8 December 2014 at 20:10, Charles Chance <charles.chance@sipcentric.com> wrote:

Hi Daniel,

You are right, it is not trivial to perform via config.

The patch was made quickly to illustrate a point, but I have reworked it now to include a parameter for enabling the check, as well as accounting for more than one Path header.

If you think it is worthwhile, I will post the full patch for review tomorrow.

Best regards,

Charles

On 8 Dec 2014 16:28, "Daniel-Constantin Mierla" <miconda@gmail.com> wrote:

On 08/12/14 16:40, Charles Chance wrote:


On 8 December 2014 at 15:09, Olle E. Johansson <oej@edvina.net> wrote:

On 08 Dec 2014, at 16:00, Charles Chance <charles.chance@sipcentric.com> wrote:

Hi Olle,

msg_apply_changes() is for getting the Path saved the first place if adding/saving on the same instance.

My patch is for later on, to avoid looping if lookup is performed on the same instance that received the register.

Scenario is 2 x registrar/location servers, both sharing common DB - no separate edge proxies, but each adds itself as Path before saving (which is where msg_apply_changes() comes in).
Can't you sort that out in the routing script? I don't see why we need to add this in the code... 

If the topmost, leftmost routing header in the outbound INVITE points to me, remove it and move on.
You have the branch route for that kind of manipulation.

What am I missing?

If I got it right upon quick read, this case is not trivial to handle via config file -- i.e., it is about saving registration with local address as a Path, the registration can be read by same proxy or another one (the other will have to send the register to this instance, this one will need to ignore the path).

After lookup("location"), the first Path appears as outbound proxy address ($du / dst_uri), but it is also added in the lumps to be a Route header for outgoing INVITE. If there are more than on Path header, things can get quite complex to handle from config and might be easier to simplify by adding a module parameter to enable/disable the proposed patch.

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

_______________________________________________
sr-dev mailing list
sr-dev@lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev



www.sipcentric.com

Follow us on twitter @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. 

_______________________________________________
sr-dev mailing list
sr-dev@lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev


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

_______________________________________________
sr-dev mailing list
sr-dev@lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev



www.sipcentric.com

Follow us on twitter @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