You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2010/01/25 15:21:05 UTC
Re: [PATCH] Fix the awful "200 OK" error message when can't
connect to repository
On Fri, 2010-01-22, Ivan Zhakov wrote:
> On Wed, Jan 13, 2010 at 6:04 PM, Julian Foad <ju...@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.
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?
- Julian
Re: [PATCH] Fix the awful "200 OK" error message when can't
connect to repository
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2010-01-26, Ivan Zhakov wrote:
> > 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.
Thank you.
> > 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.
Well, I don't know, so I'll leave it as it is.
- Julian
Re: [PATCH] Fix the awful "200 OK" error message when can't connect
to repository
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Mon, Jan 25, 2010 at 6:21 PM, Julian Foad <ju...@btopenworld.com> wrote:
> On Fri, 2010-01-22, Ivan Zhakov wrote:
>> On Wed, Jan 13, 2010 at 6:04 PM, Julian Foad <ju...@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