[PATCH] HTTP reason phrases not always returned in ra_dav error messages

[Bill Tutt <ra...@lyra.org>]

While trying to diagnose whether an error a proxy error or not I noticed
not all ra_dav error messages were including the Neon error message
correctly.
 
Attached is a patch that takes a stab at fixing the problem.  I'm not so
happy about what I did to props.c, any other ideas would definately be
appreciated.
 
Here's the Changelog:
 
* libsvn_ra_dav/props.c:
  (svn_ra_dav__get_props) Use error helper function to ensure
      that the HTTP reason string gets included in the error context.
* libsvn_ra_dav/util.c:
  (svn_ra_dav__parsed_request, svn_ra_dav__request_dispatch) Adjust
      the code knowing that Neon can return NE_OK while still having
      a failure status code.

FYI,
Bill
----
Do you want a dangerous fugitive staying in your flat?
No.
Well, don't upset him and he'll be a nice fugitive staying in your flat.
 
 

Re: [PATCH] HTTP reason phrases not always returned in ra_dav error messages

[Ben Collins-Sussman <su...@collab.net>]

"Bill Tutt" <ra...@lyra.org> writes:

> While trying to diagnose whether an error a proxy error or not I noticed
> not all ra_dav error messages were including the Neon error message
> correctly.
[...]
> * libsvn_ra_dav/util.c:
>   (svn_ra_dav__parsed_request, svn_ra_dav__request_dispatch) Adjust
>       the code knowing that Neon can return NE_OK while still having
>       a failure status code.

Sure, this makes sense.  All you're doing is changing

   if (A) error;  if (B) error;

into

   if (A || B) error;

It doesn't change the logic, just improves the error-throwing detail.
Looks good.

>  
> * libsvn_ra_dav/props.c:
>   (svn_ra_dav__get_props) Use error helper function to ensure
>       that the HTTP reason string gets included in the error context.

This may not be as safe.  You're changing the logic here:

> -  if (rv != NE_OK)
> +  if (rv != NE_OK
> +      || status_code != 200)
>      {
>        const char *msg = apr_psprintf(pool, "PROPFIND of %s", url);
>        return svn_ra_dav__convert_error(sess, msg, rv);
>      }
> -
> -  if (404 == status_code)
> -    return svn_error_createf(SVN_ERR_RA_DAV_PROPS_NOT_FOUND, NULL,
> -                             "Failed to fetch props for '%s'", url);


Although I guess simply combining the if statements here won't help
you.  You need to be able to catch 5XX codes from your proxy failures.

Maybe we should change this section of __get_props to

   if (nv != NE_OK || status_code < 200 || status_code > 299)

?

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org