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?
>...
> + 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.
> + 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.
>...
> @@ -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();".
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.
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().
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1028344
Received on 2009-01-16 11:33:45 CET