Re: New function proposal

[David Bateman]

David Bateman wrote:
> address@hidden wrote:
>   
>>> Ok, I'm now compiling this. However, I'd like to add a unit test to the
>>> popen2 code. Taking the example code from popen2 and modifying it
>>> slightly I have a test like
>>>       
>>  
>> I did some additional tests and found out that the problem isn't really
>> the fact that reading a non-blocking pipe always looks like an EOF
>> whether there's no input or the pipe is closed, but more the fact that
>> errno is not reset by the fgets call, such that you don't see any
>> difference
>> when the actual EOF is reached. If you reset errno before calling fgets,
>> then it works with non-blocking pipes, except that you have to check
>> for EINVAL instead of EAGAIN.
>>     
> I have a patch that sort of works for posix, attached. The main issues
> were that it seems that somewhere in oct-syscall.cc stdio.h is being
> included, and that stdio.h typedef's stdin and stdout to _IO_FILE*. Then
> there is a type mismatch in the second argument to dup2.. I just replace
> stdin with STDIN_FILENO, and the same for stdout and it seems to work.
> Also got rid of the "panic" call as this case should return an error to
> octave rather than panicking..
>
> However, I have an issue in that the execvp call in
> octave_syscalls::popen2 seems to be ignoring the arguments and therefore
> popen2("sort","-nr") gives the same results as popen2("sort")!! I've
> looked and can't quite see why this is happening, and so thought to at
> least put the patch I have that sort of works available so that someone
> else might look at it..
>
> I'll keep looking and see if I can figure out this strange behavior..
>
>   
Ok, I understand the issue... The posix exec family of functions assume
that the first value in the argument list is the command to execute
itself. Therefore, for a command like "sort -nr" execvp in the popen2
posix function should have called execvp with "sort" for the command and
"sort -nr" for the argument list.. The attached patch fixes this and a
couple of other things like a spurious error when killing the child
process when the execvp fails...

I do however see a number of defunct child processes, but that is no
different from the old popen2 function.. I think this patch is good to go..

Regards
David



-- 
David Bateman                                address@hidden
Motorola Labs - Paris                        +33 1 69 35 48 04 (Ph) 
Parc Les Algorithmes, Commune de St Aubin    +33 6 72 01 06 33 (Mob) 
91193 Gif-Sur-Yvette FRANCE                  +33 1 69 35 77 01 (Fax) 

The information contained in this communication has been classified as: 

[x] General Business Information 
[ ] Motorola Internal Use Only 
[ ] Motorola Confidential Proprietary

*** ./liboctave/lo-sysdep.cc.orig33     2007-01-18 16:18:08.705157955 +0100
--- ./liboctave/lo-sysdep.cc    2007-02-15 18:08:17.775636069 +0100
***************
*** 27,32 ****
--- 27,33 ----
  
  #include <iostream>
  #include <string>
+ #include <vector>
  
  #ifdef HAVE_UNISTD_H
  #ifdef HAVE_SYS_TYPES_H
***************
*** 35,44 ****
--- 36,50 ----
  #include <unistd.h>
  #endif
  
+ #ifdef HAVE_FCNTL_H
+ #include <fcntl.h>
+ #endif
+ 
  #include "file-ops.h"
  #include "lo-error.h"
  #include "pathlen.h"
  #include "lo-sysdep.h"
+ #include "str-vec.h"
  
  std::string
  octave_getcwd (void)
***************
*** 99,105 ****
  #endif
  }
  
! #if defined (_MSC_VER)
  
  // FIXME -- it would probably be better to adapt the versions of
  // opendir, readdir, and closedir from Emacs as they appear to be more
--- 105,174 ----
  #endif
  }
  
