Your Tombstone Reanimation branch

[Kamen Mazdrashki]

Hi Andrew,

On Thu, Dec 18, 2014 at 6:00 AM, Andrew Bartlett <abartlet at samba.org> wrote:
>
> On Tue, 2014-12-16 at 04:06 +0200, Kamen Mazdrashki wrote:
> > Hi Andrew,
> >
> > On Tue, Dec 16, 2014 at 12:47 AM, Andrew Bartlett <abartlet at samba.org>
> > wrote:
> > >
> > > On Tue, 2014-12-16 at 09:25 +1300, Andrew Bartlett wrote:
> > > > On Mon, 2014-12-15 at 17:46 +1300, Andrew Bartlett wrote:
> > > > > Kamen,
> > > > >
> > > > > Garming and I did some work to review your
> tombstone-reanimation-wip
> > > > > brnach.  I've pushed it back to
> > > > >
> > >
> https://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/tombstone-reanimation-wip
> > > with some TODO markers (metze style).
> > > > >
> > > >
> > > > > We need to go over it again, and to run the tests, but it is an
> > > > > impressive lump of work and I look forward to seeing it landed!
> > > >
> > > > I've run a full make test, and the ldap.secdesc.python test fails.  I
> > > > attach the summary file, and the below is the failing subunit.  Can
> you
> > > > look into this please?
> > >
> > > Kamen,
> > >
> > > The attached patch fixes the issue.  See the markers in the branch.
> > >
> > > I'm now re-running a full make test.
> > >
> > > Nadya,
> > >
> > > Some of the patches need you to sign off on them again, can you look
> > > over:
> > >
> > >
> > >
> https://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/tombstone-reanimation-wip
> > >
> > >
> > Thank you very much for your review and fixing the 'make test' issue!
> > I am building a dev machine (kind of) so tomorrow, hopefully, I will be
> > able to address your comments.
> >
> > As a stretch goal, I will address your concerns for env variable names.
> > As you suggested, I will prefix env variables used by tests with TEST_
>
> Garming and I have been looking over the branch again, and between us we
> had a few more concerns.
>

Thanks for your thorough review


>
> One that both of us were a little bothered by, was this commit:
>
> https://git.samba.org/?p=abartlet/samba.git/.git;a=commitdiff;h=dda27fc7dbc69b9203caf57cfa1a7829164e8db5
>
> This is where we have a copy of the code from samldb.  As I think we
> discussed much earlier, this really isn't ideal.  My concerns additional
> to that are particularly around the userAccountControl -> samAccountType
> -> primaryGroupID logic.  This really needs to be kept in one place, in
> samldb.
>

yes, this is copy (more or less I think) from samldb_add handler.
What we have discussed actually was about objectCategory :)
I will look into this one too (can't recall right now why I opted for
copy&paste&tune
approach rather than common function)


>
> I'm also not really comfortable with:
>
> https://git.samba.org/?p=abartlet/samba.git/.git;a=commitdiff;h=7dfc15b54978ed006a98790db8126d5131dcc3b1
>
> This hunk skips all validation of the primaryGroupID, including if the
> group still exists.  Now, the system groups implied by the
> userAccountControl should not go away, but I don't like just skipping
> checks like this.
> -       el = ldb_msg_find_element(ac->msg, "primaryGroupID");
> -       if (el != NULL) {
> -               ret = samldb_prim_group_trigger(ac);
> -               if (ret != LDB_SUCCESS) {
> -                       return ret;
> +       if (is_undelete == NULL) {
> +               el = ldb_msg_find_element(ac->msg, "primaryGroupID");
> +               if (el != NULL) {
> +                       ret = samldb_prim_group_trigger(ac);
> +                       if (ret != LDB_SUCCESS) {
> +                               return ret;
> +                       }
>                 }
>
> Instead, it should take the ADD arm of samldb_prim_group_trigger (as
> this is what it really looks like to the DB), and at least confirm the
> group still exists with samldb_prim_group_set().
>

I will look into this one too.
As far as I recall, I skipped this because primaryGroupID already exists.
Haven't thought about this group being non-existent anymore - thanks for
pointing that out.
I wonder what the result would be out of a test to verify reanimation
of object with non-existent primaryGroupID


>
> Finally, what do you intend to do with _attributes_for_class in your
> tombstone_reanimate.py?  It looks like the parts of a test you were
> describing early-on with a grand plan for checking each possible
> attribute, but it is all un-used right now.  If you want it to be a
> utility for schema validation or such, perhaps put it in another file?
>
> Yes, you are right. This is a leftover. I will remove it. Perhaps I left
it there
because it actually works :). But it is not used anyway. Sorry for the noise

Thanks,
kamen


> Thanks,
>
> Andrew Bartlett
>
> --
> Andrew Bartlett
> http://samba.org/~abartlet/
> Authentication Developer, Samba Team  http://samba.org
> Samba Developer, Catalyst IT
> http://catalyst.net.nz/services/samba
>
>
>
>
>