review of your master branch

Wed Mar 9 22:39:19 MST 2011

Hi Matthias,

Sorry again for being so slow in reviewing your changes. Here are some
comments:

    commit f8002261ff36a2cf70560765ffb9066a524445ed
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sun Mar 6 17:46:08 2011 +0100

	s4:setup/provision - fix an output message

	Mention that Windows 2000 function level is supported as well.

yep, looks good

    commit 81bd4ddcbc1b090d4b754f1c145ffab433eec275
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sat Mar 5 14:15:38 2011 +0100

	s4:partition LDB module - fix typo

looks good    

    commit 712832285e0027e286eefd50d56a0b5425e97ed8
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sun Mar 6 15:51:12 2011 +0100

	s4:partition_init LDB module - fix a typo

looks good    

    commit ef43208c67ae5b4fb74bc99282f58a2fcc0c916b
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sat Mar 5 17:15:19 2011 +0100

	s4:new_partition LDB module - fix comments

looks good    

    commit 4fb4fb88525eb60a39e770e562b54f90b81f1743
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sun Mar 6 15:55:45 2011 +0100

	s4:partition LDB module - add some comments

looks good    

    commit 941479cd65c3091166bc471d59894ae0bd921a87
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sun Mar 6 20:16:43 2011 +0100

	ldb:ldb_controls.c - always allocate enough space

	The size for an additional "struct ldb_control" shouldn't hurt and so
	the excluded control can also be NULL.

ok. I don't really understand why the old code worked the way it
did, but your change looks sensible to me. The only additional
change I'd recommend is a talloc_realloc() at the end, so that the
talloc size of the array is correct. It is good to keep that
accurate, as it helps with debugging.

    commit cbd21b6459979072aec66924f38902f7a4fb6086
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sat Mar 5 18:45:08 2011 +0100

	s4:partition LDB module - fill in parent requests for inheriting the flags

	Probably it doesn't matter in this cases but just for consistency.

looks good, assuming it passes tests (sometimes we have subtle
code that relies on not inheriting flags)

    commit 23557e771b7218f288303389e82854435ce980d7
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sat Mar 5 16:59:02 2011 +0100

	s4:instancetype LDB module - use "ldb" pointer for referencing the LDB context

looks good    

    commit e0a52643b2a31b4878ea23deb02d74db82232ef7
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sat Mar 5 17:28:13 2011 +0100

	s4:instancetype LDB module - don't impede control requests

looks good    

    commit ff08173fce7e7f991c0c8a23c8de71de89698c0f
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sat Mar 5 17:24:10 2011 +0100

	s4:instancetype LDB module - perform here only the "instanceType" constraint checks

	The boilerplate entries (when this support has been implemented) should
	be provided by the "new_partition" LDB module. These are for example the
	deleted object and lost and found container.

I agree that this makes more sense. It doesn't look like we ever create the
boilerplate entries at the moment anyway.

    commit 442585ba41f229a34fe71cee61fec92da18f428c
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sat Mar 5 17:15:55 2011 +0100

	s4:new_partition LDB module - uninstantiated partitions are handled by the second "if"

	Therefore move the first "if" a bit lower - so the preceding rules
	should fit.

do we have a test for this at the moment? If we do, then this patch is fine    

    commit 39415055d0701a25ba3dcb7855525e682273646e
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sat Mar 5 20:38:14 2011 +0100

	s4:repl_meta_data LDB module - remove the current partition control unless it was requested

ok, that makes sense.    

    commit cfdfb0bef33f0b16c4247356deed64c9faac5ea3
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sat Mar 5 13:56:10 2011 +0100

	s4:repl_meta_data LDB module - don't remove the partition control twice

	"controls" is already the controls list which has the partition control
	removed. It is generated by "ldb_controls_except_specified" in line 378.

looks good

    commit a2029128fc7db418624ca1b387cc560c0469c3d2
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sat Mar 5 14:52:18 2011 +0100

	s4:simple_ldap_map LDB module - enhance current partition control checks

	Don't stop the server if it hasn't been filled in correctly. An LDB
	error should be enough.

looks OK, except LDB_ERR_CONSTRAINT_VIOLATION may not be the right
error code. Maybe LDB_ERR_PROTOCOL_ERROR? The debug should make it clear anyway.

    commit 3022527847d4002c6f4bb96ac2b654776e9cb079
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sat Mar 5 19:02:43 2011 +0100

	s4:partition LDB module - "partition_sequence_number" - remove meaningless "if"s

	These current partition controls are always added for the two EXOP operations.

yep, looks good (we'll get LDB_ERR_ATTRIBUTE_OR_VALUE_EXISTS if the control is there already)

    commit 48687abe20fffdfd8940cf3ac404fe0566178b88
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sun Mar 6 15:37:51 2011 +0100

	s4:partition LDB module - "partition_replicate" doesn't handle the search requests

	That is done by "partition_search".

looks good

    commit 1f588a049089741f006913c0bd7e1e0e6f7311b6
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sun Mar 6 15:56:06 2011 +0100

	s4:partition LDB module - move the "data" check a bit higher

	It can be performed a bit earlier.

looks good

    commit df8aab638e297373b3418da66a7eb16d1c46bd4c
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sun Mar 6 15:56:48 2011 +0100

	s4:partition LDB module - extended operations - make the initialisation check consistent

	To the other operations.

looks good

    commit aec07f0143d9b2864aafa8aea96ccea72629e5d1
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sat Mar 5 14:25:59 2011 +0100

	s4:partition LDB module - the current partition control should only be added if requested

	That means if the informations before a request are unknown
	("repl_meta_data" LDB module) then an empty control (no data) has to be sent.

it doesn't look like we ever use a saved_controls value after getting it from ldb_save_controls(),
so we should probably make that 3rd argument optional, and allow NULL (its in our API, so I guess
we shouldn't just remove it).

In reviewing this, I'm having some difficulty in seeing exactly how
this control is used in repl_meta_data.c. Our tests should be OK for
this, but would you mind running a replication test under valgrind
after this change to ensure we haven't introduced anything subtle.

    commit ab4458f74a6f490c22be3cd5d50b697ab7a96622
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sat Mar 5 14:42:40 2011 +0100

	s4:repl_meta_data LDB module - always ask manually for the current partition control

	Otherwise the "partition" LDB module doesn't give it back anymore.

this is also OK if valgrind doesn't complain :-)

    commit 503bf3ef67b7006705232b99fa0323e7a38a8745
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Sun Mar 6 21:46:02 2011 +0100

	s4:extended_dn_store LDB module - use the new request as generic memory contexts

	To prevent memory leaks.

I don't see how this prevents memory leaks. I think the change is harmless, but I'm 
curious what leak you had in mind

    commit 926a89cad3267d26087a916dce9cfe30e97ea26d
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Wed Mar 9 09:09:15 2011 +0100

	lib/util/fault.c - "call_backtrace" - no need to have "backtrace_size" as size_t

	The function "backtrace" returns an "int".

looks good

    commit 5f8436247edc34628eae25e9892cd1a055f37a83
    Author: Matthias Dieter Wallnöfer <mdw at samba.org>
    Date:   Wed Mar 9 09:29:30 2011 +0100

	s4:lib/tls/wscript - exclude known broken GNUTLS releases

	This definitely fixes bug #7218.

looks good

Thanks for all the good work!

Cheers, Tridge