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

Jelmer Vernooij jelmer at jelmer.uk
Thu Jun 4 09:16:35 MDT 2015


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. 

> >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).

> >>@@ -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.

> >>+        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:" ?)

> >>+                    #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.

> >>-        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)

Cheers,

Jelmer


More information about the samba-technical mailing list