! #if defined (__WIN32__) && ! defined (__CYGWIN__)
! 
! pid_t
! octave_popen2 (const std::string& cmd, const string_vector& args, bool 
sync_mode,
!     int *fildes, std::string& msg)
! {
!   pid_t pid;
!   PROCESS_INFORMATION pi;
!   STARTUPINFO si;
!   std::string command = "\"" + cmd + "\"";
!   HANDLE hProcess = GetCurrentProcess(), childRead, childWrite, parentRead, 
parentWrite;
!   DWORD pipeMode;
! 
!   ZeroMemory (&pi, sizeof (pi));
!   ZeroMemory (&si, sizeof (si));
!   si.cb = sizeof (si);
! 
!   if (! CreatePipe (&childRead, &parentWrite, 0, 0) ||
!       ! DuplicateHandle (hProcess, childRead, hProcess, &childRead, 0, TRUE, 
DUPLICATE_SAME_ACCESS | DUPLICATE_CLOSE_SOURCE))
!     {
!       msg = "popen2: pipe creation failed";
!       return -1;
!     }
!   if (! CreatePipe (&parentRead, &childWrite, 0, 0) ||
!       ! DuplicateHandle (hProcess, childWrite, hProcess, &childWrite, 0, 
TRUE, DUPLICATE_SAME_ACCESS | DUPLICATE_CLOSE_SOURCE))
!     {
!       msg = "popen2: pipe creation failed";
!       return -1;
!     }
!   if (! sync_mode)
!     {
!       pipeMode = PIPE_NOWAIT;
!       SetNamedPipeHandleState (parentRead, &pipeMode, 0, 0);
!       SetNamedPipeHandleState (parentWrite, &pipeMode, 0, 0);
!     }
!   fildes[1] = _open_osfhandle (reinterpret_cast<long> (parentRead), _O_RDONLY 
| _O_BINARY);
!   fildes[0] = _open_osfhandle (reinterpret_cast<long> (parentWrite), 
_O_WRONLY | _O_BINARY);
!   si.dwFlags |= STARTF_USESTDHANDLES;
!   si.hStdInput = childRead;
!   si.hStdOutput = childWrite;
! 
!   // Ignore first arg as it is the command
!   for (int k=1; k<args.length(); k++)
!     command += " \"" + args[k] + "\"";
!   OCTAVE_LOCAL_BUFFER (char, c_command, command.length () + 1);
!   strcpy (c_command, command.c_str ());
!   if (! CreateProcess (0, c_command, 0, 0, TRUE, 0, 0, 0, &si, &pi))
!     {
!       msg = "popen2: process creation failed";
!       return -1;
!     }
!   pid = pi.dwProcessId;
! 
!   CloseHandle (childRead);
!   CloseHandle (childWrite);
!   CloseHandle (pi.hProcess);
!   CloseHandle (pi.hThread);
! 
!   return pid;
! }
! 
! #endif
! 
! #if defined (_MSC_VER) && ! defined (HAVE_DIRENT_H)
  
  // FIXME -- it would probably be better to adapt the versions of
  // opendir, readdir, and closedir from Emacs as they appear to be more
*** ./liboctave/lo-sysdep.h.orig33      2006-10-28 16:09:20.000000000 +0200
--- ./liboctave/lo-sysdep.h     2007-02-15 12:03:38.002391581 +0100
***************
*** 27,32 ****
--- 27,33 ----
  #include <string>
  
  #include "lo-ieee.h"
+ class string_vector;
  
  extern std::string octave_getcwd (void);
  
***************
*** 36,42 ****
  extern int gethostname (char *, int);
  #endif
  
! #if defined (_MSC_VER)
  
  // FIXME -- it would probably be better to adapt the versions of
  // opendir, readdir, and closedir from Emacs as they appear to be more
--- 37,48 ----
  extern int gethostname (char *, int);
  #endif
  
! #if defined (__WIN32__) && ! defined (__CYGWIN__)
! extern pid_t octave_popen2 (const std::string&, const string_vector&,
!     bool, int *, std::string&);
! #endif
! 
! #if defined (_MSC_VER) && ! defined (HAVE_DIRENT_H)
  
  // FIXME -- it would probably be better to adapt the versions of
  // opendir, readdir, and closedir from Emacs as they appear to be more
*** ./liboctave/oct-syscalls.cc.orig33  2006-10-28 16:09:20.000000000 +0200
--- ./liboctave/oct-syscalls.cc 2007-02-15 18:38:53.241304752 +0100
***************
*** 47,52 ****
--- 47,53 ----
  #include <signal.h>
  
  #include "lo-utils.h"
+ #include "lo-sysdep.h"
  #include "oct-syscalls.h"
  #include "str-vec.h"
  
***************
*** 356,361 ****
--- 357,456 ----
    return status;
  }
  
