[PATCH] make samba-tool aware of all 7 fsmo roles
Rowland Penny
repenny241155 at gmail.com
Thu Jun 4 10:49:14 MDT 2015
On 04/06/15 17:17, Rowland Penny wrote:
> On 04/06/15 16:16, Jelmer Vernooij wrote:
>> On Thu, Jun 04, 2015 at 02:00:47PM +0100, Rowland Penny wrote:
>>> On 04/06/15 13:23, Jelmer Vernooij wrote:
>>>> On Thu, Jun 04, 2015 at 11:36:29AM +0100, Rowland Penny wrote:
>>>>> On 03/06/15 00:53, Jelmer Vernooij wrote:
>>>>> OK, hopefully the attached patch addresses all the problems
>>>>> pointed out by
>>>>> Jelmer (but I not holding my breath ;-) ), that is apart from the
>>>>> lining up
>>>>> of the new lines, I have left these in because, on my terminal, it
>>>>> makes it
>>>>> easier to read.
>>>> Thanks! Here is another round of comments, just a few this time.
>>> I knew I shouldn't hold my breath :-D
>> :) I hope this helps; please do let me know if I should clarify
>> comments, and thanks for
>> your patience.
>
> No problem, only way to learn.
>
>>>> Please do fix the newlines. Consistency in coding style matters; we
>>>> don't
>>>> line up newlines anywhere else in our codebase. See also
>>>> https://www.python.org/dev/peps/pep-0008/#pet-peeves
>>> Will do, even though it makes the output a bit weird, I would have
>>> thought
>>> readability would outweigh coding style.
>> Different people have different opinions as to what is more readable.
>> :-)
>> The idea of the coding style is that we're consistent, so the
>> formatting is
>> predictable (and you can get used to it).
>
> OK, I can understand that.
>
>>
>>>>> @@ -119,26 +250,78 @@ all=all of the above"""),
>>>>> else:
>>>>> raise CommandError("Invalid FSMO role.")
>>>>> #first try to transfer to avoid problem if the owner is
>>>>> still active
>>>>> - if force is None:
>>>>> - self.message("Attempting transfer...")
>>>>> - try:
>>>>> - transfer_role(self.outf, role, samdb)
>>>>> - self.outf.write("FSMO seize was not required, as
>>>>> transfer of '%s' role was successful\n" % role)
>>>>> - return
>>>>> - except CommandError:
>>>>> - #transfer failed, use the big axe...
>>>>> + if master_owner != serviceName:
>>>>> + if force is None:
>>>>> + self.message("Attempting transfer...")
>>>>> + try:
>>>>> + transfer_role(self.outf, role, samdb)
>>>>> + return
>>>>> + except:
>>>>> + #transfer failed, use the big axe...
>>>> ^^ Please don't use "except:" unless you're going to re-raise the
>>>> exception. It looks like the indentation here is off,
>>>> self.message() has the same indentation as except:?
>>> Jelmer, I would like to point out that I am still very much learning
>>> python
>>> and that block of code is based on what is in the original fsmo.py:
>>>
>>> if force is None:
>>> self.message("Attempting transfer...")
>>> try:
>>> transfer_role(self.outf, role, samdb)
>>> self.outf.write("FSMO seize was not required, as
>>> transfer of
>>> '%s' role was successful\n" % role)
>>> return
>>> except CommandError:
>>> #transfer failed, use the big axe...
>>>
>>> And at the top of fsmo.py is this:
>>>
>>> # Changes a FSMO role owner
>>> #
>>> # Copyright Nadezhda Ivanova 2009
>>> # Copyright Jelmer Vernooij 2009
>>>
>>> So I will change it, but I hope you can see where I am coming from :-)
>> Unfortunately the copyright headers often don't reflect who
>> contributed to a
>> file - people forget to add their name or add it even though they've
>> just fixed a
>> typo or two. I don't think I ever worked on the fsmo code, but it may
>> have
>> originally been based on a file that I wrote.
>
> OK, but I think you understand why I used the code the way I did .
>
>
>>
>>>>> + if master_owner != serviceName:
>>>>> + if force is None:
>>>>> + self.message("Attempting transfer...")
>>>>> + try:
>>>>> + transfer_dns_role(self.outf, sambaopts,
>>>>> credopts, role,
>>>>> + samdb)
>>>>> + #self.outf.write("Transfer of '%s' role was
>>>>> successful\n" %
>>>>> + # role)
>>>>> + return
>>>>> + except:
>>>> ^^ Please catch a specific exception.
>>> not sure how, any chance of a hint?
>> Do you mean what kind of exception to catch? What kind of exception
>> would you expect to be
>> raised here? (in other words, why did you put in the "except:" ?)
>
> Well from what I know already (not much, I will admit) I thought that
> if it failed it would do something else, a bit like if - then - else
> in bash.
>
>>
>>>>> + #transfer failed, use the big axe...
>>>>> + self.message("Transfer unsuccessful,
>>>>> seizing...")
>>>> ^^ transfer_dns_role() doesn't necessarily raise an exception when
>>>> it fails.
>>> Does it matter? the code has tried to transfer the role and failed,
>>> it then
>>> goes on to seize the role, the code is also based on the original
>>> transfer_role() code
>> The code you based it on probably isn't perfect, but we shouldn't
>> allow more instances of
>> the bad patterns to be introduced (lest more people copy it, etc).
>>
>> transfer_dns_role() should be consistent in how it deals with errors
>> - either it should
>> raise an exception if it fails (which I would prefer), or it should
>> print to standard out and
>> return - not a combination of both.
>
> OK, I understand better now, I will go away and try and sort it.
>
>>
>>>>> - assert len(res) == 1
>>>>> - self.ridMaster = res[0]["fSMORoleOwner"][0]
>>>>> -
>>>>> - self.message("InfrastructureMasterRole owner: " +
>>>>> self.infrastructureMaster)
>>>>> - self.message("RidAllocationMasterRole owner: " +
>>>>> self.ridMaster)
>>>>> - self.message("PdcEmulationMasterRole owner: " +
>>>>> self.pdcEmulator)
>>>>> - self.message("DomainNamingMasterRole owner: " +
>>>>> self.namingMaster)
>>>>> - self.message("SchemaMasterRole owner: " + self.schemaMaster)
>>>>> + forest_dn = "DC=" + samdb.forest_dns_name().replace(".",
>>>>> ",DC=")
>>>> ^^ Hmm, do we not have a standard function for determining a dn
>>>> from a DNS name, or a way to get the forest DN?
>>> Couldn't find one.
>> Andrew, do you know of one?
>>
>> If there isn't one, it would be good to introduce one. (That's just a
>> side-note, I
>> don't think that should block this patch set)
>
> If I did create one, where should I create it, inside fsmo.py or
> samdb.py or somewhere else.
>
> Rowland
>
>>
>> Cheers,
>>
>> Jelmer
>
OK, so I had a look at this:
>>> + if master_owner != serviceName:
>>> + if force is None:
>>> + self.message("Attempting transfer...")
>>> + try:
>>> + transfer_dns_role(self.outf, sambaopts,
credopts, role,
>>> + samdb)
>>> + #self.outf.write("Transfer of '%s' role was
successful\n" %
>>> + # role)
>>> + return
>>> + except:
I now think we were discussing the wrong thing :-D
I have re-written it to this:
if master_owner != serviceName:
if force is None:
self.message("Attempting transfer...")
try:
#Any Error msg will be printed by 'transfer_role'
transfer_role(self.outf, sambaopts, credopts,
role,
samdb)
return
So when it goes to 'transfer_role' it ends up running this:
if master_owner != new_owner:
try:
samdb.modify(m)
except LdbError, (num, msg):
raise CommandError("Transfer of '%s' role failed: %s" %
(role, msg))
If the transfer fails, the reason why it fails is printed, is this
sufficient or do I need to do something else ?
Rowland
More information about the samba-technical
mailing list