<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1956/">https://reviewboard.asterisk.org/r/1956/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 18th, 2012, 11:03 a.m., <b>jrose</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hey, I was talking about this issue with Matt Jordan a while ago and he came up with the following:
Since we can't really guarantee under any particular circumstance what a given return code will mean for res here, it'd be more appropriate to leave the code here as is, but set a channel variable based on the res value to allow the author of the dialplan to react to it since they should know what /bin/sh will be (and what the return codes would map to).</pre>
</blockquote>
<p>On June 18th, 2012, 12:52 p.m., <b>Tilghman Lesher</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">While that's a reasonable approach in terms of a method by which to get a result, our approach has generally been that for things which return a value, we use a dialplan function, instead. So, perhaps add a SYSTEM() dialplan function, which returns the result code.</pre>
</blockquote>
<p>On June 18th, 2012, 1:05 p.m., <b>jrose</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Well, that's true, but this is really more of a status thing than an actual return from the System command being ran... and doing it this way isn't unprecedented (app_queue for instance sets QUEUESTATUS which indicates the the status of the queue at exit). This does remind me though that there is a SHELL dialplan function that works similarly and might also need to be looked at when making these changes.</pre>
</blockquote>
<p>On June 18th, 2012, 1:22 p.m., <b>Matt Jordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hey now, I'm pretty sure I didn't say "channel variable" :-)
I think I said we should expose the return code somehow to the user, and that it could be in the dialplan. Using a function that returns a result code is a perfectly fine implementation of that.
>From an implementation perspective, I'd lean away from a channel variable. Many channels would not ever need or care about a generic 'system call return value' variable, and I would hate to have it always around with a channel. Or worse, have it around when the system call returns, but not have it be present on a channel otherwise - those types of things are rarely documented well, and people would have to know when the variable exists and when it doesn't.
What's more, the system call may not even be 'channel specific', that is, it may not change or affect the state of the channel. The script could print out "Hello world!" - so associating its return value with a channel seems to imply a dependency that does not necessarily exist.
Tilghman's suggestion of tying the return code of the script directly with the item that executed the script makes sense: we simply return the return code of the thing that was executed.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Ah, alright then. Sorry for misinterpreting.
I'm just going to go ahead and mention that if another function for invoking system commands is added (like func_system), manager will need to be updated to check manager users for SYSTEM write access where app_system and func_shell are currently checked. It'd be a fairly trivial change, but not doing it could lead to a security vulnerability.</pre>
<br />
<p>- jrose</p>
<br />
<p>On June 4th, 2012, 9:23 a.m., Denis Martinez wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Denis Martinez.</div>
<p style="color: grey;"><i>Updated June 4, 2012, 9:23 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I created a patch that improves the app_system behavior. The patch should make this application to check if a command failed to execute due to permission denied.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-19935">ASTERISK-19935</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/apps/app_system.c <span style="color: grey">(368031)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1956/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>