Greg Stein wrote:
> On Wed, Jan 14, 2009 at 21:18, C. Michael Pilato <cmpilato_at_collab.net> wrote:
>> ...
>> +++ trunk/subversion/mod_dav_svn/reports/get-location-segments.c Wed Jan 14 12:18:24 2009 (r35250)
>> ...
>> @@ -91,35 +111,65 @@ send_get_location_segments_report(ap_fil
>> const char *path)
>> {
>> apr_status_t apr_err;
>> + svn_error_t *serr;
>> + dav_error *derr = NULL;
>> dav_svn__authz_read_baton arb;
>> struct location_segment_baton location_segment_baton;
>>
>> - if ((apr_err = ap_fprintf(output, bb, DAV_XML_HEADER DEBUG_CR
>> - "<S:get-location-segments-report xmlns:S=\""
>> - SVN_XML_NAMESPACE "\" xmlns:D=\"DAV:\">"
>> - DEBUG_CR)))
>> - return svn_error_create(apr_err, 0, NULL);
>
> I don't understand why you aren't unconditionally sending the
> report-start tag. If you have a failure partway through the log
> messages, you could still end up with an unterminated report. So why
> not allow a failure with just the opener?
Most of the problems that are going to happen with
svn_repos_node_location_segments() are likely to happen before the first
invocation of the callback occurs. So, no, in the majority of error cases,
it is still possible to provide a valid XML stream to the caller by delaying
the opening.
>> ...
>> + cleanup:
>> + /* Flush the contents of the brigade (returning an error only if we
>> + don't already have one). */
>> + if (location_segment_baton.sent_opener)
>> + {
>
> Why not just flush unconditionally? This seems to add a little extra
> complication. And if you remove .sent_opener anyways, then you'll fall
> back to an unconditional flush anyways.
See above. No opener, nothing to flush, one less opportunity for an error
to occur. But I'm not particularly tied to this line of thinking -- I
merely replicated what the 'svn log' REPORT logic did.
>
>> + apr_err = ap_fflush(output, bb);
>> + if (apr_err && (! derr))
>> + {
>> + derr = dav_svn__convert_err(svn_error_create(apr_err, 0, NULL),
>> + HTTP_INTERNAL_SERVER_ERROR,
>> + "Error flushing brigade.",
>> + resource->pool);
>
> Okay. Now this is just silly :-P ... use dav_new_error() instead.
Heh. That was just the result of code evolution and failure to recognize
opportunities for simplification.
>> ...
>> @@ -128,9 +178,7 @@ dav_svn__get_location_segments_report(co
>> const apr_xml_doc *doc,
>> ap_filter_t *output)
>> {
>> - svn_error_t *serr;
>> dav_error *derr = NULL;
>> - apr_status_t apr_err;
>> apr_bucket_brigade *bb;
>> int ns;
>> apr_xml_elem *child;
>> @@ -210,20 +258,7 @@ dav_svn__get_location_segments_report(co
>> bb = apr_brigade_create(resource->pool, output->c->bucket_alloc);
>>
>> /* Alright, time to drive the response. */
>> - if ((serr = send_get_location_segments_report(output, bb, resource,
>> - peg_revision, start_rev,
>> - end_rev, path)))
>> - derr = dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
>> - "Error writing REPORT response.",
>> - resource->pool);
>> -
>> - /* Flush the contents of the brigade (returning an error only if we
>> - don't already have one). */
>> - if (((apr_err = ap_fflush(output, bb))) && (! derr))
>> - return dav_svn__convert_err(svn_error_create(apr_err, 0, NULL),
>> - HTTP_INTERNAL_SERVER_ERROR,
>> - "Error flushing brigade.",
>> - resource->pool);
>> -
>> - return derr;
>> + return send_get_location_segments_report(output, bb, resource,
>> + peg_revision, start_rev,
>> + end_rev, path);
>
> I'd say that since you created the brigade here, that you continue to
> (unconditionally) flush it here. That will also get rid of all the
> "goto cleanup" statements in the other function. You can just "return
> dav_svn__convert_err();".
Good point.
> I'll also note that some of the ap_fprintf() calls can simply be
> ap_fputs() calls since you aren't doing any substitutions. The switch
> over to dav_svn__send_xml() buys you a bit of connection-drop
> detection, but that is mostly for early-exit of processing (you
> certainly aren't going to tell the client anything). It would also be
> possible to write a function like
> SVN_ERR(dav_svn__check_connection(output)); that you could call at
> strategic points (i.e. once per log message callback) to detect/exit
> processing when the connection drops.
Okey dokey.
> Anyways... I'm a bit stymied by this whole change. I don't understand
> the motivation at all. "Fix the error handling ..." doesn't describe
> what the problem used to be. I don't see anything wrong with the old
> logic, and this new code just seems to be a bit more complicated than
> a bunch of SVN_ERR() uses in the main function, and having the wrapper
> to the convert().
In the case of a failure of svn_repos_node_location_segments before, three
problems occurred. First, the REPORT would return a 200 status code.
Secondly, we did nothing to try to keep the response body XML-parseable.
Finally, we never attempted to embed into the response the special error
tags that let the client know what went wrong. On the client side, you'd
invoke the REPORT and just back an error saying "REPORT failed: 200 OK" (or
somesuch); over SSL it was a "Secure connection truncated" error.
So this change was meant to remedy all of those problems by mimicking the
way other callback-driven REPORT response logic worked (such as that of 'svn
log').
--
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1028614
Received on 2009-01-16 14:43:29 CET