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
On 08 Dec 2014, at 15:42, Charles Chance charles.chance@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@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).
Cheers, Charles
On 8 December 2014 at 14:46, Olle E. Johansson oej@edvina.net wrote:
On 08 Dec 2014, at 15:42, Charles Chance charles.chance@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@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
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? /O
Cheers, Charles
On 8 December 2014 at 14:46, Olle E. Johansson oej@edvina.net wrote:
On 08 Dec 2014, at 15:42, Charles Chance charles.chance@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@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
sr-dev mailing list sr-dev@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@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
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?
Thank you, Olle.
Best regards,
Charles
On 08/12/14 16:40, Charles Chance wrote:
On 8 December 2014 at 15:09, Olle E. Johansson <oej@edvina.net mailto:oej@edvina.net> wrote:
On 08 Dec 2014, at 16:00, Charles Chance <charles.chance@sipcentric.com <mailto: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
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 Mierlahttp://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
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 Mierlahttp://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
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 Mierlahttp://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
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 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@gmail.com <mailto: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 <mailto:oej@edvina.net>> wrote: On 08 Dec 2014, at 16:00, Charles Chance <charles.chance@sipcentric.com <mailto: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://twitter.com/#%21/miconda> - http://www.linkedin.com/in/miconda _______________________________________________ sr-dev mailing list sr-dev@lists.sip-router.org <mailto:sr-dev@lists.sip-router.org> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-devwww.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@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
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 Mierlahttp://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 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@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
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 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: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 <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@gmail.com <mailto: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 <mailto:oej@edvina.net>> wrote: On 08 Dec 2014, at 16:00, Charles Chance <charles.chance@sipcentric.com <mailto: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://twitter.com/#%21/miconda> - http://www.linkedin.com/in/miconda _______________________________________________ sr-dev mailing list sr-dev@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@lists.sip-router.org <mailto: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://twitter.com/#%21/miconda> - http://www.linkedin.com/in/miconda _______________________________________________ sr-dev mailing list sr-dev@lists.sip-router.org <mailto:sr-dev@lists.sip-router.org> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-devwww.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 14 December 2014 at 21:40, Daniel-Constantin Mierla 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 > 0But 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
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/635f23b12eff2431ca9a14bb39f4204d... https://github.com/kamailio/kamailio/commit/4ff2c48d66a3bce2c491b44d0f1b5e93...
Thanks in advance,
Charles
On 15 December 2014 at 12:58, Charles Chance charles.chance@sipcentric.com wrote:
On 14 December 2014 at 21:40, Daniel-Constantin Mierla 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 > 0But 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
Hey Victor,
On 9 January 2015 at 17:57, Victor Seva linuxmaniac@torreviejawireless.org wrote:
On 01/09/2015 06:54 PM, Charles Chance wrote:
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?
On 09/01/15 19:00, Charles Chance wrote:
Hey Victor,
On 9 January 2015 at 17:57, Victor Seva <linuxmaniac@torreviejawireless.org mailto:linuxmaniac@torreviejawireless.org> wrote:
On 01/09/2015 06:54 PM, Charles Chance wrote: > 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?
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
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@gmail.com wrote:
On 09/01/15 19:00, Charles Chance wrote:
Hey Victor,
On 9 January 2015 at 17:57, Victor Seva < linuxmaniac@torreviejawireless.org> wrote:
On 01/09/2015 06:54 PM, Charles Chance wrote:
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?
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@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
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/635f23b12eff2431ca9a14bb39f4204d... https://github.com/kamailio/kamailio/commit/4ff2c48d66a3bce2c491b44d0f1b5e93...
Thanks in advance,
Charles
On 15 December 2014 at 12:58, Charles Chance <charles.chance@sipcentric.com mailto:charles.chance@sipcentric.com> wrote:
On 14 December 2014 at 21:40, Daniel-Constantin Mierla <miconda@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 >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, Charleswww.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.
-
Charles Chance -
Daniel-Constantin Mierla -
Olle E. Johansson -
Victor Seva