On Mon, Jan 25, 2010 at 6:21 PM, Julian Foad <julianf...@btopenworld.com> wrote:
> On Fri, 2010-01-22, Ivan Zhakov wrote:
>> On Wed, Jan 13, 2010 at 6:04 PM, Julian Foad <julianf...@btopenworld.com>
>> wrote:
>> > On Mon, 2010-01-11, Ivan Zhakov wrote:
>> > Your new patch now has a more descriptive error message, that is the
>> > same as the one produced by the parse_spool_file() function, which is
>> > good, but they are generated in different ways at different call levels,
>> > which is bad. Your patch has:
>> >
>> >> + parser_status = ne_xml_parse(pwb->parser, data, len);
>> >> + if (parser_status)
>> >> + {
>> >> + /* Pass XML parser error. */
>> >> + pwb->req->err = svn_error_createf
>> >> + (SVN_ERR_RA_DAV_REQUEST_FAILED, NULL,
>> >> + _("The %s request returned invalid XML "
>> >> + "in the response: %s (%s)"),
>> >> + pwb->req->method,
>> >> + ne_xml_get_error(pwb->parser),
>> >> + pwb->req->url);
>> >> + }
>> >
>> > whereas parse_request() calls parse_spool_file() which calls
>> > ne_xml_parse(), and then it does:
>> >
>> >> /* was there an XML parse error somewhere? */
>> >> msg = ne_xml_get_error(success_parser);
>> >> if (msg != NULL && *msg != '\0')
>> >> return svn_error_createf(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL,
>> >> _("The %s request returned invalid XML "
>> >> "in the response: %s (%s)"),
>> >> method, msg, url);
>> >
>> > Can't we do them both in the same way, somehow?
>> >
>> I don't like code duplication too, but I didn't find good way how this
>> code could be merged. So I propose to commit my patch since it makes
>> error diagnostic much better it costs just 2-line duplication.
>
> Thanks, Ivan.
>
> I looked at this a bit more. First I factored out the error message
> generation into a separate function.
>
> I committed that version as r902836.
>
Thanks!
I committed small follow up in r903153 to use SVN_RA_NEON__REQ_ERR
macro instead of assigning req->err to prevent potential error
leakage.
> When I said "different call levels", I was thinking of the fact that the
> first function, wrapper_reader_cb(), extracts the Neon parse error
> message and turns it into a Subversion error immediately after calling
> the ne_xml_parse(), whereas the second function, parsed_request(), calls
> the parser in a complex way (many times) and then extracts any Neon
> parse error message afterwards, when it is finished with the parser.
>
> If wrapper_reader_cb() converts the parser error into a Subversion error
> itself, it now has two ways of reporting an error: by returning the
> parser status code (int) and/or by setting pwb->req->err. Generally it
> is best to have just one way. I can't see a big problem with reporting
> the error both ways but I'm looking for a better way.
>
> I think the right place to call the (factored out) error detector
> function is at the end of the request that uses the parser - so,
> mirroring the end of parsed_request(), that would be at the end of
> svn_ra_neon__request_dispatch() INSTEAD OF in wrapper_reader_cb().
> However, I'm not completely sure of that.
>
> Does that make any sense?
>
I'm not sure but I'd like prefer to extract neon error ASAP. Passing
global state like 'last error' between call levels looks unreliable
for me.
--
Ivan Zhakov
VisualSVN Team
