Julian Foad <julianfoad@btopenworld.com> writes:
> 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.
As far as I can see there are circumstances where it is intentional
that both an error is returned and status_code gets set.
> 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).
It's not clear to me that checking the error before the status_code is
any more corect than checking status_code before the error. I suppose
we could try to document which errors also have a status_code but
strikes me as a poor interface.
> 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 assume from your later comments that you are referring to baton. I
think baton is different from status_code. status_code gets set, or
not set, inside svn_ra_dav__parsed_request and the caller doesn't know
exactly when it will happen and when status_code will be valid. On
the other hand baton gets set in the callers callback, so it is
entirely controlled by the caller and the caller can be expected to
know when it is valid.
> 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.
Not really. My suggestions are we do one of:
* Have svn_ra_dav__parsed_request set *STATUS_CODE to zero (or -1 or
some other magic number) before doing anything else
* Prominently document that the caller must always set *STATUS_CODE
before calling svn_ra_dav__parsed_request
but I'd prefer an ra_dav expert to handle it.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Aug 5 21:09:32 2005