subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Philip Martin <philip.mar...@wandisco.com>
Subject Re: serf error handling for locks without authn
Date Tue, 04 Jun 2013 10:33:46 GMT
Philip Martin <philip.martin@wandisco.com> writes:

> The code in libsvn_ra_serf/locks.c:determine_error looks dodgy:
>
> 213 static svn_error_t *
> 214 determine_error(svn_ra_serf__handler_t *handler,
> 215                 svn_error_t *err)
> 216 {
> 217     {
> 218       apr_status_t errcode;
> 219 
> 220       if (handler->sline.code == 423)
> 221         errcode = SVN_ERR_FS_PATH_ALREADY_LOCKED;
> 222       else if (handler->sline.code == 403)
> 223         errcode = SVN_ERR_RA_DAV_FORBIDDEN;
> 224       else
> 225         return err;
> 226 
> 227       /* Client-side or server-side error already. Return it.  */
> 228       if (err != NULL)
> 229         return err;
> 230 
> 231       /* The server did not send us a detailed human-readable error.
> 232          Provide a generic error.  */
> 233       err = svn_error_createf(errcode, NULL,
> 234                               _("Lock request failed: %d %s"),
> 235                               handler->sline.code,
> 236                               handler->sline.reason);
> 237     }
> 238 
> 239   return err;
> 240 }
>
> Lines 224-225 mean that all the following code is never executed and
> anything other than 423 or 403 is going to get lost.  I can't simply
> remove those two lines because errcode needs a value for
> svn_error_createf.

I'm trying to work out what this code should look like.  This function
is called after LOCK and PROPFIND, but not UNLOCK which has its own
code.  I think the function should be something like:

  apr_status_t errcode;

  if (err)
    return err;

  if (handler->sline.code == 200 || handler->sline.code == 207)
    return SVN_NO_ERROR;
  else if (handler->sline.code == 423)
    errcode = SVN_ERR_FS_PATH_ALREADY_LOCKED;
  else if (handler->sline.code == 403)
    errcode = SVN_ERR_RA_DAV_FORBIDDEN;
  else
    errcode = SVN_ERR_RA_DAV_REQUEST_FAILED;

  return svn_error_createf(errcode, NULL,
                           _("Lock request failed: %d %s"),
                           handler->sline.code,
                           handler->sline.reason ? handler->sline.reason : "");


where a non-NULL err is given precedence otherwise the status line is
examined.  200 is not an error and the regression tests require 207 to
be treated as success as well.

I'm not confident about changing this code but I think we have to do
something for 1.8 as the current code simply drops status line errors if
err is NULL.

-- 
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download

Mime
View raw message