8 Dec
2014
8 Dec
'14
3:42 p.m.
Hi All,
Can anyone see a problem with doing (something like) the following, to
handle the situation like
http://lists.sip-router.org/pipermail/sr-users/2014-October/085251.html?
Admittedly, it is probably a less common use case, but it has been raised
several times on the list so I believe it is a genuine one.
diff --git a/modules/registrar/lookup.c b/modules/registrar/lookup.c
index 794d968..66730b4 100644
--- a/modules/registrar/lookup.c
+++ b/modules/registrar/lookup.c
@@ -41,6 +41,7 @@
#include "../../action.h"
#include "../../mod_fix.h"
#include "../../parser/parse_rr.h"
+#include "../../forward.h"
#include "../usrloc/usrloc.h"
#include "common.h"
#include "regtime.h"
@@ -121,6 +122,7 @@ int lookup_helper(struct sip_msg* _m, udomain_t* _d,
str* _uri, int _mode)
sr_xavp_t *list=NULL;
str xname = {"ruid", 4};
sr_xval_t xval;
+ sip_uri_t path_uri;
ret = -1;
@@ -265,6 +267,14 @@ int lookup_helper(struct sip_msg* _m, udomain_t* _d,
str* _uri, int _mode)
ret = -3;
goto done;
}
+ 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;
+ }
+ }
+ /* Only use path-uri if non-local */
+ if (path_uri.host.s && !check_self(&(path_uri.host), 0, 0))
{
if (set_path_vector(_m, &ptr->path) < 0) {
LM_ERR("failed to set path vector\n");
ret = -3;
The above needs to be repeated in lookup_branches function of same file,
but I wanted to check others' opinions first.
Best regards,
Charles
--
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.
8 Dec
8 Dec
3:46 p.m.
On 08 Dec 2014, at 15:42, Charles Chance <charles.chance(a)sipcentric.com> wrote:
Hi All,
Can anyone see a problem with doing (something like) the following, to handle the
situation like http://lists.sip-router.org/pipermail/sr-users/2014-October/085251.html?
Admittedly, it is probably a less common use case, but it has been raised several times
on the list so I believe it is a genuine one.
Did you try Daniel's advice?
"See msg_apply_changes() from textopsx to get the path header visible."
Did that not work?
/O
diff --git a/modules/registrar/lookup.c b/modules/registrar/lookup.c
index 794d968..66730b4 100644
--- a/modules/registrar/lookup.c
+++ b/modules/registrar/lookup.c
@@ -41,6 +41,7 @@
#include "../../action.h"
#include "../../mod_fix.h"
#include "../../parser/parse_rr.h"
+#include "../../forward.h"
#include "../usrloc/usrloc.h"
#include "common.h"
#include "regtime.h"
@@ -121,6 +122,7 @@ int lookup_helper(struct sip_msg* _m, udomain_t* _d, str* _uri, int
_mode)
sr_xavp_t *list=NULL;
str xname = {"ruid", 4};
sr_xval_t xval;
+ sip_uri_t path_uri;
ret = -1;
@@ -265,6 +267,14 @@ int lookup_helper(struct sip_msg* _m, udomain_t* _d, str* _uri, int
_mode)
ret = -3;
goto done;
}
+ 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;
+ }
+ }
+ /* Only use path-uri if non-local */
+ if (path_uri.host.s && !check_self(&(path_uri.host), 0, 0))
{
if (set_path_vector(_m, &ptr->path) < 0) {
LM_ERR("failed to set path vector\n");
ret = -3;
The above needs to be repeated in lookup_branches function of same file, but I wanted to
check others' opinions first.
Best regards,
Charles
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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
4 p.m.
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).
Cheers,
Charles
On 8 December 2014 at 14:46, Olle E. Johansson <oej(a)edvina.net> wrote:
On 08 Dec 2014, at 15:42, Charles Chance <charles.chance(a)sipcentric.com>
wrote:
Hi All,
Can anyone see a problem with doing (something like) the following, to
handle the situation like
http://lists.sip-router.org/pipermail/sr-users/2014-October/085251.html?
Admittedly, it is probably a less common use case, but it has been raised
several times on the list so I believe it is a genuine one.
Did you try Daniel's advice?
"See msg_apply_changes() from textopsx to get the path header visible."
Did that not work?
/O
diff --git a/modules/registrar/lookup.c b/modules/registrar/lookup.c
index 794d968..66730b4 100644
--- a/modules/registrar/lookup.c
+++ b/modules/registrar/lookup.c
@@ -41,6 +41,7 @@
#include "../../action.h"
#include "../../mod_fix.h"
#include "../../parser/parse_rr.h"
+#include "../../forward.h"
#include "../usrloc/usrloc.h"
#include "common.h"
#include "regtime.h"
@@ -121,6 +122,7 @@ int lookup_helper(struct sip_msg* _m, udomain_t* _d,
str* _uri, int _mode)
sr_xavp_t *list=NULL;
str xname = {"ruid", 4};
sr_xval_t xval;
+ sip_uri_t path_uri;
ret = -1;
@@ -265,6 +267,14 @@ int lookup_helper(struct sip_msg* _m, udomain_t* _d,
str* _uri, int _mode)
ret = -3;
goto done;
}
+ 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;
+ }
+ }
+ /* Only use path-uri if non-local */
+ if (path_uri.host.s && !check_self(&(path_uri.host), 0,
0)) {
if (set_path_vector(_m, &ptr->path) < 0) {
LM_ERR("failed to set path vector\n");
ret = -3;
The above needs to be repeated in lookup_branches function of same file,
but I wanted to check others' opinions first.
Best regards,
Charles
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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
_______________________________________________
sr-dev mailing list
sr-dev(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
--
*Charles Chance*
Managing Director
t. 0121 285 4400 m. 07932 063 891
--
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.
4:09 p.m.
On 08 Dec 2014, at 16:00, Charles Chance <charles.chance(a)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?
/O
Cheers,
Charles
On 8 December 2014 at 14:46, Olle E. Johansson <oej(a)edvina.net> wrote:
On 08 Dec 2014, at 15:42, Charles Chance <charles.chance(a)sipcentric.com> wrote:
Hi All,
Can anyone see a problem with doing (something like) the following, to handle the
situation like http://lists.sip-router.org/pipermail/sr-users/2014-October/085251.html?
Admittedly, it is probably a less common use case, but it has been raised several times
on the list so I believe it is a genuine one.
Did you try Daniel's advice?
"See msg_apply_changes() from textopsx to get the path header visible."
Did that not work?
/O
diff --git a/modules/registrar/lookup.c b/modules/registrar/lookup.c
index 794d968..66730b4 100644
--- a/modules/registrar/lookup.c
+++ b/modules/registrar/lookup.c
@@ -41,6 +41,7 @@
#include "../../action.h"
#include "../../mod_fix.h"
#include "../../parser/parse_rr.h"
+#include "../../forward.h"
#include "../usrloc/usrloc.h"
#include "common.h"
#include "regtime.h"
@@ -121,6 +122,7 @@ int lookup_helper(struct sip_msg* _m, udomain_t* _d, str* _uri, int
_mode)
sr_xavp_t *list=NULL;
str xname = {"ruid", 4};
sr_xval_t xval;
+ sip_uri_t path_uri;
ret = -1;
@@ -265,6 +267,14 @@ int lookup_helper(struct sip_msg* _m, udomain_t* _d, str* _uri, int
_mode)
ret = -3;
goto done;
}
+ 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;
+ }
+ }
+ /* Only use path-uri if non-local */
+ if (path_uri.host.s && !check_self(&(path_uri.host), 0, 0))
{
if (set_path_vector(_m, &ptr->path) < 0) {
LM_ERR("failed to set path vector\n");
ret = -3;
The above needs to be repeated in lookup_branches function of same file, but I wanted to
check others' opinions first.
Best regards,
Charles
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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
_______________________________________________
sr-dev mailing list
sr-dev(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
--
Charles Chance
Managing Director
t. 0121 285 4400 m. 07932 063 891
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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
4:40 p.m.
On 8 December 2014 at 15:09, Olle E. Johansson <oej(a)edvina.net> wrote:
On 08 Dec 2014, at 16:00, Charles Chance <charles.chance(a)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?
Thank you, Olle.
Best regards,
Charles
--
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.
5:28 p.m.
On 08/12/14 16:40, Charles Chance wrote:
On 8 December 2014 at 15:09, Olle E. Johansson <oej(a)edvina.net
<mailto:oej@edvina.net>> wrote:
On 08 Dec 2014, at 16:00, Charles Chance
<charles.chance(a)sipcentric.com
<mailto:charles.chance@sipcentric.com>> wrote:
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
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?
9:10 p.m.
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(a)gmail.com>
wrote:
On 08/12/14 16:40, Charles Chance wrote:
On 8 December 2014 at 15:09, Olle E. Johansson <oej(a)edvina.net> wrote:
--
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.
On 08 Dec 2014, at 16:00, Charles Chance <charles.chance(a)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 Mierlahttp://twitter.com/#!/miconda -
http://www.linkedin.com/in/miconda
_______________________________________________
sr-dev mailing list
sr-dev(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
9 Dec
9 Dec
4:14 p.m.
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(a)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(a)gmail.com>
wrote:
>
> On 08/12/14 16:40, Charles Chance wrote:
>
>
>
> On 8 December 2014 at 15:09, Olle E. Johansson <oej(a)edvina.net> wrote:
>
>>
>> On 08 Dec 2014, at 16:00, Charles Chance <charles.chance(a)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 Mierlahttp://twitter.com/#!/miconda -
http://www.linkedin.com/in/miconda
>
>
> _______________________________________________
> sr-dev mailing list
> sr-dev(a)lists.sip-router.org
> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
>
>
--
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.
11 Dec
11 Dec
11:24 a.m.
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(a)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(a)gmail.com>
wrote:
>
> On 08/12/14 16:40, Charles Chance wrote:
>
>
>
> On 8 December 2014 at 15:09, Olle E. Johansson <oej(a)edvina.net> wrote:
>
>>
>> On 08 Dec 2014, at 16:00, Charles Chance <charles.chance(a)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 Mierlahttp://twitter.com/#!/miconda -
http://www.linkedin.com/in/miconda
>
>
> _______________________________________________
> sr-dev mailing list
> sr-dev(a)lists.sip-router.org
> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
>
>
--
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.
1:52 p.m.
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(a)sipcentric.com <mailto: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(a)gmail.com
<mailto:miconda@gmail.com>> wrote:
On 08/12/14 16:40, Charles Chance wrote:
--
Daniel-Constantin Mierla
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
On 8 December 2014 at 15:09, Olle E. Johansson
<oej(a)edvina.net <mailto:oej@edvina.net>> wrote:
On 08 Dec 2014, at 16:00, Charles Chance
<charles.chance(a)sipcentric.com
<mailto:charles.chance@sipcentric.com>> wrote:
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(a)lists.sip-router.org <mailto:sr-dev@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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev 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?
2:10 p.m.
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(a)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(a)sipcentric.com
--
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.
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(a)gmail.com>
wrote:
>
> On 08/12/14 16:40, Charles Chance wrote:
>
>
>
> On 8 December 2014 at 15:09, Olle E. Johansson <oej(a)edvina.net> wrote:
>
>>
>> On 08 Dec 2014, at 16:00, Charles Chance <
>> charles.chance(a)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 Mierlahttp://twitter.com/#!/miconda -
http://www.linkedin.com/in/miconda
>
>
> _______________________________________________
> sr-dev mailing list
> sr-dev(a)lists.sip-router.org
> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
>
>
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
listsr-dev@lists.sip-router.orghttp://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
--
Daniel-Constantin Mierlahttp://twitter.com/#!/miconda -
http://www.linkedin.com/in/miconda
_______________________________________________
sr-dev mailing list
sr-dev(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
14 Dec
14 Dec
10:40 p.m.
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(a)gmail.com <mailto: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:
--
Daniel-Constantin Mierla
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
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(a)sipcentric.com
<mailto: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(a)gmail.com <mailto:miconda@gmail.com>> wrote:
On 08/12/14 16:40, Charles Chance wrote:
--
Daniel-Constantin Mierla
http://twitter.com/#!/miconda <http://twitter.com/#%21/miconda> -
http://www.linkedin.com/in/miconda
_______________________________________________
sr-dev mailing list
sr-dev(a)lists.sip-router.org <mailto:sr-dev@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.
On 8 December 2014 at 15:09, Olle E. Johansson
<oej(a)edvina.net <mailto:oej@edvina.net>> wrote:
On 08 Dec 2014, at 16:00, Charles Chance
<charles.chance(a)sipcentric.com
<mailto:charles.chance@sipcentric.com>> wrote:
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(a)lists.sip-router.org
<mailto:sr-dev@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(a)lists.sip-router.org <mailto:sr-dev@lists.sip-router.org>
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev 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?
15 Dec
15 Dec
1:58 p.m.
On 14 December 2014 at 21:40, Daniel-Constantin Mierla <miconda(a)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
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.
9 Jan
9 Jan
6:54 p.m.
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/635f23b12eff2431ca9a14bb39f4204…
https://github.com/kamailio/kamailio/commit/4ff2c48d66a3bce2c491b44d0f1b5e9…
Thanks in advance,
Charles
On 15 December 2014 at 12:58, Charles Chance <charles.chance(a)sipcentric.com>
wrote:
On 14 December 2014 at 21:40, Daniel-Constantin Mierla <miconda(a)gmail.com>
wrote:
--
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.
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
7 p.m.
Hey Victor,
On 9 January 2015 at 17:57, Victor Seva <linuxmaniac(a)torreviejawireless.org>
wrote:
On 01/09/2015 06:54 PM, Charles Chance wrote:
Thanks - are pull requests the preferred choice now we're on GitHub?
--
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.
Would anyone like to review the final changes,
before I merge them into
master?
I usually use pull-request to get some feedback. Just my 2 cents.
7:13 p.m.
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/635f23b12eff2431ca9a14bb39f4204…
https://github.com/kamailio/kamailio/commit/4ff2c48d66a3bce2c491b44d0f1b5e9…
Thanks in advance,
Charles
On 15 December 2014 at 12:58, Charles Chance
<charles.chance(a)sipcentric.com <mailto:charles.chance@sipcentric.com>>
wrote:
On 14 December 2014 at 21:40, Daniel-Constantin Mierla
<miconda(a)gmail.com <mailto:miconda@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
--
Daniel-Constantin Mierla
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
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.
7:20 p.m.
On 09/01/15 19:00, Charles Chance wrote:
Hey Victor,
On 9 January 2015 at 17:57, Victor Seva
<linuxmaniac(a)torreviejawireless.org
<mailto:linuxmaniac@torreviejawireless.org>> wrote:
On 01/09/2015 06:54 PM, Charles Chance wrote:
For external contributors (no direct commit access), probably is more
convenient due to the option to review and make comments inline the patch.
Otherwise, it can be from case to case, more or less the preference of
the developer. If it something that needs a larger review, probably the
pull request web interface on github offers more tools and ensures that
the discussion is not lost on mailing list. Practically is like
alternative to what we used to open a bug tracker item for a patch.
It seems it allows to do pull requests even from branches of kamailio
project, as I can see you did the pull request already. I expected that
it required to fork the repository on personal github account, do
changes and then make the pull request. That would have been heavy in my
opinion for devs with commit access.
Given the above, I would consider pull requests as 'preferred' instead
of opening tracker issues with patches. But again, not enforced (or at
least not now, until majority considers is the best to do).
Cheers,
Daniel
--
Daniel-Constantin Mierla
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Would anyone like to review the final changes,
before I merge
them into
master?
I usually use pull-request to get some feedback. Just my 2 cents.
Thanks - are pull requests the preferred choice now we're on GitHub?
7:45 p.m.
Yes, given that it is possible to do between branches of the same repo, I
vote for it being preferred also.
Thanks again,
Charles
On 9 Jan 2015 18:21, "Daniel-Constantin Mierla" <miconda(a)gmail.com>
wrote:
On 09/01/15 19:00, Charles Chance wrote:
Hey Victor,
On 9 January 2015 at 17:57, Victor Seva <
linuxmaniac(a)torreviejawireless.org> wrote:
--
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.
On 01/09/2015 06:54 PM, Charles Chance wrote:
Thanks - are pull requests the preferred choice now we're on GitHub?
For external contributors (no direct commit access), probably is more
convenient due to the option to review and make comments inline the patch.
Otherwise, it can be from case to case, more or less the preference of the
developer. If it something that needs a larger review, probably the pull
request web interface on github offers more tools and ensures that the
discussion is not lost on mailing list. Practically is like alternative to
what we used to open a bug tracker item for a patch.
It seems it allows to do pull requests even from branches of kamailio
project, as I can see you did the pull request already. I expected that it
required to fork the repository on personal github account, do changes and
then make the pull request. That would have been heavy in my opinion for
devs with commit access.
Given the above, I would consider pull requests as 'preferred' instead of
opening tracker issues with patches. But again, not enforced (or at least
not now, until majority considers is the best to do).
Cheers,
Daniel
--
Daniel-Constantin Mierlahttp://twitter.com/#!/miconda -
http://www.linkedin.com/in/miconda
_______________________________________________
sr-dev mailing list
sr-dev(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
Would anyone like to review the final changes,
before I merge them into
master?
I usually use pull-request to get some feedback. Just my 2 cents.
3418
days inactive
3450
days old
18 comments
4 participants
participants (4)
-
Charles Chance -
Daniel-Constantin Mierla -
Olle E. Johansson -
Victor Seva