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
