[patches] upgradeprovision

Tue Feb 23 06:58:31 MST 2010

Hi Jelmer, and all !

 From the two patches I've  5 patches:

0001-upgradeprovision-split-the-big-script-to-put-reusabl.patch
0002-upgradeprovision-code-cleanup.patch
0003-upgrade-provision-change-the-meaning-of-handle_secur.patch
0004-upgradeprovision-Allow-script-to-be-called-with-pydo.patch
0005-upgradeprovision-Move-to-pythondoc-format.patch

The first one is to split upgradeprovision so that some function are now 
reusable (I use them for a personnal script that allow me to regenerate 
a new provision with the same parameters as the previous one but without 
the hassle to search for the different value, maybe it can be added in 
script/devel ?)
The second one is a general code cleanup, it takes care of your remarks 
on the useless comments and on the redirection of fd 2
The third is a more specific cleanup on a function so that the behavior 
is simmilar to another one in upgradeprovision
The forth one allow upgradeprovision to be used with pydoc
The fifth is for using "pydoc" format (""" strings).

Let me know.

> 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: 0003-upgrade-provision-change-the-meaning-of-handle_secur.patch
Type: text/x-patch
Size: 1690 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100223/cd54d7db/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-upgradeprovision-split-the-big-script-to-put-reusabl.patch
Type: text/x-patch
Size: 29797 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100223/cd54d7db/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-upgradeprovision-Move-to-pythondoc-format.patch
Type: text/x-patch
Size: 21944 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100223/cd54d7db/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-upgradeprovision-Allow-script-to-be-called-with-pydo.patch
Type: text/x-patch
Size: 4699 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100223/cd54d7db/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-upgradeprovision-code-cleanup.patch
Type: text/x-patch
Size: 18493 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20100223/cd54d7db/attachment-0004.bin>