Philip Martin wrote:
> While reviewing /branches/1.2.x-get-locks I came across
> svn_ra_dav__parsed_request error handling like this:
>
> err = svn_ra_dav__parsed_request(... &status_code ...);
>
> /* Map status 501: Method Not Implemented to our not implemented error.
> 1.0.x servers and older don't support this report. */
> if (status_code == 501)
> return svn_error_create(SVN_ERR_RA_NOT_IMPLEMENTED, err,
> _("Server does not support locking features"));
>
> if (err && err->apr_err == SVN_ERR_UNSUPPORTED_FEATURE)
> return svn_error_create(SVN_ERR_RA_NOT_IMPLEMENTED, err,
> _("Server does not support locking features"));
>
> Note that status_code is checked before err. That could be a problem
> since parsed_request can return an error without setting status code.
Good catch. Even if the current implementation were not able to return an
error without setting the status code, it would be against convention to assume
so. Conventionally, if a function returns an error then we do not assume that
any of its outputs are valid. (It might have returned an error during argument
validation, for example.)
> I don't think it's feasible for callers to examine err to determine
> whether status_code is valid (or is it?),
/Mais si/ - But yes, I think it is feasible and right for callers to deal with
the error return first, and only if that is null to then examine outputs like
the status code. That should be straightforward to do, but in this case the
function is insufficiently documented, so somebody will have to study its
implementation to work out what combinations of errors and status code it can
return, and whether it will need to be adjusted to work in the conventional way
(the returned error taking priority).
Looking at other callers, I see that this status code is not the only output
that is examined before the error return. In subversion/libsvn_ra_dav/fetch.c:
> err = svn_ra_dav__parsed_request(ras->sess, "REPORT", url,
> body, NULL, NULL,
> getlocks_start_element,
> getlocks_cdata_handler,
> getlocks_end_element,
> &baton,
> NULL, /* extra headers */
> &status_code,
> FALSE,
> pool);
>
> /* ### Should svn_ra_dav__parsed_request() take care of storing auth
> ### info itself? */
> err = svn_ra_dav__maybe_store_auth_info_after_result(err, ras);
>
> /* At this point, 'err' might represent a local error (neon choked,
> or maybe something went wrong storing auth creds). But if
> 'baton.err' exists, that's an error coming right from the server,
> marshalled over the network. We give that top priority. */
> if (baton.err)
> {
> if (err)
> svn_error_clear(err);
>
> /* mod_dav_svn is known to return "unsupported feature" on
> unknown REPORT requests, but it's our svn_ra.h promise to
> return a similar, specific error code. */
> if (baton.err->apr_err == SVN_ERR_UNSUPPORTED_FEATURE)
> return svn_error_create(SVN_ERR_RA_NOT_IMPLEMENTED, baton.err,
> _("Server does not support locking features"));
> return baton.err;
> }
Same sort of situation. I don't know. It looks like this function has been
written with lots of shared knowledge between it and its callers. As it's a
private function, it's not essential to sort this out from an API point of
view, but it does mean it will be hard to maintain and use.
> so I'd suggest that
> parsed_request be responsible for always setting status_code.
That's not the approach I'd like to recommend in general, but for a quick fix I
suppose it would be better than nothing. Same applies to this other "baton"
parameter, of course.
> If we decide to continue to
> make the caller responsible then svn_ra_dav__parsed_request should
> document that requirement.
Of course.
Do you feel able to do something about this? I don't.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Aug 5 20:09:56 2005