On 11/8/06, Malcolm Rowe <malcolm-svn-dev@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
Received on Thu Nov 9 00:35:40 2006