Summary:	ASTERISK-18331: app_sms failure
Reporter:	David Woodhouse (dwmw2)	Labels:
Date Opened:	2011-08-23 18:25:33	Date Closed:	2014-04-18 20:57:16
Priority:	Major	Regression?	Yes
Status:	Closed/Complete	Components:	Applications/app_sms
Versions:	1.8.5.0	Frequency of Occurrence	Constant
Related Issues:	duplicates ASTERISK-18027 SMS queued with smsq not sent is related to ASTERISK-18165 sms sending does not work
Environment:	Fedora 15 i386, B200P ISDN BRI, Dadhi.	Attachments:	( 0) asterisk-fix-sms.patch ( 1) pbx-handle-link.patch
Description:	There are three issues with app_sms. Firstly, the 'flags' field in the sms_exec() function is not initialised, causing the wrong protocol to be selected in some cases because the OPTION_TWO flag is erroneously set. This is described at https://issues.asterisk.org/jira/browse/ASTERISK-15361?focusedCommentId=160885#comment-160885 Secondly, smsq seems to work by creating a call file and then hard-linking it into the spool directory /var/spool/asterisk/outgoing. This appears to be correct behaviour according to the documentation. However, Asterisk's use of inotify has *broken* this, in favour of broken clients which do the wrong thing. Asterisk sees that hard link as a IN_CREATE event, and it assumes a broken client and *waits* for a subsequent IN_CLOSE_WRITE event on the same file, indicating that the file has been closed after being opened for write. Such an event never comes, of course. The smsq tool just unlinks its original temporary filename for the same file. I fix that second issue by using rename() instead of link() + unlink() in smsq.c, but really I'm disappointed that Asterisk penalised a valid client in order to pander to broken clients. If you're going to watch for IN_CLOSE_WRITE that's cute, but you should notice it and *refuse* to run the call file, with a clear explanation of why. And that way the broken clients can be fixed. The third issue is as described in ASTERISK-15426 — app_sms does not behave correctly when the far end stops sending data and the call isn't terminated for some reason. Just putting the trailing \n on the log message is a good start, but my patch also make app_sms handle this situation gracefully if it's already sent the REL message to instruct the other end to terminate the call. If we've sent a REL message and then the peer stops sending data, *don't* just bitch about it over and over again in the logs and keep the channel open. Just hang up our end too. With these changes, app_sms is working properly again for me with British Telecom.
Comments:	By: David Woodhouse (dwmw2) 2011-08-23 18:28:00.098-0500 Patch to fix all three issues By: David Woodhouse (dwmw2) 2011-08-26 16:45:18.301-0500 Here's a better fix for the smsq problem. In fact it wasn't trivial to replace the link() unlink() calls with a single rename() — because rename() can overwrite existing spool files, while link() is guaranteed not to do so. So as discussed on IRC, I've made the inotify code in pbx_spool cope with hard links by detecting the absence of an IN_OPEN event following the IN_CREATE. By: Leif Madsen (lmadsen) 2011-09-14 15:18:28.016-0500 This likely a good candidate for reviewboard then. If you can get a Ship It from another developer I would be happy to commit this for you. Because app_sms is a community support module you'll need to take the lead on moving the issue forward. Thanks! By: David Woodhouse (dwmw2) 2011-09-14 15:22:10.584-0500 My final fix for issue #2 already has a 'Ship it': https://reviewboard.asterisk.org/r/1391 The remaining two issues are in https://reviewboard.asterisk.org/r/1392 By: Tony Yates (tonyy451) 2014-04-14 05:18:46.369-0500 Long time user of app_sms/smsq on UK POTS.  I note that the patches referred to above by David Woodhouse still require manual addition to current versions 11.x.y, at least, in order to make app_sms work in UK (& elsewhere?). Any reason why they were not adopted?   Could this issue be re-opened please? By: Matt Jordan (mjordan) 2014-04-14 06:46:56.933-0500 The patch to {{pbx_spool}} (https://reviewboard.asterisk.org/r/1391/) was committed. I have a feeling this issue was closed as a result of that commit. The patch to {{app_sms}} was not. I took a quick look at it (https://reviewboard.asterisk.org/r/1392/) and it doesn't look like there's anything particularly wrong with it. I'll reopen this issue until the patch on r1392 is committed. Thanks for noting that it wasn't committed.