> ===================================================================
> --- 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.
> + 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.
> + 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.
> +
> + /* 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). Do you really need to close it early?
...
> + /* 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).
> + 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.
...
> + /* 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.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Mar 30 22:02:07 2006