Your Tombstone Reanimation branch
Kamen Mazdrashki
kamenim at samba.org
Thu Dec 18 19:36:43 MST 2014
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
>
>
>
>
>
More information about the samba-technical
mailing list