Re: svn commit: r22238 - trunk/subversion/libsvn_ra_svn

On Wed, Nov 08, 2006 at 03:07:54PM -0800, rooneg@tigris.org wrote:
> Encapsulate ra_svn's I/O with a stream-based wrapper. This will
> facilitate the introduction of SASL and TLS encryption.
>
> ==============================================================================
> --- trunk/subversion/libsvn_ra_svn/ra_svn.h (original)
> +++ trunk/subversion/libsvn_ra_svn/ra_svn.h Wed Nov 8 15:07:54 2006
> @@ -30,6 +30,12 @@
> #include <apr_thread_proc.h>
> #include "svn_ra_svn.h"
>
> +typedef svn_boolean_t (*ra_svn_pending_fn_t)(void *baton);
> +
> +typedef void (*ra_svn_timeout_fn_t)(void *baton, apr_interval_time_t timeout);
> +
> +typedef struct svn_ra_svn__stream_st svn_ra_svn__stream_t;
> +

Comments?

> /* Handler for blocked writes. */
> typedef svn_error_t *(*ra_svn_block_handler_t)(svn_ra_svn_conn_t *conn,
> apr_pool_t *pool,
> @@ -42,10 +48,13 @@
> /* This structure is opaque to the server. The client pokes at the
> * first few fields during setup and cleanup. */
> struct svn_ra_svn_conn_st {
> - apr_socket_t *sock; /* NULL if using in_file/out_file */
> - apr_file_t *in_file;
> - apr_file_t *out_file;
> - apr_proc_t *proc; /* Used by client.c when sock is NULL */
> + svn_ra_svn__stream_t *stream;
> +#ifdef SVN_HAVE_SASL
> + /* Although all reads and writes go through the svn_ra_svn__stream_t
> + interface, SASL still needs direct access to the underlying socket
> + for stuff like IP addresses and port numbers. */
> + apr_socket_t *sock;
> +#endif
> char read_buf[SVN_RA_SVN__READBUF_SIZE];
> char *read_ptr;
> char *read_end;

This is a public structure, isn't it? Are there any
backward-compatibility considerations we need to worry about?

> +/* Returns a stream that reads/writes from/to SOCK. */
> +svn_ra_svn__stream_t *svn_ra_svn__stream_from_sock(apr_socket_t *sock,
> + apr_pool_t *pool);
> +
> +/* Returns a stream that reads from IN_FILE and writes to OUT_FILE. */
> +svn_ra_svn__stream_t *svn_ra_svn__stream_from_files(apr_file_t *in_file,
> + apr_file_t *out_file,
> + apr_pool_t *pool);
> +
> +svn_ra_svn__stream_t *svn_ra_svn__stream_create(void *baton,
> + svn_read_fn_t read_cb,
> + svn_write_fn_t write_cb,
> + ra_svn_timeout_fn_t timeout_cb,
> + ra_svn_pending_fn_t pending_cb,
> + apr_pool_t *pool);
> +
> +svn_error_t *svn_ra_svn__stream_write(svn_ra_svn__stream_t *stream,
> + const char *data, apr_size_t *len);
> +
> +svn_error_t *svn_ra_svn__stream_read(svn_ra_svn__stream_t *stream,
> + char *data, apr_size_t *len);
> +

Those three need comments. It might also be a good idea to explain here
vaguely why the ra_svn stream type needs to differ from the general
svn_stream_t type. I also wonder whether we really need to duplicate
the base stream functionality, or whether we can create a svn_stream_t*
from which we can get the underlying ra_svn-specific stuff.

> +static svn_boolean_t pending(apr_pollfd_t *pfd, apr_pool_t *pool)
> +static svn_error_t *
> +file_read_cb(void *baton, char *buffer, apr_size_t *len)
> +static svn_error_t *
> +file_write_cb(void *baton, const char *buffer, apr_size_t *len)
> +static void
> +file_timeout_cb(void *baton, apr_interval_time_t interval)
> +static svn_boolean_t
> +file_pending_cb(void *baton)
> +static svn_error_t *
> +sock_read_cb(void *baton, char *buffer, apr_size_t *len)
> +static svn_error_t *
> +sock_write_cb(void *baton, const char *buffer, apr_size_t *len)
> +static void
> +sock_timeout_cb(void *baton, apr_interval_time_t interval)
> +static svn_boolean_t
> +sock_pending_cb(void *baton)

While I can guess what these all do, it would be nice if there was some
minimal commenting.

Regards,
Malcolm