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
Received on 2010-01-28 14:30:29 CET