Summary:	ASTERISK-03441: [new app] app_readpipe
Reporter:	Matt O'Gorman (mogorman)	Labels:
Date Opened:	2005-02-07 02:53:30.000-0600	Date Closed:	2005-02-26 12:18:42.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Applications/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) app_execute.c ( 1) app_popen.c ( 2) app_popen.c ( 3) app_popen.c ( 4) app_popen.c ( 5) app_readpipe.c ( 6) app_readpipe.c
Description:	App_readpipe, runs the popen command from C and allows you to store the result in a channel variable.  Its syntax is varname=command[|lines].  Length is the number of lines to pull in; the default is to return all of the output of the command.  This is my first real app I hope y'all enjoy.
Comments:	By: Olle Johansson (oej) 2005-02-07 08:54:14.000-0600 Thank you for contributing. In order for us to look at this patch and, if approved, include it in cvs, you need to fax in a disclaimer to Digium and mention that in this bug report. Read the bug guidelines again for instructions. By: Olle Johansson (oej) 2005-02-07 08:58:55.000-0600 A few comments: * Read the coding guidelines (in your /doc source code directory). * All variables must be lower case * Fix intendation - you have a problem around where you assign the dialplan variable Also, I think the syntax should possibly follow other functions that set variables, like dbget and setvar... popen(var=program,lines) What do you think? By: Matt O'Gorman (mogorman) 2005-02-07 11:00:34.000-0600 Hi, already have disclaimer on file as I have two patches in asterisk.  I have changed the syntax to popen(var=program,lines)  with lines still optional, and quotes supported for more complicated and piped commands.  Thanks for the review, I just realized I did not update, the man page for command, use latest file instead of the other By: Matt O'Gorman (mogorman) 2005-02-07 11:26:13.000-0600 eep! one more bug, I am sorry for not using diffs, will do when I am back on my linux box. By: Olle Johansson (oej) 2005-02-07 14:10:34.000-0600 You always need to mention that you have a disclaimer on file - in every bug report. I wonder if readpipe() is a better name for non-c-programmers? You don't need to do diffs since this a new file. Thank you anyway. By: Matt O'Gorman (mogorman) 2005-02-07 15:17:01.000-0600 execute() perhaps? By: Kevin P. Fleming (kpfleming) 2005-02-07 16:00:29.000-0600 Yes, popen is not a good name. Since this does a very similar thing to what System() does, but preserves the output, maybe something like SystemCapture() would be acceptable? By: Matt O'Gorman (mogorman) 2005-02-07 16:14:29.000-0600 any other suggestions? By: twisted (twisted) 2005-02-07 17:55:47.000-0600 ooh... a very useful app, Matt ;)  FYI - when adding new apps, diffs arent' necessary, in fact, they're a burden when trying to commit to cvs.   Also, since this code is disclaimed, you will also need to add somewhere in the header a copyright to digium as well. For an example, look in app_skel.c Thanks again for a great contribution, Mog! By: Tilghman Lesher (tilghman) 2005-02-07 18:17:49.000-0600 1. You shouldn't declare any routine name as ast_* in an app_ module.  The syntax is reserved for public interfaces (routines that are available in the core and aren't unloadable).  Additionally, the routine should be declared static. 2. Your ast_popen routine leaks a single byte each time it's called, with that initial malloc of a single char.  It should be enough simply to remove that one line. 3. It's unnecessary to check each time to see if your string contains a particular character before parsing it with strsep(3); that routine is fairly safe.  Note that strsep(3) behaves sanely, even when called with a pointer that points at NULL. 4. Other than these issues and the coding guideline issues, looks great! By: Matt O'Gorman (mogorman) 2005-02-07 19:13:17.000-0600 okay here is the updated app now named execute, got rid of the mem leak, added all rights assigned to Digium. And other minor fixes it is basically same. By: nick (nick) 2005-02-07 19:41:29.000-0600 I'm not sure execute() is the best name for it either... NB By: Matt O'Gorman (mogorman) 2005-02-07 19:52:44.000-0600 ahhhh... someone pick one then... i like execute but i would rather more people use it than it have a pretty name.   so suggest away... By: k3v (k3v) 2005-02-08 00:17:11.000-0600 SystemSetvar?  SystemInput?  SystemStdin? By: Tilghman Lesher (tilghman) 2005-02-08 00:45:18.000-0600 Execute() will almost certainly be confused with Exec(), which already exists.  I think oej's suggestion of ReadPipe is a fine one. By: Mark Spencer (markster) 2005-02-08 00:55:05.000-0600 You'll notice that there are no "popen" calls in Asterisk.  That's no accident.  I'm just trying to remember why.  I think it had to do with errors during the execution of programs.  Instead we write to a file and then open the file. By: Tilghman Lesher (tilghman) 2005-02-08 01:07:51.000-0600 Mark:  it's possibly because the buffered nature of popen could cause deadlocks, if used with both stdin and stdout. By: Matt O'Gorman (mogorman) 2005-02-08 07:25:45.000-0600 okay, so what if this was rewritten to create a tmp file in var/spool/asterisk/readpipe, with the name of the channel as the file name, of the output of popen, and then that is read in, then it is deleted.  is that safe enough for general asterisk public? By: Matt O'Gorman (mogorman) 2005-02-08 08:16:54.000-0600 also corydon your version of my app doesnt except the pipes in quotes.  I mean you shrunk down the reading part, but it doesnt parse the way mine does, for example mine could handle the following inputs exten=> 5555,1,popen(TEMP=ls) exten=> 5555,n,noop(${TEMP}) exten=> 5555,n,popen(TEMP=ls|2) exten=> 5555,n,noop(${TEMP}) exten=> 5555,n,popen(TEMP="ls -l") exten=> 5555,n,noop(${TEMP}) exten=> 5555,n,popen(TEMP="ls -l|grep *"|3) exten=> 5555,n,noop(${TEMP}) exten=> 5555,n,popen(TEMP="ls -l",3) exten=> 5555,n,noop(${TEMP}) exten=> 5555,n,wait(3) exten=> 5555,n,popen(TEMP=ls|a) exten=> 5555,n,noop(${TEMP}) exten=> 5555,n,popen(TEMP="ls -l") exten=> 5555,n,noop(${TEMP}) exten=> 5555,n,popen(TEMP="ls -l"|a) exten=> 5555,n,noop(${TEMP}) exten=> 5555,n,popen(TEMP="ls -l",a) exten=> 5555,n,noop(${TEMP}) yours fails on any with | encapsulated in the "" and all of my fault cases.  I just thought for the extra few lines it was worth adding in that extra functionality. By: Tilghman Lesher (tilghman) 2005-02-08 09:13:27.000-0600 It still doesn't require all the extra parsing (see updated).  Also, I had missed the other memory leak, which was that after you allocated the buffer for the results, you never freed it. By: Tilghman Lesher (tilghman) 2005-02-08 09:19:03.000-0600 On the use of popen:  for this limited functionality, it's probably fine, as long as you don't attempt to do both stdin and stdout on the same pipe. In terms of opening an output file on the filesystem, note what happens when two different channels run ReadPipe at the same time.  You'll need to plan for that. By: Matt O'Gorman (mogorman) 2005-02-08 09:43:06.000-0600 hey, this is updated just slightly, you had extra variable, and it now passes my run tests.   also I was just going to name the file the name of the channel name so that there would be no collisions. in that directory I mentioned By: Mark Spencer (markster) 2005-02-08 13:51:11.000-0600 I thought you were going to make it a read pipe? By: Matt O'Gorman (mogorman) 2005-02-08 14:36:15.000-0600 what mark?  Like we discussed going to redo this, thinking better impelmentation would be to alter or add file reader into asterisk, and alter system, to function more like popen, by creating a file in /var/spool/asterisk/system/$CHAN_NAME , and then read that in, and delete it as it would be more safe.  Thoughts? By: Matt O'Gorman (mogorman) 2005-02-08 14:36:42.000-0600 oh and also probably add a app_reader as well that just calls the ast function. By: Mark Spencer (markster) 2005-02-26 10:47:57.000-0600 Folding this into the updated bug ASTERISK-3590