[PATCH] pytalloc and pidl changes for refcounting and incorrect msDS-IntID handling

[Andrew Bartlett]

On Sat, 2016-02-27 at 07:26 +1300, Andrew Bartlett wrote:
> On Fri, 2016-02-26 at 17:57 +0100, Stefan Metzmacher wrote:
> > Am 25.02.2016 um 02:38 schrieb Andrew Bartlett:
> > > On Tue, 2016-02-23 at 16:19 +1300, Andrew Bartlett wrote:
> > > > On Mon, 2016-02-22 at 17:54 +0100, Stefan Metzmacher wrote:
> > > > > Hi Andrew,
> > > > >  
> > > > > I like the typechecking on pytalloc_get_mem_ctx() and
> > > > > pytalloc_get_ptr(),
> > > > > which means this is mostly transparent, which simplifies a
> > > > > lot.
> > > > 
> > > > Thanks.  I was worried what you might think of that.
> > > > 
> > > > > I think we can even avoid having a pytalloc_get_ptr_mem_ctx()
> > > > > function
> > > > > and just let pytalloc_get_mem_ctx() return the talloc_ptr_ctx
> > > > > for
> > > > > a BaseObject.
> > > > 
> > > > Done.  I've also removed an extra memory context we didn't
> > > > need.
> > > > 
> > > > > I've also some more comments I'll write within the next few
> > > > > days.
> > > > > 
> > > > 
> > > > Great.  I'm continuing to update my replMetaData-attid branch
> > > > on
> > > > git://
> > > > git.catalyst.net.nz/samba.git with the patches as I improve
> > > > things.
> > > > 
> > > > Attached are the improved patches for pytalloc (other patches
> > > > for
> > > > msDS-
> > > > IntID in the branch).
> > > > 
> > > > Jelmer,
> > > > 
> > > > One thing I noticed is that talloc_guide.txt and the code seem
> > > > to
> > > > disagree on if Py_DECREF(str) is needed on the type pointer
> > > > from
> > > > pytalloc_GetObjectType().  I've kept the behaviour consistent
> > > > with
> > > > the
> > > > existing code, but it seems wrong. 
> > > > 
> > > > This may be our chance to fix (and make more complex...) this
> > > > code,
> > > > or
> > > > the docs.
> > > 
> > > I've updated the replMetaData-attid branch, fixing and clarifying
> > > the
> > > pidl changes.  There are less changes to pidl thanks to your
> > > suggestion
> > > to just change what pytalloc_get_mem_ctx() returns. 
> > > 
> > > I look forward to your review,
> > 
> > Can you have a look at the diff between
> > http://git.catalyst.net.nz/gw?p=samba.git;a=shortlog;h=refs/heads/r
> > ep
> > lMetaData-attid
> > and
> > https://git.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/head
> > s/
> > master4-attid
> 
> I'll look at that next week.
> 
> > I think
> > http://git.catalyst.net.nz/gw?p=samba.git;a=commitdiff;h=01e1a173bf
> > 13
> > 83b71be08d9b1ee40ea1f8bc9ba7
> > can be skipped for now see
> > https://git.samba.org/?p=metze/samba/wip.git;a=commit;h=6a363243a79
> > f2
> > 291a734fd1cc5b288ddc1c2425c
> > in order to avoid problems with backporting large patches, if split
> > into multiple patches it should be fine for master (only).
> 
> OK.
> 
> > If you're fine with my changes, I'll review the non talloc changes
> > and
> > push tomorrow.
> > 
> > Somehow a private autobuild (with my branch fails :-( )
> 
> Yes, I see the same on an autobuild I started similarly on our
> cloud. 
>  I'm not sure if the late removal of an intermediate mem_ctx did it,
> but clearly the tests in array.py are not a good enough simulation.

I've fixed the main issue here (a genuine error in the calculation of
incorrect attid values), but I don't have any idea on what the segfault
is.  It may well be an existing unrelated issue, that we haven't
figured out or accidentally fixed in the PIDL changes.

I've pushed a branch called master4-attid to the catalyst repo with
what I hope you can agree with, understanding that our mutual puzzle on
the segfault remains.  I am running a number of builds on the Catalyst
cloud with this and my original branch, and hopefully I get some idea
past the dozens of other flapping tests.

Andrew Bartlett

-- 
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT   
https://catalyst.net.nz/services/samba