[PATCH] make samba-tool aware of all 7 fsmo roles

[Rowland Penny]

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