[prev in list] [next in list] [prev in thread] [next in thread] 

List:       subversion-dev
Subject:    Re: [PATCH] Was: Re: r19004
From:       "Peter N. Lundblad" <peter () famlundblad ! se>
Date:       2006-03-30 20:01:34
Message-ID: 17452.14622.376221.472211 () localhost ! localdomain
[Download RAW message or body]

> ===================================================================
> --- 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

[prev in list] [next in list] [prev in thread] [next in thread]