Re: svn commit: r30843 - trunk/subversion/libsvn_ra_serf

[Karl Fogel <kf...@red-bean.com>]

lgo@tigris.org writes:
> Log:
> ra_serf: Gracefully handle XML parsing errors, instead of aborting.
>
> * subversion/libsvn_ra_serf/replay.c
>   (struct replay_context_t): add field parser_ctx. We need this
>    reference to get back the XML parsing error.
>   (svn_ra_serf__replay_range): correctly handle the first parser error in
>    the list of received responses.
>
> --- trunk/subversion/libsvn_ra_serf/replay.c	(r30842)
> +++ trunk/subversion/libsvn_ra_serf/replay.c	(r30843)
> @@ -116,6 +116,9 @@ typedef struct {
>    apr_hash_t *revs_props;
>    apr_hash_t *props;
>  
> +  /* Keep a reference to the XML parser ctx to report any errors. */
> +  svn_ra_serf__xml_parser_t *parser_ctx;
> +
>  } replay_context_t;
>  
>  
> @@ -693,6 +696,7 @@ svn_ra_serf__replay_range(svn_ra_session
>        svn_ra_serf__list_t *done_list;
>        svn_ra_serf__list_t *done_reports = NULL;
>        replay_context_t *replay_ctx;
> +      int status_code;
>  
>        /* Send pending requests, if any. Limit the number of outstanding 
>           requests to MAX_OUTSTANDING_REQUESTS. */
> @@ -746,12 +750,16 @@ svn_ra_serf__replay_range(svn_ra_session
>            parser_ctx->start = start_replay;
>            parser_ctx->end = end_replay;
>            parser_ctx->cdata = cdata_replay;
> +          parser_ctx->status_code = &status_code;
>            parser_ctx->done = &replay_ctx->done;
>            parser_ctx->done_list = &done_reports;
>            parser_ctx->done_item = &replay_ctx->done_item;
>            handler->response_handler = svn_ra_serf__handle_xml_parser;
>            handler->response_baton = parser_ctx;

It might be worth making a comment where we declare 'status_code' that
its sole purpose is to be an address so that the
'parser_ctx->status_code' field will not be NULL (and therefore the
parser will not abort on error).  Otherwise, the fact that we carefully
assign status_code's address but never check its value later could be
confusing.

(Or am I misinterpreting what's going on?)

That concern notwithstanding, I've voted for this in 1.5.x/STATUS.

-Karl

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r30843 - trunk/subversion/libsvn_ra_serf

[Lieven Govaerts <sv...@mobsol.be>]

Karl Fogel wrote:
> lgo@tigris.org writes:
>> Log:
>> ra_serf: Gracefully handle XML parsing errors, instead of aborting.
>>
..
>>        svn_ra_serf__list_t *done_reports = NULL;
>>        replay_context_t *replay_ctx;
>> +      int status_code;
>>  
>>        /* Send pending requests, if any. Limit the number of outstanding 
..>
> It might be worth making a comment where we declare 'status_code' that
> its sole purpose is to be an address so that the
> 'parser_ctx->status_code' field will not be NULL (and therefore the
> parser will not abort on error).  Otherwise, the fact that we carefully
> assign status_code's address but never check its value later could be
> confusing.
> 
> (Or am I misinterpreting what's going on?)
> 

I added the comment in r30992, as you're right in that status_code 
doesn't serve any real purpose besides avoiding the abort for now.

> That concern notwithstanding, I've voted for this in 1.5.x/STATUS.
> 

Thank you, that's very nice :-)

thanks for the review Karl.

Lieven

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org