Hi Rutger!
Rutger van Beusekom <rutger.van.beuse...@verum.com> skribis:
> This patch replaces open-process with piped-process in posix.c and
> reimplement open-process with piped-process in popen.scm. This allows
> setting up a pipeline in guile scheme using the new pipeline procedure
> in popen.scm and enables its use on operating systems which happen to
> lack the capability to fork, but do offer the capability captured by
> start_child (see posix.c).
Nice! That’s definitely very useful to have. We’ll need to check what
Andy thinks, but I think it can be added in the 3.0 series.
> From 3c8f5d534419418234cfe7d3eda8227951bc208a Mon Sep 17 00:00:00 2001
> From: Rutger van Beusekom <rutger.van.beuse...@verum.com>
> Date: Mon, 2 Mar 2020 10:38:57 +0100
> Subject: [PATCH] Allow client code to create pipe pairs when opening a
> process.
>
> * libguile/posix.c (scm_piped_process): Replace open_process by piped_process.
Could you mention functions renamed/removed here? The ChangeLog format
is about boringly listing all the language-entity-level changes. :-)
> * module/ice-9/popen.scm (pipe->fdes): Convert pipe pair to fdes pair.
> (open-process): Implement open-process with piped-process.
> (pipeline): Implement a pipeline with piped-process.
> static SCM
> -scm_open_process (SCM mode, SCM prog, SCM args)
> -#define FUNC_NAME "open-process"
> +scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
> +#define FUNC_NAME "piped-process"
> +/* SCM_DEFINE (scm_piped_process, "piped-process", 2, 2, 0, */
> +/* (SCM prog, SCM args, SCM from, SCM to), */
> +/* "Execute the command indicated by @var{prog} with arguments
> @var(args),\n" */
> +/* "optionally connected by an input and an output pipe.\n" */
> +/* "@var(from) and @var(to) are either #f or a valid file descriptor\n" */
> +/* "of an input and an output pipe, respectively.\n" */
> +/* "\n" */
> +/* "This function returns the PID of the process executing @var(prog)." */
> +/* "\n" */
> +/* "Example:\n" */
> +/* "(let ((p (pipe)))\n" */
> +/* " (piped-process \"echo\" '(\"foo\" \"bar\")\n" */
> +/* " (cons (port->fdes (car p))\n" */
> +/* " (port->fdes (cdr p))))\n" */
> +/* " (display (read-string (car p))))\n" */
> +/* "(let ((p (pipe)))\n" */
> +/* " (read-string (piped-process \"echo\" '(\"foo\" \"bar\")\n" */
> +/* " (port->fdes (car p)))))\n") */
> +/* #define FUNC_NAME scm_piped_process */
I guess you can remove the commented-out bits…
> - int c2p[2]; /* Child to parent. */
> - int p2c[2]; /* Parent to child. */
> + int c2p[2] = {}; /* Child to parent. */
> + int p2c[2] = {}; /* Parent to child. */
… and this hunk, to minimize change.
> +++ b/module/ice-9/popen.scm
> @@ -22,9 +22,10 @@
> #:use-module (rnrs bytevectors)
> #:use-module (ice-9 binary-ports)
> #:use-module (ice-9 threads)
> + #:use-module (srfi srfi-1)
> #:use-module (srfi srfi-9)
> #:export (port/pid-table open-pipe* open-pipe close-pipe open-input-pipe
> - open-output-pipe open-input-output-pipe))
> + open-output-pipe open-input-output-pipe pipe->fdes piped-process
> pipeline))
I would not export ‘pipe->fdes’. I’m not sure about exporting
‘piped-process’: it’s a bit low-level and we might want to reserve
ourselves the possibility to change it, like this patch does actually.
WDYT?
> +(define (open-process mode command . args)
> + (let* ((from (and (or (equal? mode OPEN_READ) (equal? mode OPEN_BOTH))
> (pipe->fdes)))
> + (to (and (or (equal? mode OPEN_WRITE) (equal? mode OPEN_BOTH))
> (pipe->fdes)))
> + (pid (piped-process command args from to)))
> + (values (and from (fdes->inport (car from))) (and to (fdes->outport (cdr
> to))) pid)))
Please wrap lines to 80 chars.
I suggest using ‘string=?’ above instead of ‘equal?’. Also, could you
add a docstring?
> +(define (pipeline procs)
> + "Execute a pipeline of @code(procs) -- where a proc is a list of a
> +command and its arguments as strings -- returning an input port to the
> +end of the pipeline, an output port to the beginning of the pipeline and
> +a list of PIDs of the @code(procs)"
Perhaps s/procs/commands/ would be clearer? Also, @var{commands}
instead of @code.
Could you also add an entry in doc/ref/*.texi, in the “Pipes” node,
perhaps with one of the examples you gave?
> +++ b/test-suite/tests/popen.test
> @@ -211,3 +211,37 @@ exec 2>~a; read REPLY"
> (let ((st (close-pipe (open-output-pipe "exit 1"))))
> (and (status:exit-val st)
> (= 1 (status:exit-val st)))))))
> +
> +
> +;;
> +;; pipeline related tests
> +;;
> +
> +(use-modules (ice-9 receive))
> +(use-modules (ice-9 rdelim))
Please move these to the top-level ‘define-module’ form.
One last thing: we’d need you to assign copyright to the FSF for this.
We can discuss it off-line if you want.
Thank you for this great and long overdue addition!
Ludo’.
