[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