Paul,
Today it occurred to me: since this is specific to a particular platform,
wouldn't it be possible to just use apr_os_file_get() (see apr_portable.h) to
avoid this poll mess altogether? Just duplicate the stdin file handle to
the stdin descriptor of the child process.
(I was looking for this "get the native file handle" function before,
but didn't find it because I was looking in the wrong place. Sorry for
that:-()
(If that doesn't work for some reason, I'll have a look at your new patch.)
Thanks,
//Peter
Paul Burba writes:
> --=_mixed 004A71CB85257145_=
> Content-Type: text/plain; charset="US-ASCII"
>
> "Peter N. Lundblad" <peter@famlundblad.se> wrote on 03/30/2006 03:01:34
> PM:
>
> > > ===================================================================
> > > --- subversion/libsvn_repos/hooks.c (revision 19091)
> > > +++ subversion/libsvn_repos/hooks.c (working copy)
> > ...
> > > + while (1)
> > > {
> > > - while (1)
> > > + apr_size_t bytes_read = sizeof(stdin_buffer);
> > > + int wc;
> > > +
> > > + svn_error_t *err;
> > > +
> > > + /* Blocking poll until we are ready to write some stdin to the
> script
> > > + * or read it's stderr. */
> > > + if (poll(pfds, 2, -1) == -1)
> > > {
> > > - rc = read(stderr_pipe[0], buffer, sizeof(buffer));
> > > - if (rc == -1)
> > > + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> > > + "Error polling stderr/stdin of
> hook "
> > > + "script '%s'", cmd);
> > > + }
> > > +
> > > + /* Immediate poll to check if we can read stderr from the
> script. */
> > > + if (poll((struct pollfd *)(&pfds[0]), 1, 0) == -1)
> > > + {
> > > + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> > > + "Error polling stderr of hook "
> > > + "script '%s'", cmd);
> > > + }
> > > +
> >
> > What's the point of this? Shouldn't the POLLIN/POLLOUT flags in
> > revents be enough to know what you can read/write? I think one poll
> > should be enough for this whole loop.
>
> In hindsight there is no point...I hadn't worked with poll() before and
> got caught in the trap of solving one problem, then another, then another,
> etc., without realizing subsequent solutions negated the need for earlier
> ones. Anyway, rather than run through my misconceptions, I'll leave it at
> you're right, one poll is adequate.
>
> > > + if (read_errstream && more_stderr && pfds[0].revents & POLLIN)
> > > + {
> > > + do {
> > > + int rc = read(stderr_pipe[0], stderr_buffer,
> > > + sizeof(stderr_buffer));
> >
> > Why this nested loop? Even if you get a short read here, you would
> > get
> > back here after the next poll in the main loop.
>
> Ditto.
>
> > > + if (rc == -1)
> > > + {
> > > + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM,
> NULL,
> > > + "Error reading stderr of
> hook "
> > > + "script '%s'", cmd);
> > > + }
> > > +
> > > + svn_stringbuf_appendbytes(script_output, stderr_buffer,
> rc);
> >
> > Another technique would be to just svn_stringbuf_ensure the stringbuf
> > before
> > the read and reading directly into the stringbuf, and after that
> > increasing the length by the actual amount of data read. That's a
> > minor simplification, though.
>
> Nice idea, did that.
>
> > > +
> > > + /* If read() returned 0 then EOF was found. */
> > > + if (rc == 0)
> > > + {
> > > + /* Close the read end of the stderr pipe. */
> > > + if (close(stderr_pipe[0]) == -1)
> > > + return
> > svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> > > + "Error closing read end
> of "
> > > + "stderr pipe to hook
> script "
> > > + "'%s'", cmd);
> > > + more_stderr = FALSE;
> >
> > Hmmm, so next time, you'll poll a closed file descriptor (or a
> > descriptor opened by another thread)
>
> Hadn't thought of that latter case...
>
> >. Do you really need to close it early?
>
> ...and it doesn't appear so, moved the close till after the do...while
> loop.
>
> > ...
> > > + /* Immediate poll to check if we can write stdin to the script.
> */
> > > + if (poll((struct pollfd *)(&pfds[1]), 1, 0) == -1)
> > > + {
> > > + return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> > > + "Error polling stdin of hook "
> > > + "script '%s'", cmd);
> > > + }
> > > +
> >
> > Same question applies here (about why so many polls).
>
> That's gone now too.
>
> > > + if (stdin_handle && pfds[1].revents & POLLOUT)
> > > + {
> > > + /* Don't lose any stdin: Use what we last read into the
> buffer
> > > + * if the previous write to stdin pipe failed. */
> > > + if (last_stdin_write_succeeded)
> > > {
> > > - return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM,
> NULL,
> > > - "Error reading stderr of hook
> "
> > > - "script '%s'", cmd);
> > > + err = svn_io_file_read(stdin_handle, stdin_buffer,
> > > + &bytes_read, pool);
> > > + bytes_last_read = bytes_read;
> > > }
> > > + if (err)
> > > + {
> > > + if (APR_STATUS_IS_EOF(err->apr_err))
> > > + {
> > > + /* Make use of stdin_handle as flag to detect when
> we are
> > > + * done writing stdin to the script. */
> > > + stdin_handle = NULL;
> > >
> > > - svn_stringbuf_appendbytes(script_output, buffer, rc);
> > > -
> > > - /* If read() returned 0 then EOF was found. */
> > > - if (rc == 0)
> > > - {
> > > - /* Close the read end of the stderr pipe. */
> > > - if (close(stderr_pipe[0]) == -1)
> > > + /* If there is no more stdin close the write end of
> the
> > > + * stdin pipe. */
> > > + if (stdin_handle == NULL && close(stdin_pipe[1]) ==
> -1)
> > > + return
> > svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> > > + "Error closing write end
> of "
> > > + "stdin pipe to hook
> script "
> > > + "'%s'", cmd);
> >
> > This needs to be closed, though. But I think it is wrong to continue
> > polling after it was closed.
>
> Looks like you are commenting on the same code twice, did you mean this
> comment to apply to the closure of stdin_pipe[1]? If so, then I think
> pipe must be closed within the do...while loop. If I wait until after the
> loop to close it, the script hangs while attempting to read stdin. I'm
> not sure how I signal to the script that there is no more stdin for it to
> read other than closing Subversion's end of the pipe.
>
> Regardless, this still leaves your concern about polling a closed file
> descriptor. To avoid this I now set pfds[1].fd to -1 when closing the
> stdin pipe. IBM suggests using this value in their examples to
> effectively switch off polling for a particular file descriptor. The only
> potential problem I see is calling poll() when both file descriptors are
> -1 since it blocks. I changed the do...while loop to a while loop to
> avoid this.
>
> One other thing, since stderr_pipe[0] and/or stdin_pipe[1] are undefined
> if read_errstrem and/or stdin_handle are FALSE/NULL, I changed the
> initialization of the pfds array:
>
> pfds[0].fd = read_errstream ? stderr_pipe[0] : -1;
> pfds[0].events = POLLIN;
> pfds[1].fd = stdin_handle != NULL ? stdin_pipe[1] : -1;
> pfds[1].events = POLLOUT;
>
> > ...
> > > + /* Are we done writing to stdin pipe and reading from stderr
> pipe? */
> > > + if (!more_stderr && stdin_handle == NULL)
> > > + break;
> > > + } /* while(1) */
> >
> > You could change this to a do ... while() loop, making the condition a
> > part of the loop condition.
> >
> > Unless I'm missing something, it looks like this could be made a
> > little less complex as suggested above.
>
> A lot less complex is more like it. Thanks for all the help, please let
> me know if you see any other problems.
>
> Paul B.
>
>
> _____________________________________________________________________________
> Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
> _____________________________________________________________________________
> --=_mixed 004A71CB85257145_=
> Content-Type: text/plain; name="hooks.multiplexing.diff.ver4.txt"
> Content-Disposition: attachment; filename="hooks.multiplexing.diff.ver4.txt"
> Content-Transfer-Encoding: quoted-printable
>
> Index: subversion/libsvn=5Frepos/hooks.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- subversion/libsvn=5Frepos/hooks.c (revision 19130)
> +++ subversion/libsvn=5Frepos/hooks.c (working copy)
> @@ -25,6 +25,9 @@
> #ifdef AS400=0A #include <spawn.h>=0A #include <fcntl.h>=0A+#include <sys/=
> poll.h>=0A+#include <sys/types.h>=0A+#include <unistd.h>=0A #endif=0A =0A #=
> include "svn=5Ferror.h"=0A@@ -171,11 +174,12 @@
> return err;=0A }=0A #else /* Run hooks with spawn() on OS400. */=0A+#def=
> ine AS400=5FBUFFER=5FSIZE 128=0A {=0A const char *script=5Fstderr=5Futf8 =
> =3D "";=0A const char **native=5Fargs;=0A- char buffer[20];=0A- int rc,=
> fd=5Fmap[3], stderr=5Fpipe[2], stdin=5Fpipe[2], exitcode;=0A+ char stdin=
> =5Fbuffer[AS400=5FBUFFER=5FSIZE];=0A+ int fd=5Fmap[3], stderr=5Fpipe[2], s=
> tdin=5Fpipe[2], exitcode;=0A svn=5Fstringbuf=5Ft *script=5Foutput =3D svn=
> =5Fstringbuf=5Fcreate("", pool);=0A pid=5Ft child=5Fpid, wait=5Frv;=0A =
> apr=5Fsize=5Ft args=5Farr=5Fsize =3D 0, i;=0A@@ -186,6 +190,11 @@
> char *xmp=5Fenvp[2] =3D {"QIBM=5FUSE=5FDESCRIPTOR=5FSTDIO=3DY", NULL};=
> =0A const char *dev=5Fnull=5Febcdic =3D SVN=5FNULL=5FDEVICE=5FNAME;=0A #p=
> ragma convert(1208)=0A+ svn=5Fboolean=5Ft more=5Fstderr =3D read=5Ferrstre=
> am;=0A+ svn=5Fboolean=5Ft more=5Fstdin =3D stdin=5Fhandle ? TRUE : FALSE;=
> =0A+ svn=5Fboolean=5Ft last=5Fstdin=5Fwrite=5Fsucceeded =3D TRUE;=0A+ apr=
> =5Fsize=5Ft bytes=5Flast=5Fread;=0A+ struct pollfd pfds[2];=0A =0A /* Fi=
> nd number of elements in args array. */=0A while (args[args=5Farr=5Fsize]=
> !=3D NULL)=0A@@ -222,6 +231,14 @@
> cmd);=0A }=0A fd=5Fmap[0]=
> =3D stdin=5Fpipe[0];=0A+=0A+ /* Prevent blocking on write end of stdi=
> n pipe. */=0A+ if (fcntl(stdin=5Fpipe[1], F=5FSETFL, O=5FNONBLOCK) =3D=
> =3D -1)=0A+ {=0A+ return svn=5Ferror=5Fcreatef(SVN=5FERR=5F=
> EXTERNAL=5FPROGRAM, NULL,=0A+ "Can't set =
> flag for stdin pipe for "=0A+ "hook '%s'"=
> , cmd);=0A+ }=0A }=0A else=0A {=0A@@ -273,38 +290,6 @@
> cmd);=0A }=0A =0A- /* If there is "APR=
> stdin", read it and then write it to the hook's=0A- * stdin pipe. */=0A-=
> if (stdin=5Fhandle)=0A- {=0A- while (1)=0A- {=0A- =
> apr=5Fsize=5Ft bytes=5Fread =3D sizeof(buffer);=0A- int wc;=0A- =
> svn=5Ferror=5Ft *err =3D svn=5Fio=5Ffile=5Fread(stdin=5Fhandle, buf=
> fer,=0A- &bytes=5Fread, pool);=
> =0A- if (err && APR=5FSTATUS=5FIS=5FEOF(err->apr=5Ferr))=0A- =
> break;=0A-=0A- if (err)=0A- return svn=5Ferror=5F=
> createf(SVN=5FERR=5FEXTERNAL=5FPROGRAM, NULL,=0A- =
> "Error piping stdin to hook "=0A- =
> "script '%s'", cmd);=0A-=0A- wc =3D write(stdin=5Fpipe[1], b=
> uffer, bytes=5Fread);=0A- if (wc =3D=3D -1) =0A- re=
> turn svn=5Ferror=5Fcreatef(SVN=5FERR=5FEXTERNAL=5FPROGRAM, NULL,=0A- =
> "Error piping stdin to hook "=0A- =
> "script '%s'", cmd);=0A- }=0A-=0A- /* =
> Close the write end of the stdin pipe. */=0A- if (close(stdin=5Fpipe[1=
> ]) =3D=3D -1)=0A- return svn=5Ferror=5Fcreatef(SVN=5FERR=5FEXTERNAL=
> =5FPROGRAM, NULL,=0A- "Error closing write =
> end of stdin pipe "=0A- "to hook script '%s=
> '", cmd);=0A- }=0A-=0A /* Close the stdout file descriptor. */=0A if=
> (close(fd=5Fmap[1]) =3D=3D -1)=0A return svn=5Ferror=5Fcreatef(SVN=5FE=
> RR=5FEXTERNAL=5FPROGRAM, NULL,=0A@@ -317,13 +302,49 @@
> return svn=5Ferror=5Fcreatef(SVN=5FERR=5FEXTERNAL=5FPROGRAM, NULL,=0A =
> "Error closing write end of stderr pipe to "=
> =0A "hook script '%s'", cmd);=0A+ =
> =0A+ /* Close read end of stdin pipe so we don't hang on t=
> he write. */=0A+ if (close(fd=5Fmap[0]) =3D=3D -1)=0A+ return svn=5Ferr=
> or=5Fcreatef(SVN=5FERR=5FEXTERNAL=5FPROGRAM, NULL,=0A+ =
> "Error closing read end of stdin pipe to "=0A+ =
> "hook script '%s'", cmd);=0A =0A- /* Read the hook's stderr if we =
> care about that. */=0A- if (read=5Ferrstream)=0A+ /* Specify the events w=
> e want to poll:=0A+ * =0A+ * 1) Can read from the script's stderr pipe=
> =0A+ * 2) Can write to the script's stdin pipe=0A+ */=0A+ pfds[0].fd =
> =3D read=5Ferrstream ? stderr=5Fpipe[0] : -1;=0A+ pfds[0].events =3D POLLI=
> N;=0A+ pfds[1].fd =3D stdin=5Fhandle !=3D NULL ? stdin=5Fpipe[1] : -1;=0A+=
> pfds[1].events =3D POLLOUT;=0A+ =0A+ while (more=5Fstderr || more=
> =5Fstdin)=0A {=0A- while (1)=0A+ apr=5Fsize=5Ft bytes=5Fread =
> =3D sizeof(stdin=5Fbuffer);=0A+ int wc;=0A+ svn=5Ferror=5Ft *err;=
> =0A+ =0A+ /* Blocking poll until we are ready to write some stdin=
> to the script=0A+ * or read it's stderr. */=0A+ if (poll(pfds, =
> 2, -1) =3D=3D -1)=0A {=0A- rc =3D read(stderr=5Fpipe[0], b=
> uffer, sizeof(buffer));=0A+ return svn=5Ferror=5Fcreatef(SVN=5FERR=
> =5FEXTERNAL=5FPROGRAM, NULL,=0A+ "Error p=
> olling stderr/stdin of hook "=0A+ "script=
> '%s'", cmd);=0A+ }=0A+=0A+ if (read=5Ferrstream && more=5Fstde=
> rr && pfds[0].revents & POLLIN)=0A+ {=0A+ int rc;=0A+=0A+ =
> svn=5Fstringbuf=5Fensure(script=5Foutput,=0A+ =
> script=5Foutput->len + AS400=5FBUFFER=5FSIZE + 1);=0A+=0A+ =
> rc =3D read(stderr=5Fpipe[0],=0A+ &(script=5Foutput->d=
> ata[script=5Foutput->len]),=0A+ AS400=5FBUFFER=5FSIZE);=
> =0A+=0A if (rc =3D=3D -1)=0A {=0A retur=
> n svn=5Ferror=5Fcreatef(SVN=5FERR=5FEXTERNAL=5FPROGRAM, NULL,=0A@@ -331,21 =
> +352,84 @@
> "script '%s'", cmd);=0A =
> }=0A =0A- svn=5Fstringbuf=5Fappendbytes(script=5Foutput, buffer, =
> rc);=0A+ script=5Foutput->len +=3D rc;=0A =0A- /* If read=
> () returned 0 then EOF was found. */=0A if (rc =3D=3D 0)=0A =
> {=0A- /* Close the read end of the stderr pipe. */=0A- =
> if (close(stderr=5Fpipe[0]) =3D=3D -1)=0A+ /* If =
> read() returned 0 then EOF was found and we are done=0A+ * re=
> ading stderr. */=0A+ more=5Fstderr =3D FALSE;=0A+ }=
> =0A+ }=0A+=0A+ if (more=5Fstdin && pfds[1].revents & POLLOUT)=
> =0A+ {=0A+ /* Don't lose any stdin: Use what we last read i=
> nto the buffer=0A+ * if the previous write to stdin pipe failed. =
> */=0A+ if (last=5Fstdin=5Fwrite=5Fsucceeded)=0A+ {=0A+ =
> err =3D svn=5Fio=5Ffile=5Fread(stdin=5Fhandle, stdin=5Fbuffer,=
> =0A+ &bytes=5Fread, pool);=0A+ =
> bytes=5Flast=5Fread =3D bytes=5Fread;=0A+ }=0A+ if=
> (err)=0A+ {=0A+ if (APR=5FSTATUS=5FIS=5FEOF(err->a=
> pr=5Ferr))=0A+ {=0A+ /* Done writing stdin =
> to the script. */=0A+ more=5Fstdin =3D FALSE;=0A+=0A+ =
> /* Don't poll stdin pipe anymore. */=0A+ pfds=
> [1].fd =3D -1;=0A+=0A+ /* Close the write end of the stder=
> r pipe. */=0A+ if (close(stdin=5Fpipe[1]) =3D=3D -1)=0A+ =
> return svn=5Ferror=5Fcreatef(SVN=5FERR=5FEXTERNAL=5FPROGR=
> AM, NULL,=0A+ "Error closing wr=
> ite end of "=0A+ "stdin pipe to=
> hook script "=0A+ "'%s'", cmd)=
> ;=0A+ }=0A+ else=0A return svn=
> =5Ferror=5Fcreatef(SVN=5FERR=5FEXTERNAL=5FPROGRAM, NULL,=0A- =
> "Error closing read end of stderr "=0A- =
> "pipe to hook script '%s'", cmd);=0A- =
> break;=0A+ "Error piping std=
> in to hook "=0A+ "script '%s'", cmd=
> );=0A }=0A+ else=0A+ {=0A+ wc =
> =3D write(stdin=5Fpipe[1], stdin=5Fbuffer, bytes=5Flast=5Fread);=0A+ =
> if (wc =3D=3D -1)=0A+ {=0A+ if (errn=
> o =3D=3D EWOULDBLOCK)=0A+ {=0A+ las=
> t=5Fstdin=5Fwrite=5Fsucceeded =3D FALSE;=0A+ }=0A+ =
> else if (errno =3D=3D EPIPE)=0A+ {=0A+ =
> /* If the script exits without ever trying to read=0A+ =
> * stdin we'll get a broken pipe error. */=0A+ =
> more=5Fstdin =3D FALSE;=0A+ }=0A+ =
> else=0A+ {=0A+ return svn=5Fer=
> ror=5Fcreatef(SVN=5FERR=5FEXTERNAL=5FPROGRAM,=0A+ =
> NULL, "Error piping stdin "=0A+ =
> "to hook script '%s'", cmd);=0A+ =
> }=0A+ }=0A+ else=0A+ {=0A+ =
> last=5Fstdin=5Fwrite=5Fsucceeded =3D TRUE;=0A+ =
> }=0A+ }=0A }=0A- }=0A+ } /* while (more=5Fstderr |=
> | more=5Fstdin) */=0A =0A+ /* Close the read end of the stderr pipe. */=0A=
> + if (read=5Ferrstream && close(stderr=5Fpipe[0]) =3D=3D -1)=0A+ return=
> svn=5Ferror=5Fcreatef(SVN=5FERR=5FEXTERNAL=5FPROGRAM, NULL,=0A+ =
> "Error closing read end of stderr "=0A+ =
> "pipe to hook script '%s'", cmd);=0A+=0A /* Wait for the child=
> process to complete. */=0A wait=5Frv =3D waitpid(child=5Fpid, &exitcode,=
> 0);=0A if (wait=5Frv =3D=3D -1)=0A@@ -355,12 +439,6 @@
> "hook script '%s'", cmd);=0A }=0A =0A- =
> /* Close read end of stdin pipe if it exists. */=0A- if (close(fd=5Fmap[0=
> ]) =3D=3D -1)=0A- return svn=5Ferror=5Fcreatef(SVN=5FERR=5FEXTERNAL=5FPR=
> OGRAM, NULL,=0A- "Error closing read end of std=
> in pipe to hook "=0A- "script '%s'", cmd);=0A-=
> =0A if (!svn=5Fstringbuf=5Fisempty(script=5Foutput))=0A {=0A /*=
> OS400 scripts produce EBCDIC stderr, so convert it. */=0A=
> --=_mixed 004A71CB85257145_=--
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Apr 3 15:46:54 2006