[PATCH] Address some -O3 issues on Ubuntu 10.04

Jeremy Allison jra at samba.org
Thu Aug 4 20:49:32 UTC 2016


On Thu, Aug 04, 2016 at 09:57:22PM +0200, Michael Adam wrote:
> > Well after metze's analysis I have one final attempt
> > at fixing them (attached). I think patch #1 is a
> > genuine bugfix that needs to go in regardless.
> 
> Good! But see 1 comment inline below.
> 
> > Patch #2 is a band-aid for the error, but something
> > like it is needed to stop recursion (which currently
> > isn't prevented in the repl code at all).
> 
> I somehow kind of like that one. :-)
> 
> I'd be happy to review+ it in if it fixes our problems
> for now, but if other, more drs/ad-prone engineers could
> give their 2 cents, I'd be more comfortable. :-)

Yeah, that would be welcome. Something like this is needed,
even if not exactly this code.

Right now there's nothing to stop the loop I mentioned
in the previous comment from causing the server to spin.

> First these comments, and now waiting for my autobuild
> before pushing again... (at 1937(14754)/1951 at 3h11m40s ...)
> fingers crossed...
> 
> Cheers - Michael
> 
> See comment inline below:
> 
> > From a58d4eb3cd62156291468d648d23ff473d0202e8 Mon Sep 17 00:00:00 2001
> > From: Jeremy Allison <jra at samba.org>
> > Date: Thu, 4 Aug 2016 11:09:21 -0700
> > Subject: [PATCH 1/2] s4: repl: Ensure all error paths in
> >  dreplsrv_op_pull_source_get_changes_trigger() are protected with tevent
> >  returns.
> > 
> > Otherwise dreplsrv_op_pull_source_get_changes_trigger() could infinitely recurse.
> > 
> > Signed-off-by: Jeremy Allison <jra at samba.org>
> > ---
> >  source4/dsdb/repl/drepl_out_helpers.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/source4/dsdb/repl/drepl_out_helpers.c b/source4/dsdb/repl/drepl_out_helpers.c
> > index bf8a372..29bb9d5 100644
> > --- a/source4/dsdb/repl/drepl_out_helpers.c
> > +++ b/source4/dsdb/repl/drepl_out_helpers.c
> > @@ -446,6 +446,9 @@ static void dreplsrv_op_pull_source_get_changes_trigger(struct tevent_req *req)
> >  		if (!W_ERROR_IS_OK(werr)) {
> >  			DEBUG(0,(__location__ ": Failed to convert UDV for %s : %s\n",
> >  				 ldb_dn_get_linearized(partition->dn), win_errstr(werr)));
> > +			/* The above error can only be OUT_OF_MEMORY. */
> > +			tevent_req_nterror(req, NT_STATUS_NO_MEMORY);
> > +			return;
> 
> Why not use tevent_req_werror(req, werr) ?

Because I'd forgotten it existed :-). Good catch - thanks !

I'll push this (with the fixup).

> > @@ -470,6 +473,7 @@ static void dreplsrv_op_pull_source_get_changes_trigger(struct tevent_req *req)
> >  		status = dreplsrv_get_gc_partial_attribute_set(service, r, &pas);
> >  		if (!NT_STATUS_IS_OK(status)) {
> >  			DEBUG(0,(__location__ ": Failed to construct GC partial attribute set : %s\n", nt_errstr(status)));
> > +			tevent_req_nterror(req, status);
> >  			return;
> >  		}
> >  		replica_flags &= ~DRSUAPI_DRS_WRIT_REP;
> > @@ -482,6 +486,7 @@ static void dreplsrv_op_pull_source_get_changes_trigger(struct tevent_req *req)
> >  		status = dreplsrv_get_rodc_partial_attribute_set(service, r, &pas, for_schema);
> >  		if (!NT_STATUS_IS_OK(status)) {
> >  			DEBUG(0,(__location__ ": Failed to construct RODC partial attribute set : %s\n", nt_errstr(status)));
> > +			tevent_req_nterror(req, status);
> >  			return;
> >  		}
> >  		replica_flags &= ~DRSUAPI_DRS_WRIT_REP;
> > -- 
> > 2.8.0.rc3.226.g39d4020
> 
> Apart from the question above, reviewed-by: me





More information about the samba-technical mailing list