"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.
_____________________________________________________________________________
---------------------------------------------------------------------
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:33:42 2006