Re: svn commit: r1555774 - in /subversion/trunk/subversion/libsvn_ra_serf: eagain_bucket.c ra_serf.h update.c util.c

[Ivan Zhakov <iv...@visualsvn.com>]

On 6 January 2014 17:59,  <rh...@apache.org> wrote:
> Author: rhuijben
> Date: Mon Jan  6 13:59:45 2014
> New Revision: 1555774
>
> URL: http://svn.apache.org/r1555774
> Log:
> In libsvn_ra_serf, replace the pause handling for the update response that was
> embedded through the old -not transition based- xml parser with a pause
> handling response handler wrapper directly in the update logic.
>
Hi Bert, see my comments below.

> +
> +static apr_status_t
> +eagain_bucket_read(serf_bucket_t *bucket,
> +                   apr_size_t requested,
> +                   const char **data,
> +                   apr_size_t *len)
> +{
> +  eagain_baton_t *eab = bucket->data;
> +
> +  if (eab->remaining > 0)
> +    {
> +      *data = eab->data;
> +      if (requested > eab->remaining || requested == SERF_READ_ALL_AVAIL)
> +        {
> +          *len = eab->remaining;
> +          eab->data = NULL;
> +          eab->remaining = 0;
> +        }
> +      else
> +        {
> +          *len = requested;
> +          eab->data += requested;
> +          eab->remaining -= requested;
> +        }
> +
> +      if (eab->remaining)
> +        return APR_SUCCESS;
> +    }
> +
> +  return APR_EAGAIN;
> +}
> +
Alternative approach is reuse SIMPLE bucket at make eagain_bucket
wrapper around any inner bucket transforming APR_EOF to APR_EAGAIN.

[...]

> +
> +static apr_status_t
> +eagain_bucket_peek(serf_bucket_t *bucket,
> +                   const char **data,
> +                   apr_size_t *len)
> +{
> +  const eagain_baton_t *eab = bucket->data;
> +
> +  *data = eab->data;
> +  *len = eab->remaining;
> +
> +  return *data == NULL ? APR_EOF : APR_SUCCESS;
> +}
Are you sure about returning APR_EOF here?

[...]

>
> Modified: subversion/trunk/subversion/libsvn_ra_serf/update.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/update.c?rev=1555774&r1=1555773&r2=1555774&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_serf/update.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_serf/update.c Mon Jan  6 13:59:45 2014
[...]

> +/* Delaying wrapping reponse handler, to avoid creating too many
> +   requests to deliver efficiently */
> +static svn_error_t *
> +update_delay_handler(serf_request_t *request,
> +                     serf_bucket_t *response,
> +                     void *handler_baton,
> +                     apr_pool_t *scratch_pool)
> +{
[...]
> +         if (len == 0 && !at_eof)
> +           return svn_ra_serf__wrap_err(status, NULL);
> +
> +         /* If not at EOF create a bucket that finishes with EAGAIN, otherwise
> +            use a standard bucket with default EOF handling */
> +         err = udb->inner_handler(request,
> +                                  at_eof
> +                                   ? serf_bucket_simple_create(
> +                                            data, len, NULL, NULL,
> +                                            serf_request_get_alloc(request))
> +                                   : svn_ra_serf__create_bucket_with_eagain(
> +                                            data, len,
> +                                            serf_request_get_alloc(request)),
With my suggestion above this code will be converted to something like:
[[[
bucket = serf_bucket_simple_create();
if (!at_eof)
  bucket = svn_ra_serf__create_bucket_with_eagain(bucket);
err = udb->inner_handler(request, bucket, udb->inner_handler_baton, iterpool);
]]]

-- 
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com