[patches] upgradeprovision

[Jelmer Vernooij]

Hi Matthieu,

On Mon, 2010-02-01 at 02:03 +0300, Matthieu Patou wrote:
> I reattached the patches that tried (and achieved I hope) to take care 
> of all your remarks.
Thanks! Here's another review; I'll happily merge after my comments
below are addressed.

Please use """-style docstrings at the top of functions rather than
adding comments right above the function, as is the convenience. In
addition the docstrings are shown by command-line API browsing tools
like pydoc as well as the various tools that can generate HTML API
documentation.

> >> +import os
> >> +import sys
> >> +import string
> >> +import re
> >> +# Find right directory when running from source tree
> >>      
> > ^^^ This comment doesn't seem true anymore
> >    
> Ok I'll take your remarks
You seem to've changed the comment in bin/upgradeprovision. However, my
review comment was about the comment in upgradehelpers.py which is still
there after the related "sys.path.insert()" line was (correctly)
removed.

> >> +# Create a fresh new reference provision
> >> +# This provision will be the reference for knowing what has changed in the
> >> +# since the latest upgrade in the current provision
> >> +def newprovision(names,setup_dir,creds,session,smbconf,provdir,messagefunc):
> >> +	if os.path.isdir(provdir):
> >> +		rmall(provdir)
> >> +	logstd=os.path.join(provdir,"log.std")
> >> +	os.chdir(os.path.join(setup_dir,".."))
> >> +	os.mkdir(provdir)
> >> +	os.close(2)
> >> +	sys.stderr = open("%s/provision.log"%provdir, "w")
> >>      
> > ^^^ please don't do this sort of stuff. instead, pass in a function that
> > 	can be used for printing debug output or a file handle that should
> > 	be used for loggin.
> >    
> Well the problem is that provision is printing on the output and it 
> didn't seems to be easily changeable.
> Here are the strings output:
> pdc_fsmo_init: no domain object present: (skip loading of domain details)
> 
> naming_fsmo_init: no partitions dn present: (skip loading of naming 
> contexts details)
> 
> naming_fsmo_init: no partitions dn present: (skip loading of naming 
> contexts details)
> 
> they are due to this kind of code in dsdb/samdb/ldb_modules/naming_fsmo.c
>                  ldb_debug(ldb, LDB_DEBUG_WARNING,
>                            "naming_fsmo_init: no partitions dn present: 
> (skip loading of naming contexts details)\n");
> 
> So if there is a way to make ldb_debug quiet then I can easily remove 
> this part.
I think we should work on making the ldb naming_fsmo module quiet, but
we should do that by fixing the module rather than by adding a nasty
hack here to silence those warnings. 

I'm not familiar with that part of the ldb code, but we should make sure
it either doesn't warn unnecessarily (perhaps downgrade the log level to
'debug' ?) or if the warning is correct we should make sure we fix the
issue.

> >> +# This function sorts two DNs in the lexicographical order and put higher level
> >> +# DN before.
> >> +# So given the dns cn=bar,cn=foo and cn=foo the later will be return as smaller
> >> +# (-1) as it has less level
> >>      
> > ^^^ You should be able to use sort() with dn's, since we expose
> > 	ldb_dn_cmp() at the Python level. Have you tried that?
> >    
> Yes I checked with simo and andrew B. once on irc it didn't do the job.
> See the demo script attached.
I've just verified that we are indeed calling ldb_dn_compare() under the
hood, so perhaps it would make sense to adjust that. 

Simo, do you think it would make sense for ldb_dn_compare() to sort
parent dn's before their children ?

Cheers,

Jelmer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100204/0a73167b/attachment.pgp>