Re: [asterisk-dev] [Code Review] 2944: PJSIP messaging: send message to URI.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2944/ --- (Updated Dec. 11, 2013, 2:31 p.m.) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Repository: Asterisk Description --- Added the ability to send messages to a URI (an associated endpoint is no longer needed). Diffs - branches/12/res/res_pjsip_messaging.c 403400 branches/12/res/res_pjsip/pjsip_configuration.c 403400 branches/12/res/res_pjsip/include/res_pjsip_private.h 403400 branches/12/res/res_pjsip/config_global.c 403400 branches/12/res/res_pjsip.c 403400 branches/12/include/asterisk/res_pjsip.h 403400 branches/12/configs/pjsip.conf.sample 403400 Diff: https://reviewboard.asterisk.org/r/2944/diff/ Testing --- Had the PJSIP message module send a SIP message to a specified IP address. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 2944: PJSIP messaging: send message to URI.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2944/ --- (Updated Dec. 9, 2013, 10:17 a.m.) Review request for Asterisk Developers. Changes --- Addressed review findings. Repository: Asterisk Description --- Added the ability to send messages to a URI (an associated endpoint is no longer needed). Diffs (updated) - branches/12/res/res_pjsip_messaging.c 403400 branches/12/res/res_pjsip/pjsip_configuration.c 403400 branches/12/res/res_pjsip/include/res_pjsip_private.h 403400 branches/12/res/res_pjsip/config_global.c 403400 branches/12/res/res_pjsip.c 403400 branches/12/include/asterisk/res_pjsip.h 403400 branches/12/configs/pjsip.conf.sample 403400 Diff: https://reviewboard.asterisk.org/r/2944/diff/ Testing --- Had the PJSIP message module send a SIP message to a specified IP address. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 2944: PJSIP messaging: send message to URI.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2944/#review10338 --- Ship it! Ship It! - Mark Michelson On Dec. 9, 2013, 4:17 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2944/ --- (Updated Dec. 9, 2013, 4:17 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- Added the ability to send messages to a URI (an associated endpoint is no longer needed). Diffs - branches/12/res/res_pjsip_messaging.c 403400 branches/12/res/res_pjsip/pjsip_configuration.c 403400 branches/12/res/res_pjsip/include/res_pjsip_private.h 403400 branches/12/res/res_pjsip/config_global.c 403400 branches/12/res/res_pjsip.c 403400 branches/12/include/asterisk/res_pjsip.h 403400 branches/12/configs/pjsip.conf.sample 403400 Diff: https://reviewboard.asterisk.org/r/2944/diff/ Testing --- Had the PJSIP message module send a SIP message to a specified IP address. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 2944: PJSIP messaging: send message to URI.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2944/#review10319 --- branches/12/res/res_pjsip/config_global.c https://reviewboard.asterisk.org/r/2944/#comment19718 I don't think this change is warranted. branches/12/res/res_pjsip/config_global.c https://reviewboard.asterisk.org/r/2944/#comment19720 This function has a problem similar to some of the problems Josh pointed out on his review. Since cfg is scoped to this function, once this function returns, cfg-default_outbound_endpoint cannot be assumed safe to use since cfg (and its contents) may get freed. - Mark Michelson On Dec. 5, 2013, 8:26 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2944/ --- (Updated Dec. 5, 2013, 8:26 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- Added the ability to send messages to a URI (an associated endpoint is no longer needed). Diffs - branches/12/res/res_pjsip_messaging.c 403341 branches/12/res/res_pjsip/pjsip_configuration.c 403341 branches/12/res/res_pjsip/include/res_pjsip_private.h 403341 branches/12/res/res_pjsip/config_global.c 403341 branches/12/res/res_pjsip.c 403341 branches/12/include/asterisk/res_pjsip.h 403341 branches/12/configs/pjsip.conf.sample 403341 Diff: https://reviewboard.asterisk.org/r/2944/diff/ Testing --- Had the PJSIP message module send a SIP message to a specified IP address. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 2944: PJSIP messaging: send message to URI.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2944/#review10303 --- branches/12/configs/pjsip.conf.sample https://reviewboard.asterisk.org/r/2944/#comment19688 I'd extend this explanation a bit more. Endpoint to use when sending an outbound request to a URI without a specified endpoint. branches/12/res/res_pjsip.c https://reviewboard.asterisk.org/r/2944/#comment19682 Stray ; at the end of the default value. branches/12/res/res_pjsip_messaging.c https://reviewboard.asterisk.org/r/2944/#comment19683 sips should still be valid here, otherwise you can't get the PJSIP transport manager to use a TLS transport branches/12/res/res_pjsip_messaging.c https://reviewboard.asterisk.org/r/2944/#comment19684 I don't like this. Falling back to the default outbound endpoint if the specified one could not be found could bring about unexpected behavior for the user, and be harder to diagnose. I would prefer that it would be an error situation. branches/12/res/res_pjsip_messaging.c https://reviewboard.asterisk.org/r/2944/#comment19685 As this now properly decs the reference count on the contact you are not guaranteed that contact-uri will remain valid past this. branches/12/res/res_pjsip_messaging.c https://reviewboard.asterisk.org/r/2944/#comment19686 Same applies here since it is scoped, unless I'm crazy. branches/12/res/res_pjsip_messaging.c https://reviewboard.asterisk.org/r/2944/#comment19687 You could make this error a bit more ... warm. ;) - Joshua Colp On Dec. 3, 2013, 8:12 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2944/ --- (Updated Dec. 3, 2013, 8:12 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- Added the ability to send messages to a URI (an associated endpoint is no longer needed). Diffs - branches/12/res/res_pjsip_messaging.c 403341 branches/12/res/res_pjsip/pjsip_configuration.c 403341 branches/12/res/res_pjsip/include/res_pjsip_private.h 403341 branches/12/res/res_pjsip/config_global.c 403341 branches/12/res/res_pjsip.c 403341 branches/12/include/asterisk/res_pjsip.h 403341 branches/12/configs/pjsip.conf.sample 403341 Diff: https://reviewboard.asterisk.org/r/2944/diff/ Testing --- Had the PJSIP message module send a SIP message to a specified IP address. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 2944: PJSIP messaging: send message to URI.
On Dec. 5, 2013, 6:56 a.m., Joshua Colp wrote: branches/12/res/res_pjsip_messaging.c, lines 155-156 https://reviewboard.asterisk.org/r/2944/diff/3/?file=48992#file48992line155 I don't like this. Falling back to the default outbound endpoint if the specified one could not be found could bring about unexpected behavior for the user, and be harder to diagnose. I would prefer that it would be an error situation. Where would the default be used then? I figured by specifying a default outbound endpoint in the config file the user would be aware of the fact that if an endpoint is not found on an outgoing message then the default would be used. - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2944/#review10303 --- On Dec. 3, 2013, 2:12 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2944/ --- (Updated Dec. 3, 2013, 2:12 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- Added the ability to send messages to a URI (an associated endpoint is no longer needed). Diffs - branches/12/res/res_pjsip_messaging.c 403341 branches/12/res/res_pjsip/pjsip_configuration.c 403341 branches/12/res/res_pjsip/include/res_pjsip_private.h 403341 branches/12/res/res_pjsip/config_global.c 403341 branches/12/res/res_pjsip.c 403341 branches/12/include/asterisk/res_pjsip.h 403341 branches/12/configs/pjsip.conf.sample 403341 Diff: https://reviewboard.asterisk.org/r/2944/diff/ Testing --- Had the PJSIP message module send a SIP message to a specified IP address. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 2944: PJSIP messaging: send message to URI.
On Dec. 5, 2013, 12:56 p.m., Joshua Colp wrote: branches/12/res/res_pjsip_messaging.c, lines 155-156 https://reviewboard.asterisk.org/r/2944/diff/3/?file=48992#file48992line155 I don't like this. Falling back to the default outbound endpoint if the specified one could not be found could bring about unexpected behavior for the user, and be harder to diagnose. I would prefer that it would be an error situation. Kevin Harwell wrote: Where would the default be used then? I figured by specifying a default outbound endpoint in the config file the user would be aware of the fact that if an endpoint is not found on an outgoing message then the default would be used. I'd only use it if no endpoint has been specified. I'd rather our behavior be strict (just like the configuration stuff is) instead of implicit behavior which may not be anywhere near what the user wants, and could end up causing problems. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2944/#review10303 --- On Dec. 3, 2013, 8:12 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2944/ --- (Updated Dec. 3, 2013, 8:12 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- Added the ability to send messages to a URI (an associated endpoint is no longer needed). Diffs - branches/12/res/res_pjsip_messaging.c 403341 branches/12/res/res_pjsip/pjsip_configuration.c 403341 branches/12/res/res_pjsip/include/res_pjsip_private.h 403341 branches/12/res/res_pjsip/config_global.c 403341 branches/12/res/res_pjsip.c 403341 branches/12/include/asterisk/res_pjsip.h 403341 branches/12/configs/pjsip.conf.sample 403341 Diff: https://reviewboard.asterisk.org/r/2944/diff/ Testing --- Had the PJSIP message module send a SIP message to a specified IP address. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 2944: PJSIP messaging: send message to URI.
On Dec. 5, 2013, 6:56 a.m., Joshua Colp wrote: branches/12/res/res_pjsip_messaging.c, lines 155-156 https://reviewboard.asterisk.org/r/2944/diff/3/?file=48992#file48992line155 I don't like this. Falling back to the default outbound endpoint if the specified one could not be found could bring about unexpected behavior for the user, and be harder to diagnose. I would prefer that it would be an error situation. Kevin Harwell wrote: Where would the default be used then? I figured by specifying a default outbound endpoint in the config file the user would be aware of the fact that if an endpoint is not found on an outgoing message then the default would be used. Joshua Colp wrote: I'd only use it if no endpoint has been specified. I'd rather our behavior be strict (just like the configuration stuff is) instead of implicit behavior which may not be anywhere near what the user wants, and could end up causing problems. After speaking with Josh on IRC it was decided that there is really no good way to distinguish between an endpoint vs. an ip address. Especially since an endpoint name could look just like an ip address. So, going to leave the code as is for this case since we want to check that it is potentially a valid endpoint first. - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2944/#review10303 --- On Dec. 3, 2013, 2:12 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2944/ --- (Updated Dec. 3, 2013, 2:12 p.m.) Review request for Asterisk Developers. Repository: Asterisk Description --- Added the ability to send messages to a URI (an associated endpoint is no longer needed). Diffs - branches/12/res/res_pjsip_messaging.c 403341 branches/12/res/res_pjsip/pjsip_configuration.c 403341 branches/12/res/res_pjsip/include/res_pjsip_private.h 403341 branches/12/res/res_pjsip/config_global.c 403341 branches/12/res/res_pjsip.c 403341 branches/12/include/asterisk/res_pjsip.h 403341 branches/12/configs/pjsip.conf.sample 403341 Diff: https://reviewboard.asterisk.org/r/2944/diff/ Testing --- Had the PJSIP message module send a SIP message to a specified IP address. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 2944: PJSIP messaging: send message to URI.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2944/ --- (Updated Dec. 5, 2013, 2:26 p.m.) Review request for Asterisk Developers. Changes --- Updated per review findings. Repository: Asterisk Description --- Added the ability to send messages to a URI (an associated endpoint is no longer needed). Diffs (updated) - branches/12/res/res_pjsip_messaging.c 403341 branches/12/res/res_pjsip/pjsip_configuration.c 403341 branches/12/res/res_pjsip/include/res_pjsip_private.h 403341 branches/12/res/res_pjsip/config_global.c 403341 branches/12/res/res_pjsip.c 403341 branches/12/include/asterisk/res_pjsip.h 403341 branches/12/configs/pjsip.conf.sample 403341 Diff: https://reviewboard.asterisk.org/r/2944/diff/ Testing --- Had the PJSIP message module send a SIP message to a specified IP address. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 2944: PJSIP messaging: send message to URI.
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/2944/ --- (Updated Dec. 3, 2013, 2:12 p.m.) Review request for Asterisk Developers. Changes --- Updated based on review findings/feedback. Going with Mark Michelson's #2 suggestion: Sending to an arbitray URI now requires a default outbound endpoint that can be specified under the global options section within pjsip.conf. Also fixed a couple minor off nominal memory leaks in res_pjsip_messaging. Repository: Asterisk Description --- Added the ability to send messages to a URI (an associated endpoint is no longer needed). Diffs (updated) - branches/12/res/res_pjsip_messaging.c 403341 branches/12/res/res_pjsip/pjsip_configuration.c 403341 branches/12/res/res_pjsip/include/res_pjsip_private.h 403341 branches/12/res/res_pjsip/config_global.c 403341 branches/12/res/res_pjsip.c 403341 branches/12/include/asterisk/res_pjsip.h 403341 branches/12/configs/pjsip.conf.sample 403341 Diff: https://reviewboard.asterisk.org/r/2944/diff/ Testing --- Had the PJSIP message module send a SIP message to a specified IP address. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
