Bert Huijben wrote on Thu, Aug 05, 2010 at 08:50:15 +0200:
>
>
> > -----Original Message-----
> > From: Daniel Shahaf [mailto:d.s_at_daniel.shahaf.name]
> > Sent: donderdag 5 augustus 2010 8:13
> > To: dev_at_subversion.apache.org
> > Cc: commits_at_subversion.apache.org; Daniel Shahaf
> > Subject: Re: svn commit: r982415 - in
> > /subversion/trunk/subversion/libsvn_ra_serf: commit.c replay.c serf.c
> > update.c
> >
> > rhuijben_at_apache.org wrote on Wed, Aug 04, 2010 at 22:20:30 -0000:
> > > Author: rhuijben
> > > Date: Wed Aug 4 22:20:30 2010
> > > New Revision: 982415
> > >
> > > URL: http://svn.apache.org/viewvc?rev=982415&view=rev
> > > Log:
> > > Following up on r982398, fix a few more mostly theoretical error
> > leaks.
> > >
> >
> > Thanks; review below.
> >
> > > Modified: subversion/trunk/subversion/libsvn_ra_serf/commit.c
> > > URL:
> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf
> > /commit.c?rev=982415&r1=982414&r2=982415&view=diff
> > >
> > =======================================================================
> > =======
> > > --- subversion/trunk/subversion/libsvn_ra_serf/commit.c (original)
> > > +++ subversion/trunk/subversion/libsvn_ra_serf/commit.c Wed Aug 4
> > 22:20:30 2010
> > > @@ -748,6 +748,7 @@ create_proppatch_body(void *baton,
> > > {
> > > proppatch_context_t *ctx = baton;
> > > serf_bucket_t *body_bkt;
> > > + svn_error_t *err;
> > >
> > > body_bkt = serf_bucket_aggregate_create(alloc);
> > >
> > > @@ -764,9 +765,10 @@ create_proppatch_body(void *baton,
> > > svn_ra_serf__add_open_tag_buckets(body_bkt, alloc, "D:set",
> > NULL);
> > > svn_ra_serf__add_open_tag_buckets(body_bkt, alloc, "D:prop",
> > NULL);
> > >
> > > - svn_ra_serf__walk_all_props(ctx->changed_props, ctx->path,
> > > - SVN_INVALID_REVNUM,
> > > - proppatch_walker, body_bkt, pool);
> > > + err = svn_ra_serf__walk_all_props(ctx->changed_props, ctx-
> > >path,
> > > + SVN_INVALID_REVNUM,
> > > + proppatch_walker, body_bkt,
> > pool);
> > > + svn_error_clear(err); /* ### */
> > >
> >
> > What is the reason for silently ignoring errors here?
>
> This is a serf callback which can't return errors. Clearing is better then
> leaking.
>
I disagree. If the walker did return an error, we can't be sure it's safe
to continue, so IMO we shouldn't.
We could clear the err and return NULL (or however this callback signals an
error to serf), or send the err to some global handler, or just
SVN_ERR_ASSERT_NO_RETURN(!err). But just ignoring&clearing it seems wrong.
(And it's going to be pretty nasty to debug this, I guess, if an error
should someday get thrown here.)
> >
> > > svn_ra_serf__add_close_tag_buckets(body_bkt, alloc, "D:prop");
> > > svn_ra_serf__add_close_tag_buckets(body_bkt, alloc, "D:set");
> > > @@ -777,9 +779,10 @@ create_proppatch_body(void *baton,
> > > svn_ra_serf__add_open_tag_buckets(body_bkt, alloc, "D:remove",
> > NULL);
> > > svn_ra_serf__add_open_tag_buckets(body_bkt, alloc, "D:prop",
> > NULL);
> > >
> > > - svn_ra_serf__walk_all_props(ctx->removed_props, ctx->path,
> > > - SVN_INVALID_REVNUM,
> > > - proppatch_walker, body_bkt, pool);
> > > + err = svn_ra_serf__walk_all_props(ctx->removed_props, ctx-
> > >path,
> > > + SVN_INVALID_REVNUM,
> > > + proppatch_walker, body_bkt,
> > pool);
> > > + svn_error_clear(err); /* ### */
> > >
> >
> > Same question.
>
> Same function.
Same disagreement.
Received on 2010-08-05 22:18:52 CEST