You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Malcolm Rowe <ma...@farside.org.uk> on 2006/11/08 23:19:13 UTC
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
Re: svn commit: r22238 - trunk/subversion/libsvn_ra_svn
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Wed, Nov 08, 2006 at 06:35:18PM -0500, Garrett Rooney wrote:
> >This is a public structure, isn't it? Are there any
> >backward-compatibility considerations we need to worry about?
>
> It's declared in a header file that's private (never installed
> anywhere), and it's only ever shared between svnserve and
> libsvn_ra_svn, which need to be upgraded at the same time anyway, so
> no, I don't think so.
>
You're right, sorry. I was confusing it with svn_ra_svn.h in include/.
> >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.
>
> The original reason was that someone (ghudson?) objected to adding the
> required interfaces to svn_stream_t to make this work. If I recall
> correctly the original version of this code did the same stuff via an
> svn_stream_ioctl function or something like that.
>
Yes, I thought I remembered something along those lines. An ioctl
interface is probably a bit sucky, but I honestly can't think of anything
better (svn_stream_t isn't public, so we couldn't even overlay the two
structures in a super-/sub-class style and downcast where necessary).
> >> +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.
>
> Honestly, here I disagree, you're talking about functions where the
> comments would be almost as long as the actual code, and would add no
> meaning. If you look at the code this replaced there was perhaps one
> comment, and that still exists in the new code. The fact that the
> code has migrated into a function does not mean it immediately needs a
> comment saying that it does exactly what you'd expect it to do based
> on the name of the function and the interface it implements.
>
I was really just angling for a minimal
/* Functions to implement the ra_svn_stream... interface for
(file-backed | socket-backed) streams. */
kind of thing. I agree that there's no point in documenting each one in
turn, but a reference to the interface would be nice. (And something to
explain what pending() does, since it's not part of either of those
groups, it looks like).
Regards,
Malcolm
Re: svn commit: r22238 - trunk/subversion/libsvn_ra_svn
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 11/8/06, Malcolm Rowe <ma...@farside.org.uk> wrote:
> > +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?
Agreed, those could probably stand to have some...
> > /* 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?
It's declared in a header file that's private (never installed
anywhere), and it's only ever shared between svnserve and
libsvn_ra_svn, which need to be upgraded at the same time anyway, so
no, I don't think so.
> > +/* 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.
The original reason was that someone (ghudson?) objected to adding the
required interfaces to svn_stream_t to make this work. If I recall
correctly the original version of this code did the same stuff via an
svn_stream_ioctl function or something like that.
Agreed that some comments would help here.
> > +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.
Honestly, here I disagree, you're talking about functions where the
comments would be almost as long as the actual code, and would add no
meaning. If you look at the code this replaced there was perhaps one
comment, and that still exists in the new code. The fact that the
code has migrated into a function does not mean it immediately needs a
comment saying that it does exactly what you'd expect it to do based
on the name of the function and the interface it implements.
If Vlad doesn't send in a patch to add some more comments in the next
day or so I'll go through and add them in the rest of the code,
particularly the parts where the new interfaces are declared.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org