+ pid_t
+ octave_syscalls::popen2 (const std::string& cmd, const string_vector& args,
+     bool sync_mode, int *fildes)
+ {
+   std::string msg;
+   bool interactive = false;
+   return popen2 (cmd, args, sync_mode, fildes, msg, interactive);
+ }
+ 
+ pid_t
+ octave_syscalls::popen2 (const std::string& cmd, const string_vector& args,
+     bool sync_mode, int *fildes, std::string& msg)
+ {
+   bool interactive = false;
+   return popen2 (cmd, args, sync_mode, fildes, msg, interactive);
+ }
+ 
+ pid_t
+ octave_syscalls::popen2 (const std::string& cmd, const string_vector& args,
+     bool sync_mode, int *fildes, std::string& msg, bool &interactive)
+ {
+ #if defined (__WIN32__) && ! defined (__CYGWIN__)
+   return ::octave_popen2 (cmd, args, sync_mode, fildes, msg);
+ #else
+   pid_t pid;
+   int child_stdin[2], child_stdout[2];
+ 
+   if (pipe (child_stdin, msg) == 0)
+     {
+       if (pipe (child_stdout, msg) == 0)
+         {
+           pid = fork (msg);
+           if (pid < 0)
+             msg = "popen2: process creation failed -- " + msg;
+           else if (pid == 0)
+             {
+             std::string child_msg;
+ 
+             interactive = false;
+ 
+               // Child process
+               ::close (child_stdin[1]);
+               ::close (child_stdout[0]);
+ 
+               if (dup2 (child_stdin[0], STDIN_FILENO) >= 0)
+                 {
+                   ::close (child_stdin[0]);
+                   if (dup2 (child_stdout[1], STDOUT_FILENO) >= 0)
+                     {
+                       ::close (child_stdout[1]);
+                       if (execvp (cmd, args, child_msg) < 0)
+                         child_msg = "popen2 (child): unable to start process 
-- " + child_msg;
+                     }
+                   else
+                     child_msg = "popen2 (child): file handle duplication 
failed -- " + child_msg;
+                 }
+               else
+                 child_msg = "popen2 (child): file handle duplication failed 
-- " + child_msg;
+             
+             (*current_liboctave_error_handler)(child_msg.c_str());
+             
+             exit(0);
+             }
+           else
+             {
+               // Parent process
+               ::close (child_stdin[0]);
+               ::close (child_stdout[1]);
+ #if defined (F_SETFL) && defined (O_NONBLOCK)
+               if (! sync_mode && fcntl (child_stdout[0], F_SETFL, O_NONBLOCK, 
msg) < 0)
+                 msg = "popen2: error setting file mode -- " + msg;
+               else
+ #endif
+                 {
+                   fildes[0] = child_stdin [1];
+                   fildes[1] = child_stdout [0];
+                   return pid;
+                 }
+             }
+           ::close (child_stdout[0]);
+           ::close (child_stdout[1]);
+         }
+       else
+         msg = "popen2: pipe creation failed -- " + msg;
+       ::close (child_stdin[0]);
+       ::close (child_stdin[1]);
+     }
+   else
+     msg = "popen2: pipe creation failed -- " + msg;
+ 
+   return -1;
+ #endif
+ }
+ 
  /*
  ;;; Local Variables: ***
  ;;; mode: C++ ***
*** ./liboctave/oct-syscalls.h.orig33   2006-10-27 03:45:56.000000000 +0200
--- ./liboctave/oct-syscalls.h  2007-02-15 18:37:55.377049755 +0100
***************
*** 67,72 ****
--- 67,76 ----
  
    static int kill (pid_t, int);
    static int kill (pid_t, int, std::string&);
+ 
+   static pid_t popen2 (const std::string&, const string_vector&, bool, int *);
+   static pid_t popen2 (const std::string&, const string_vector&, bool, int *, 
std::string&);
+   static pid_t popen2 (const std::string&, const string_vector&, bool, int *, 
std::string&, bool &interactive);
  };
  
  #endif
*** ./scripts/miscellaneous/popen2.m.orig33     2006-10-10 18:10:27.000000000 
+0200
--- ./scripts/miscellaneous/popen2.m    2007-02-15 15:04:23.242558835 +0100
***************
*** 1,127 ****
- ## Copyright (C) 1996, 1997 John W. Eaton
- ##
- ## This file is part of Octave.
- ##
- ## Octave is free software; you can redistribute it and/or modify it
- ## under the terms of the GNU General Public License as published by
- ## the Free Software Foundation; either version 2, or (at your option)
- ## any later version.
- ##
- ## Octave is distributed in the hope that it will be useful, but
- ## WITHOUT ANY WARRANTY; without even the implied warranty of
- ## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- ## General Public License for more details.
- ##
- ## You should have received a copy of the GNU General Public License
- ## along with Octave; see the file COPYING.  If not, write to the Free
- ## Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
- ## 02110-1301, USA.
- 
- ## -*- texinfo -*-
- ## @deftypefn {Function File} address@hidden, @var{out}, @var{pid}] =} popen2 
(@var{command}, @var{args})
- ## Start a subprocess with two-way communication.  The name of the process
- ## is given by @var{command}, and @var{args} is an array of strings
- ## containing options for the command.  The file identifiers for the input
- ## and output streams of the subprocess are returned in @var{in} and
- ## @var{out}.  If execution of the command is successful, @var{pid}
- ## contains the process ID of the subprocess.  Otherwise, @var{pid} is
- ## @minus{}1.
- ##
- ## For example,
- ##
- ## @example
- ## @group
- ## [in, out, pid] = popen2 ("sort", "-nr");
- ## fputs (in, "these\nare\nsome\nstrings\n");
- ## fclose (in);
- ## EAGAIN = errno ("EAGAIN");
- ## done = false;
- ## do
- ##   s = fgets (out);
- ##   if (ischar (s))
- ##     fputs (stdout, s);
- ##   elseif (errno () == EAGAIN)
- ##     sleep (0.1);
- ##     fclear (out);
- ##   else
- ##     done = true;
- ##   endif
- ## until (done)
- ## fclose (out);
- ##      @print{} are
- ##      @print{} some
- ##      @print{} strings
- ##      @print{} these
- ## @end group
- ## @end example
- ## @end deftypefn
- 
- ## Author: jwe
- 
- function [in, out, pid] = popen2 (command, args)
- 
-   in = -1;
-   out = -1;
-   pid = -1;
- 
-   if (nargin == 1 || nargin == 2)
- 
-     if (nargin == 1)
-       args = "";
-     endif
- 
-     if (ischar (command))
- 
-       [stdin_pipe, stdin_status] = pipe ();
-       [stdout_pipe, stdout_status] = pipe ();
- 
-       if (stdin_status == 0 && stdout_status == 0)
- 
-         pid = fork ();
- 
-         if (pid == 0)
- 
-         ## In the child.
- 
-           fclose (nth (stdin_pipe, 2));
-           fclose (nth (stdout_pipe, 1));
- 
-           dup2 (nth (stdin_pipe, 1), stdin);
-           fclose (nth (stdin_pipe, 1));
- 
-           dup2 (nth (stdout_pipe, 2), stdout);
-           fclose (nth (stdout_pipe, 2));
- 
-           if (exec (command, args) < 0)
-             error ("popen2: unable to start process `%s'", command);
-             exit (0);
-           endif
- 
-         elseif (pid)
- 
-         ## In the parent.
- 
-           fclose (nth (stdin_pipe, 1));
-           fclose (nth (stdout_pipe, 2));
- 
-           if (fcntl (nth (stdout_pipe, 1), F_SETFL, O_NONBLOCK) < 0)
-             error ("popen2: error setting file mode");
-           else
-             in = nth (stdin_pipe, 2);
-             out = nth (stdout_pipe, 1);
-           endif
- 
-         elseif (pid < 0)
-           error ("popen2: fork failed -- unable to create child process");
-         endif
-       else
-         error ("popen2: pipe creation failed");
-       endif
-     else
-       error ("popen2: file name must be a string");
-     endif
-   else
-     print_usage ();
-   endif
- 
- endfunction
--- 0 ----
*** ./src/syscalls.cc.orig33    2006-06-30 20:19:21.000000000 +0200
--- ./src/syscalls.cc   2007-02-15 18:41:00.595298886 +0100
***************
*** 60,65 ****
--- 60,66 ----
  #include "sysdep.h"
  #include "utils.h"
  #include "variables.h"
+ #include "input.h"
  
  static Octave_map
  mk_stat_map (const file_stat& fs)
***************
*** 220,225 ****
--- 221,393 ----
    return retval;
  }
  
+ DEFUN (popen2, args, ,
+  "-*- texinfo -*-\n\
+ @deftypefn {Function File} address@hidden, @var{out}, @var{pid}] =} popen2 
(@var{command}, @var{args})\n\
+ Start a subprocess with two-way communication.  The name of the process\n\
+ is given by @var{command}, and @var{args} is an array of strings\n\
+ containing options for the command.  The file identifiers for the input\n\
+ and output streams of the subprocess are returned in @var{in} and\n\
+ @var{out}.  If execution of the command is successful, @var{pid}\n\
+ contains the process ID of the subprocess.  Otherwise, @var{pid} is\n\
+ @minus{}1.\n\
+ \n\
+ For example,\n\
+ \n\
+ @example\n\
+ @group\n\
+ [in, out, pid] = popen2 (\"sort\", \"-nr\");\n\
+ fputs (in, \"these\\nare\\nsome\\nstrings\\n\");\n\
+ fclose (in);\n\
+ EAGAIN = errno (\"EAGAIN\");\n\
+ done = false;\n\
+ do\n\
+   s = fgets (out);\n\
+   if (ischar (s))\n\
+     fputs (stdout, s);\n\
+   elseif (errno () == EAGAIN)\n\
+     sleep (0.1);\n\
+     fclear (out);\n\
+   else\n\
+     done = true;\n\
+   endif\n\
+ until (done)\n\
+ fclose (out);\n\
+      @print{} are\n\
+      @print{} some\n\
+      @print{} strings\n\
+      @print{} these\n\
+ @end group\n\
+ @end example\n\
+ @end deftypefn")
+ {
+   octave_value_list retval;
+ 
+   retval(2) = -1;
+   retval(1) = Matrix ();
+   retval(0) = Matrix ();
+ 
+   int nargin = args.length ();
+ 
+   if (nargin >= 1 && nargin <= 3)
+     {
+       std::string exec_file = args(0).string_value();
+ 
+       if (! error_state)
+         {
+         string_vector arg_list;
+ 
+         if (nargin >= 2)
+           {
+             string_vector tmp = args(1).all_strings ();
+ 
+             if (! error_state)
+               {
+                 int len = tmp.length ();
+ 
+                 arg_list.resize (len + 1);
+ 
+                 arg_list[0] = exec_file;
+ 
+                 for (int i = 0; i < len; i++)
+                   arg_list[i+1] = tmp[i];
+               }
+             else
+               error ("popen2: arguments must be character strings");
+           }
+         else
+           {
+             arg_list.resize (1);
+ 
+             arg_list[0] = exec_file;
+           }
+ 
+           if (! error_state)
+             {
+               bool sync_mode = (nargin == 3 ? args(2).bool_value() : false);
+ 
+               if (! error_state)
+                 {
+                   int fildes[2];
+                   std::string msg;
+                   pid_t pid;
+ 
+                   pid = octave_syscalls::popen2 (exec_file, arg_list, 
sync_mode, fildes, msg, interactive);
+                   if (pid >= 0)
+                     {
+                       FILE *ifile = fdopen (fildes[1], "r");
+                       FILE *ofile = fdopen (fildes[0], "w");
+ 
+                       std::string nm;
+ 
+                       octave_stream is = octave_stdiostream::create (nm, 
ifile,
+                           std::ios::in);
+ 
+                       octave_stream os = octave_stdiostream::create (nm, 
ofile,
+                           std::ios::out);
+ 
+                       Cell file_ids (1, 2);
+ 
+                       retval(0) = octave_stream_list::insert (os);
+                       retval(1) = octave_stream_list::insert (is);
+                                         retval(2) = pid;
+                     }
+                                 else
+                     error (msg.c_str ());
+                 }
+             }
+           else
+             error ("popen2: arguments must be character strings");
+         }
+       else
+         error ("popen2: first argument must be a string");
+     }
+   else
+     print_usage ();
+ 
+   return retval;
+ }
+ 
+ /*
+ 
+ %!test
+ %!  if (isunix())
+ %!    [in, out, pid] = popen2 ("sort", "-nr");
+ %!    fputs (in, "these\nare\nsome\nstrings\n");
+ %!    EAGAIN = errno ("EAGAIN");
+ %!  else
+ %!    [in, out, pid] = popen2 ("sort", "/R", 1);
+ %!    fputs (in, "these\\nare\\nsome\\nstrings\\n");
+ %!    EAGAIN = errno ("EINVAL");
+ %!  endif
+ %!  fclose (in);
+ %!  done = false;
+ %!  str = {};
+ %!  idx = 0;
+ %!  do
+ %!     if (!isunix())
+ %!       errno (0);
+ %!     endif
+ %!     s = fgets (out);
+ %!     if (ischar (s))
+ %!       idx++;
+ %!       str{idx} = s;
+ %!     elseif (errno () == EAGAIN)
+ %!       sleep (0.1);
+ %!       fclear (out);
+ %!     else
+ %!       done = true;
+ %!     endif
+ %!   until (done)
+ %!  fclose (out);
+ %!  if (isunix())
+ %!    assert(str,{"these\n","strings\n","some\n","are\n"})
+ %!  else
+ %!    assert(str,{"these\\r\\n","strings\\r\\n","some\\r\\n","are\\r\\n"})
+ %!  endif
+ 
+ */
+ 
  DEFUN (fcntl, args, ,
   "-*- texinfo -*-\n\
  @deftypefn {Built-in Function} address@hidden, @var{msg}] =} fcntl 
(@var{fid}, @var{request}, @var{arg})\n\
***************
*** 644,652 ****
  
  DEFUN (pipe, args, ,
    "-*- texinfo -*-\n\
! @deftypefn {Built-in Function} address@hidden, @var{err}, @var{msg}] =} pipe 
()\n\
! Create a pipe and return the vector @var{file_ids}, which corresponding\n\
! to the reading and writing ends of the pipe.\n\
  \n\
  If successful, @var{err} is 0 and @var{msg} is an empty string.\n\
  Otherwise, @var{err} is nonzero and @var{msg} contains a\n\
--- 812,820 ----
  
  DEFUN (pipe, args, ,
    "-*- texinfo -*-\n\
! @deftypefn {Built-in Function} address@hidden, @var{write_fd}, @var{err}, 
@var{msg}] =} pipe ()\n\
! Create a pipe and return the reading and writing ends of the pipe\n\
! into @var{read_fd} and @var{write_fd} respectively.\n\
  \n\
  If successful, @var{err} is 0 and @var{msg} is an empty string.\n\
  Otherwise, @var{err} is nonzero and @var{msg} contains a\n\
***************
*** 655,663 ****
  {
    octave_value_list retval;
  
!   retval(2) = std::string ();
    retval(1) = -1;
!   retval(0) = Matrix ();
  
    int nargin = args.length ();
  
--- 823,832 ----
  {
    octave_value_list retval;
  
!   retval(3) = std::string ();
!   retval(2) = -1;
    retval(1) = -1;
!   retval(0) = -1;
  
    int nargin = args.length ();
  
***************
*** 670,676 ****
        int status = octave_syscalls::pipe (fid, msg);
  
        if (status < 0)
!       retval(2) = msg;
        else
        {
          FILE *ifile = fdopen (fid[0], "r");
--- 839,845 ----
        int status = octave_syscalls::pipe (fid, msg);
  
        if (status < 0)
!       retval(3) = msg;
        else
        {
          FILE *ifile = fdopen (fid[0], "r");
***************
*** 684,696 ****
          octave_stream os = octave_stdiostream::create (nm, ofile,
                                                         std::ios::out);
  
!         octave_value_list file_ids;
! 
!         file_ids(1) = octave_stream_list::insert (os);
!         file_ids(0) = octave_stream_list::insert (is);
  
!         retval(1) = status;
!           retval(0) = octave_value (file_ids);
        }
      }
    else
--- 853,862 ----
          octave_stream os = octave_stdiostream::create (nm, ofile,
                                                         std::ios::out);
  
!         retval(1) = octave_stream_list::insert (os);
!         retval(0) = octave_stream_list::insert (is);
  
!         retval(2) = status;
        }
      }
    else