[sr-dev] Registrar - check for local path
Daniel-Constantin Mierla
miconda at gmail.com
Sun Dec 14 22:40:52 CET 2014
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 at gmail.com <mailto:miconda at 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 at sipcentric.com
>> <mailto:charles.chance at 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 at gmail.com <mailto:miconda at gmail.com>> wrote:
>>
>>
>> On 08/12/14 16:40, Charles Chance wrote:
>>>
>>>
>>> On 8 December 2014 at 15:09, Olle E. Johansson
>>> <oej at edvina.net <mailto:oej at edvina.net>> wrote:
>>>
>>>
>>> On 08 Dec 2014, at 16:00, Charles Chance
>>> <charles.chance at sipcentric.com
>>> <mailto:charles.chance at 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://twitter.com/#%21/miconda> - http://www.linkedin.com/in/miconda
>>
>>
>> _______________________________________________
>> sr-dev mailing list
>> sr-dev at lists.sip-router.org
>> <mailto:sr-dev at lists.sip-router.org>
>> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
>>
>>
>>
>> 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.
>>
>>
>> _______________________________________________
>> sr-dev mailing list
>> sr-dev at lists.sip-router.org <mailto:sr-dev at lists.sip-router.org>
>> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
>
> --
> Daniel-Constantin Mierla
> http://twitter.com/#!/miconda <http://twitter.com/#%21/miconda> - http://www.linkedin.com/in/miconda
>
>
> _______________________________________________
> sr-dev mailing list
> sr-dev at lists.sip-router.org <mailto:sr-dev at lists.sip-router.org>
> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
>
>
>
> 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/20141214/09b767fe/attachment-0001.html>
More information about the sr-dev
mailing list