[PATCH] Do not deleted the deleted objects container, and never write out a 0000-0000... GUID

[Andrew Bartlett]

On Thu, 2013-09-19 at 10:35 -0700, Matthieu Patou wrote:
> Hi Andrew,
> 
> Few remarks :
> 
> There is a couple of white space here and there:
> 
> 
> Applying: python/drs: Ensure to pass in the local invocationID during 
> the domain join
> /usr/local/src/samba/.git/rebase-apply/patch:148: trailing whitespace.
> 
> warning: 1 line applied after fixing whitespace errors.
> Applying: dsdb-repl_meta_data: Check for a NULL invocationID and do not 
> proceed
> /usr/local/src/samba/.git/rebase-apply/patch:15: trailing whitespace.
>          return replmd_replicated_request_werror(ar, 
> WERR_DS_DRA_INTERNAL_ERROR);
> warning: 1 line applied after fixing whitespace errors.
> Applying: dsdb: Refuse to return an all-zero invocationID
> /usr/local/src/samba/.git/rebase-apply/patch:15: trailing whitespace.
>              DEBUG(0, ("Failed to find our own NTDS Settings 
> invocationId in the ldb!\n"));
> warning: 1 line applied after fixing whitespace errors.
> Applying: dsdb-repl_meta_data: Do not re-delete the Deleted Objects DN 
> during replication
> Applying: dsdb-repl_meta_data: Make handling of Deleted Objects DN 
> clearer in delete
> Applying: lib/messaging: Check the GUID type correctly
> 
> --- a/source4/dsdb/common/util.c
> +++ b/source4/dsdb/common/util.c
> @@ -1302,6 +1302,7 @@ const struct GUID *samdb_ntds_invocation_id(struct ldb_context *ldb)
>   	/* see if we have a cached copy */
>   	invocation_id = (struct GUID *)ldb_get_opaque(ldb, "cache.invocation_id");
>   	if (invocation_id) {
> +		SMB_ASSERT(!GUID_all_zero(invocation_id));
>   		return invocation_id;
>   	}
> 
> 
> Do we really want to have an assert here ?
> If you have systems in the wild with this, samba will suddenly stop work 
> for them.

Yes, we do.  This is for our own invocation ID, as read from the
database.  I have then modified the calling functions to simply decline
to set a all-zero invocation ID. 

> Also in Patch 4 you moved some code that was in replmd_delete_internals, 
> which is fine but can you also move the comments explaining why.
> I attached a small variation on your patch to this email.
> 
> Apart from this minor remarks, it's ok.
> Also I think we should check that when we replicate that we don't have a 
> replication coming with NULL invocation id (0000-000...) for every 
> single attribute.

I'm pending the reply from Microsoft on exactly what check is done
where, so we can be as strict as they are. 

I'm sorry I didn't address your comments before I pushed, I got a review
from metze and didn't notice your mail.  Regardless, let us rework your
patch into a new one and get that also into master, to ensure we don't
forget this hard lesson any time soon. 

Thanks,

Andrew Bartlett


-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org