Re: serf error handling for locks without authn

Philip Martin <philip.martin_at_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