Just quickly scanning and it looks good, but I think you leaked an error
(twice - with the same construct).
In subversion/mod_dav_svn/log.c:
> @@ -284,43 +299,33 @@
> log_receiver,
> &lrb,
> resource->pool);
> -
> if (serr)
> {
> - /* If we can, report a DAV error. */
> - if (lrb.needs_header)
> - {
> - /* Bail out before writing any of <S:log-report>. */
> - return dav_svn_convert_err(serr, HTTP_BAD_REQUEST,
> serr->message);
> - }
> - else
> - {
> - /* ### We've sent some content to the output filter, meaning
> - ### that we cannot simply return an error here. In the
> - ### future, mod_dav may specify a way to signal an error
> - ### even after the response stream has begun.
> -
> - ### For now we punt, sending the error message to the
> - ### client as a <S:log-item> (using its <D:version-name>
> - ### and <D:comment> children).
> -
> - ### http://subversion.tigris.org/issues/show_bug.cgi?id=816
> - ### describes a situation where this helps.*/
> - log_receiver(&lrb,
> - NULL,
> - SVN_INVALID_REVNUM,
> - "", "",
> - serr->message,
> - resource->pool);
> - }
> + derr = dav_svn_convert_err(serr, HTTP_BAD_REQUEST, serr->message);
> + goto cleanup;
> }
>
> - /* End the log report. */
> - maybe_send_header(&lrb);
> - send_xml(&lrb, "</S:log-report>" DEBUG_CR);
> + if ((serr = maybe_send_header(&lrb)))
> + {
> + derr = dav_svn_convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
> + "Error beginning REPORT reponse.");
> + goto cleanup;
> + }
> +
> + if ((serr = send_xml(&lrb, "</S:log-report>" DEBUG_CR)))
> + {
> + derr = dav_svn_convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
> + "Error ending REPORT reponse.");
> + goto cleanup;
> + }
>
> - /* flush the contents of the brigade */
> - ap_fflush(output, lrb.bb);
> + cleanup:
>
> - return NULL;
> + /* Flush the contents of the brigade (returning an error only if we
> + don't already have one). */
> + if (((apr_err = ap_fflush(output, lrb.bb))) && (! derr))
> + derr = dav_svn_convert_err(svn_error_create(apr_err, 0, NULL),
> + HTTP_INTERNAL_SERVER_ERROR,
> + "Error flushing brigade.");
> + return derr;
> }
You're not clearing serr here, or above, but neither does
dav_svn_convert_err.
> Modified:
trunk/subversion/mod_dav_svn/update.c
==============================================================================
> --- trunk/subversion/mod_dav_svn/update.c (original)
> +++ trunk/subversion/mod_dav_svn/update.c Fri Jan 23 12:34:32 2004
> @@ -1389,28 +1407,52 @@
> TRUE, /* send entryprops */
> FALSE, /* don't ignore ancestry */
> resource->pool);
> -
> - send_xml(&uc, "</S:resource-walk>" DEBUG_CR);
> + if (serr)
> + {
> + derr = dav_svn_convert_err(serr,
> HTTP_INTERNAL_SERVER_ERROR,
> + "Resource walk failed.");
> + goto cleanup;
> + }
> +
> + serr = send_xml(&uc, "</S:resource-walk>" DEBUG_CR);
> + if (serr)
> + {
> + derr = dav_svn_convert_err(serr,
> HTTP_INTERNAL_SERVER_ERROR,
> + "Unable to complete resource
> walk.");
> + goto cleanup;
> + }
> }
> }
>
> /* Close the report body, unless some error prevented it from being
> started in the first place. */
> if (uc.started_update)
> - send_xml(&uc, "</S:update-report>" DEBUG_CR);
> + {
> + serr = send_xml(&uc, "</S:update-report>" DEBUG_CR);
> + if (serr)
> + {
> + derr = dav_svn_convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
> + "Unable to complete update
> report.");
> + goto cleanup;
> + }
> + }
> +
> + cleanup:
>
> - /* flush the contents of the brigade */
> - ap_fflush(output, uc.bb);
> + /* Flush the contents of the brigade (returning an error only if we
> + don't already have one). */
> + if (((apr_err = ap_fflush(output, uc.bb))) && (! derr))
> + derr = dav_svn_convert_err(svn_error_create(apr_err, 0, NULL),
> + HTTP_INTERNAL_SERVER_ERROR,
> + "Error flushing brigade.");
>
> /* if an error was produced EITHER by the dir_delta drive or the
> resource-walker... */
> - if (serr != NULL)
> + if (derr)
> {
> - svn_error_clear(svn_repos_abort_report(rbaton, resource->pool));
> - return dav_svn_convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
> - "A failure occurred during the
> completion "
> - "and response generation for the update
> "
> - "report.");
> + if (rbaton)
> + svn_error_clear(svn_repos_abort_report(rbaton, resource->pool));
> + return derr;
> }
>
> /* Destroy our subpool. */
>
Same here. You're not destroying serr, but neither does dav_svn_convert_err.
bye,
Erik.
--
+++ GMX - die erste Adresse für Mail, Message, More +++
Bis 31.1.: TopMail + Digicam für nur 29 EUR http://www.gmx.net/topmail
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jan 23 20:10:35 2004