Summary:ASTERISK-18331: app_sms failure
Reporter:David Woodhouse (dwmw2)Labels:
Date Opened:2011-08-23 18:25:33Date Closed:2014-04-18 20:57:16
Versions: Frequency of
duplicatesASTERISK-18027 SMS queued with smsq not sent
is related toASTERISK-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.