[PATCH] some improvements to samba-tool visualize and miscellaneous other things

Douglas Bagnall douglas.bagnall at catalyst.net.nz
Wed May 30 23:52:54 UTC 2018


On 31/05/18 11:10, Douglas Bagnall via samba-technical wrote:
> If you don't like these please say so quickly because Andrew's autobuild
> finger is twitching.
> 
> Some of this is prelimnary work for a more human readable samba-tool drs
> showrepl output.

This time with patches.


-------------- next part --------------
From cfed328c153f8f8c58a3438d91307db3c898e1df Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Fri, 23 Feb 2018 11:11:27 +1300
Subject: [PATCH 01/25] samba-tool visualize: group (and colour) DCs by site

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/graph.py            | 29 ++++++++++++++++++++++-------
 python/samba/netcmd/visualize.py | 17 +++++++++++++++--
 2 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/python/samba/graph.py b/python/samba/graph.py
index 7dfc19015e7..7d195e3574a 100644
--- a/python/samba/graph.py
+++ b/python/samba/graph.py
@@ -22,6 +22,7 @@ from __future__ import print_function
 from __future__ import division
 from samba import colour
 import sys
+from itertools import cycle, groupby
 
 FONT_SIZE = 10
 
@@ -511,7 +512,8 @@ def distance_matrix(vertices, edges,
                     utf8=False,
                     colour=None,
                     shorten_names=False,
-                    generate_key=False):
+                    generate_key=False,
+                    grouping_function=None):
     lines = []
     write = lines.append
 
@@ -528,9 +530,22 @@ def distance_matrix(vertices, edges,
 
     colours = COLOUR_SETS[colour]
 
+    colour_cycle = cycle(colours.get('alternate rows', ('',)))
+
     if vertices is None:
         vertices = sorted(set(x[0] for x in edges) | set(x[1] for x in edges))
 
+    if grouping_function is not None:
+        # we sort and colour according to the grouping function
+        # which can be used to e.g. alternate colours by site.
+        vertices = sorted(vertices, key=grouping_function)
+        colour_list = []
+        for k, v in groupby(vertices, key=grouping_function):
+            c = next(colour_cycle)
+            colour_list.extend(c for x in v)
+    else:
+        colour_list = [next(colour_cycle) for v in vertices]
+
     if shorten_names:
         edges, vertices, replacements = shorten_vertex_names(edges,
                                                              vertices,
@@ -540,7 +555,6 @@ def distance_matrix(vertices, edges,
     vlen = max(6, max(len(v) for v in vertices))
 
     # first, the key for the columns
-    colour_cycle = colours.get('alternate rows', ('',))
     c_header = colours.get('header', '')
     c_disconn = colours.get('disconnected', '')
     c_conn = colours.get('connected', '')
@@ -556,7 +570,7 @@ def distance_matrix(vertices, edges,
                                        c_reset))
     for i, v in enumerate(vertices):
         j = len(vertices) - i
-        c = colour_cycle[i % len(colour_cycle)]
+        c = colour_list[i]
         if j == 1:
             start = '%s%ssource%s' % (vspace[:-6], c_header, c_reset)
         else:
@@ -575,7 +589,7 @@ def distance_matrix(vertices, edges,
     connections = find_transitive_distance(vertices, edges)
 
     for i, v in enumerate(vertices):
-        c = colour_cycle[i % len(colour_cycle)]
+        c = colour_list[i]
         links = connections[v]
         row = []
         for v2 in vertices:
@@ -596,13 +610,14 @@ def distance_matrix(vertices, edges,
         write('%s%*s%s %s%s' % (c, vlen, v, c_reset,
                                 ''.join(row), c_reset))
 
+    example_c = next(colour_cycle)
     if shorten_names:
         write('')
         for substitute, original in reversed(replacements):
-            write("'%s%s%s' stands for '%s%s%s'" % (colour_cycle[0],
+            write("'%s%s%s' stands for '%s%s%s'" % (example_c,
                                                     substitute,
                                                     c_reset,
-                                                    colour_cycle[0],
+                                                    example_c,
                                                     original,
                                                     c_reset))
     if generate_key:
@@ -611,7 +626,7 @@ def distance_matrix(vertices, edges,
               "indicated number of steps." % (c_header, c_reset,
                                               c_header, c_reset))
         write("%s%s%s means zero steps (it is the same DC)" %
-              (colour_cycle[0], diagonal, c_reset))
+              (example_c, diagonal, c_reset))
         write("%s1%s means a direct link" % (c_conn, c_reset))
         write("%s2%s means a transitive link involving two steps "
               "(i.e. one intermediate DC)" %
diff --git a/python/samba/netcmd/visualize.py b/python/samba/netcmd/visualize.py
index b9d126a8cb3..311476a60ef 100644
--- a/python/samba/netcmd/visualize.py
+++ b/python/samba/netcmd/visualize.py
@@ -32,6 +32,7 @@ from samba.graph import dot_graph
 from samba.graph import distance_matrix, COLOUR_SETS
 from ldb import SCOPE_BASE, SCOPE_SUBTREE, LdbError
 import time
+import re
 from samba.kcc import KCC
 from samba.kcc.kcc_utils import KCCError
 from samba.compat import text_type
@@ -158,6 +159,16 @@ class GraphCommand(Command):
         return color_scheme
 
 
+def get_dnstr_site(dn):
+    """Helper function for sorting and grouping DNs by site, if
+    possible."""
+    m = re.search(r'CN=Servers,CN=\s*([^,]+)\s*,CN=Sites', dn)
+    if m:
+        return m.group(1)
+    # Oh well, let it sort by DN
+    return dn
+
+
 def colour_hash(x):
     """Generate a randomish but consistent darkish colour based on the
     given object."""
@@ -316,7 +327,8 @@ class cmd_reps(GraphCommand):
                                             utf8=utf8,
                                             colour=color_scheme,
                                             shorten_names=shorten_names,
-                                            generate_key=key)
+                                            generate_key=key,
+                                            grouping_function=get_dnstr_site)
 
                         s = "\n%s\n%s" % (header_strings[direction] % part, s)
                         self.write(s, output)
@@ -512,7 +524,8 @@ class cmd_ntdsconn(GraphCommand):
                                 utf8=utf8,
                                 colour=color_scheme,
                                 shorten_names=shorten_names,
-                                generate_key=key)
+                                generate_key=key,
+                                grouping_function=get_dnstr_site)
             self.write('\n%s\n%s\n%s' % (title, s, epilog), output)
             return
 
-- 
2.17.0


From 94a3a537e2b59d0ab36bbcaf5b6e60504d33cc93 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Fri, 23 Feb 2018 11:12:53 +1300
Subject: [PATCH 02/25] samba-tool visualize tests: reduce noise on stdout

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/tests/samba_tool/visualize.py | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/python/samba/tests/samba_tool/visualize.py b/python/samba/tests/samba_tool/visualize.py
index a90c77364b7..c015abf3b2d 100644
--- a/python/samba/tests/samba_tool/visualize.py
+++ b/python/samba/tests/samba_tool/visualize.py
@@ -24,7 +24,7 @@ We don't test samba-tool visualize reps here because repsTo and
 repsFrom are not replicated, and there are actual remote servers to
 query.
 """
-
+from __future__ import print_function
 import samba
 import os
 import re
@@ -289,7 +289,6 @@ class SambaToolVisualizeLdif(SambaToolCmdTest):
                                         self.tempdir,
                                         self.lp, tag='disconnected')
         dburl = 'tdb://' + dbfile
-        print(dbfile)
         result, output, err = self.runsubcmd("visualize", "ntdsconn",
                                              '-H', dburl,
                                              '--color=no', '-S')
@@ -319,8 +318,6 @@ class SambaToolVisualizeLdif(SambaToolCmdTest):
                                           '-o', '-')
         self.assertCmdSuccess(result, dot, err)
         self.remove_files(dbfile)
-        print(dot)
-
         self.assertStringsEqual(EXPECTED_DOT_NTDSCONN_DISCONNECTED, dot,
                                 strip=True)
 
@@ -343,7 +340,6 @@ class SambaToolVisualizeLdif(SambaToolCmdTest):
         self.assertStringsEqual(EXPECTED_DOT_NTDSCONN_DISCONNECTED, dot)
 
         self.remove_files(dbfile, dot_file)
-        print(dot)
 
 EXPECTED_DOT_MULTISITE_NO_KEY = r"""/* generated by samba */
 digraph A_samba_tool_production {
-- 
2.17.0


From 7e20367b9388cc90c4e57ace1ef9bd4f2d6f8b95 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed, 7 Mar 2018 13:55:08 +1300
Subject: [PATCH 03/25] samba-tool visualize ntdsconn: add --importldif option

This visualizes the NTDSConnections in an LDIF file exported via
`samba_kcc --exportldif`. This functionality is already available in a
roundabout way -- you can use `samba_kcc --import_ldif`, and use the
DB that generates. This just shortens the process.

The ldif import/export feature is useful for analysing AD networks
offsite without exposing too much sensitive data.

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/netcmd/visualize.py           | 28 +++++++++++++++++++---
 python/samba/tests/samba_tool/visualize.py | 16 +++++++++++++
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/python/samba/netcmd/visualize.py b/python/samba/netcmd/visualize.py
index 311476a60ef..e66030d3107 100644
--- a/python/samba/netcmd/visualize.py
+++ b/python/samba/netcmd/visualize.py
@@ -33,7 +33,7 @@ from samba.graph import distance_matrix, COLOUR_SETS
 from ldb import SCOPE_BASE, SCOPE_SUBTREE, LdbError
 import time
 import re
-from samba.kcc import KCC
+from samba.kcc import KCC, ldif_import_export
 from samba.kcc.kcc_utils import KCCError
 from samba.compat import text_type
 
@@ -400,13 +400,31 @@ class NTDSConn(object):
 
 class cmd_ntdsconn(GraphCommand):
     "Draw the NTDSConnection graph"
+    takes_options = COMMON_OPTIONS + [
+        Option("--importldif", help="graph from samba_kcc generated ldif",
+               default=None),
+    ]
+
+    def import_ldif_db(self, ldif, lp):
+        d = tempfile.mkdtemp(prefix='samba-tool-visualise')
+        fn = os.path.join(d, 'imported.ldb')
+        self._tmp_fn_to_delete = fn
+        samdb = ldif_import_export.ldif_to_samdb(fn, lp, ldif)
+        return fn
+
     def run(self, H=None, output=None, shorten_names=False,
             key=True, talk_to_remote=False,
             sambaopts=None, credopts=None, versionopts=None,
             color=None, color_scheme=None,
-            utf8=None, format=None):
+            utf8=None, format=None, importldif=None):
+
         lp = sambaopts.get_loadparm()
-        creds = credopts.get_credentials(lp, fallback_machine=True)
+        if importldif is None:
+            creds = credopts.get_credentials(lp, fallback_machine=True)
+        else:
+            creds = None
+            H = self.import_ldif_db(importldif, lp)
+
         local_kcc, dsas = self.get_kcc_and_dsas(H, lp, creds)
         local_dsa_dn = local_kcc.my_dsa_dnstr.split(',', 1)[1]
         vertices = set()
@@ -449,6 +467,10 @@ class cmd_ntdsconn(GraphCommand):
                 attested_edges.append((msg['fromServer'][0],
                                        dest_dn, ntds_dn))
 
+        if importldif and H == self._tmp_fn_to_delete:
+            os.remove(H)
+            os.rmdir(os.path.dirname(H))
+
         # now we overlay all the graphs and generate styles accordingly
         edges = {}
         for src, dest, attester in attested_edges:
diff --git a/python/samba/tests/samba_tool/visualize.py b/python/samba/tests/samba_tool/visualize.py
index c015abf3b2d..1afb210b7bd 100644
--- a/python/samba/tests/samba_tool/visualize.py
+++ b/python/samba/tests/samba_tool/visualize.py
@@ -122,6 +122,22 @@ class SambaToolVisualizeLdif(SambaToolCmdTest):
 
             self.assertStringsEqual(monochrome, uncoloured, strip=True)
 
+    def test_import_ldif(self):
+        """Make sure the samba-tool visualize --importldif option gives the
+        same output as using the externally generated db from the same
+        LDIF."""
+        result, s1, err = self.runsubcmd("visualize", "ntdsconn",
+                                         '-H', self.dburl,
+                                         '--color=no', '-S')
+        self.assertCmdSuccess(result, s1, err)
+
+        result, s2, err = self.runsubcmd("visualize", "ntdsconn",
+                                         '--importldif', MULTISITE_LDIF,
+                                         '--color=no', '-S')
+        self.assertCmdSuccess(result, s2, err)
+
+        self.assertStringsEqual(s1, s2)
+
     def test_output_file(self):
         """Check that writing to a file works, with and without
         --color=auto."""
-- 
2.17.0


From 95c68d54c1cf7d16558d19a050fe645d6723e622 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Thu, 8 Mar 2018 14:29:40 +1300
Subject: [PATCH 04/25] samba-tool viusalize: mark RODCs in distance matrix

RODCs should not be replicating out, which means they look alarming
when they are working properly. We label them as RODCs to reminds users
that no outbound replication is expected.

This results in slightly rejigged output formatting.

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/graph.py                      |  8 ++++-
 python/samba/netcmd/visualize.py           | 39 ++++++++++++++++++----
 python/samba/tests/samba_tool/visualize.py |  6 ++--
 3 files changed, 43 insertions(+), 10 deletions(-)

diff --git a/python/samba/graph.py b/python/samba/graph.py
index 7d195e3574a..a36dc25c2f7 100644
--- a/python/samba/graph.py
+++ b/python/samba/graph.py
@@ -513,7 +513,8 @@ def distance_matrix(vertices, edges,
                     colour=None,
                     shorten_names=False,
                     generate_key=False,
-                    grouping_function=None):
+                    grouping_function=None,
+                    row_comments=None):
     lines = []
     write = lines.append
 
@@ -525,8 +526,10 @@ def distance_matrix(vertices, edges,
         diagonal = '·'
         #missing = '🕱'
         missing = '-'
+        right_arrow = '←'
     else:
         vertical, horizontal, corner, diagonal, missing = '|-,0-'
+        right_arrow = '<-'
 
     colours = COLOUR_SETS[colour]
 
@@ -607,6 +610,9 @@ def distance_matrix(vertices, edges,
                     link = '+'
                 row.append('%s%s%s' % (ct, link, c_reset))
 
+        if row_comments is not None and row_comments[i]:
+            row.append('%s %s %s' % (c_reset, right_arrow, row_comments[i]))
+
         write('%s%*s%s %s%s' % (c, vlen, v, c_reset,
                                 ''.join(row), c_reset))
 
diff --git a/python/samba/netcmd/visualize.py b/python/samba/netcmd/visualize.py
index e66030d3107..2b8c5fc57ff 100644
--- a/python/samba/netcmd/visualize.py
+++ b/python/samba/netcmd/visualize.py
@@ -450,7 +450,13 @@ class cmd_ntdsconn(GraphCommand):
                 ntds_dn = 'CN=NTDS Settings,' + dsa_dn
                 dn = dsa_dn
 
-            vertices.add(ntds_dn)
+            res = samdb.search(ntds_dn,
+                               scope=SCOPE_BASE,
+                               attrs=["msDS-isRODC"])
+
+            is_rodc = res[0]["msDS-isRODC"][0] == 'TRUE'
+
+            vertices.add((ntds_dn, 'RODC' if is_rodc else ''))
             # XXX we could also look at schedule
             res = samdb.search(dn,
                                scope=SCOPE_SUBTREE,
@@ -482,16 +488,25 @@ class cmd_ntdsconn(GraphCommand):
                 edges[k] = e
             e.attest(attester)
 
+        vertices, rodc_status = zip(*sorted(vertices))
+
         if self.calc_output_format(format, output) == 'distance':
             color_scheme = self.calc_distance_color_scheme(color,
                                                            color_scheme,
                                                            output)
+            colours = COLOUR_SETS[color_scheme]
+            c_header = colours.get('header', '')
+            c_reset = colours.get('reset', '')
+
+            epilog = []
+            if 'RODC' in rodc_status:
+                epilog.append('No outbound connections are expected from RODCs')
+
             if not talk_to_remote:
                 # If we are not talking to remote servers, we list all
                 # the connections.
                 graph_edges = edges.keys()
                 title = 'NTDS Connections known to %s' % local_dsa_dn
-                epilog = ''
 
             else:
                 # If we are talking to the remotes, there are
@@ -521,7 +536,7 @@ class cmd_ntdsconn(GraphCommand):
                         both_deny.append(e)
 
                 title = 'NTDS Connections known to each destination DC'
-                epilog = []
+
                 if both_deny:
                     epilog.append('The following connections are alleged by '
                                   'DCs other than the source and '
@@ -540,15 +555,25 @@ class cmd_ntdsconn(GraphCommand):
                                   'are not known to the source DC:\n')
                     for e in source_denies:
                         epilog.append('  %s -> %s\n' % e)
-                epilog = ''.join(epilog)
 
-            s = distance_matrix(sorted(vertices), graph_edges,
+
+            s = distance_matrix(vertices, graph_edges,
                                 utf8=utf8,
                                 colour=color_scheme,
                                 shorten_names=shorten_names,
                                 generate_key=key,
-                                grouping_function=get_dnstr_site)
-            self.write('\n%s\n%s\n%s' % (title, s, epilog), output)
+                                grouping_function=get_dnstr_site,
+                                row_comments=rodc_status)
+
+            epilog = ''.join(epilog)
+            if epilog:
+                epilog = '\n%sNOTES%s\n%s' % (c_header,
+                                              c_reset,
+                                              epilog)
+
+            self.write('\n%s\n\n%s\n%s' % (title,
+                                           s,
+                                           epilog), output)
             return
 
         dot_edges = []
diff --git a/python/samba/tests/samba_tool/visualize.py b/python/samba/tests/samba_tool/visualize.py
index 1afb210b7bd..43757e7b497 100644
--- a/python/samba/tests/samba_tool/visualize.py
+++ b/python/samba/tests/samba_tool/visualize.py
@@ -71,11 +71,12 @@ def samdb_from_ldif(ldif, tempdir, lp, dsa=None, tag=''):
     return (samdb, dburl)
 
 
-def collapse_space(s):
+def collapse_space(s, keep_empty_lines=False):
     lines = []
     for line in s.splitlines():
         line = ' '.join(line.strip().split())
-        lines.append(line)
+        if line or keep_empty_lines:
+            lines.append(line)
     return '\n'.join(lines)
 
 
@@ -453,6 +454,7 @@ key_0__label -> elision0 [style=invis; weight=9]
 
 EXPECTED_DISTANCE_GRAPH_WITH_KEY = """
 NTDS Connections known to CN=LOCALDC,CN=Servers,CN=Default-First-Site-Name,CN=Sites,CN=Configuration,DC=samba,DC=example,DC=com
+
                             destination
                   ,-------- *,CN=CLIENT+
                   |,------- *,CN=LOCALDC+
-- 
2.17.0


From 7117731987dd45e5775f40624d6840844a6278d9 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Thu, 8 Mar 2018 17:42:18 +1300
Subject: [PATCH 05/25] samba-tool visualise: --xdot option for instant
 graphviz visualisation

This is a convenience for people who have xdot (and X11).

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/netcmd/visualize.py           | 32 +++++++++++++--
 python/samba/tests/samba_tool/visualize.py | 45 ++++++++++++++++++++++
 2 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/python/samba/netcmd/visualize.py b/python/samba/netcmd/visualize.py
index 2b8c5fc57ff..c9bc8244df6 100644
--- a/python/samba/netcmd/visualize.py
+++ b/python/samba/netcmd/visualize.py
@@ -22,6 +22,7 @@ from __future__ import print_function
 import os
 import sys
 from collections import defaultdict
+import subprocess
 
 import tempfile
 import samba
@@ -44,6 +45,8 @@ COMMON_OPTIONS = [
            type=str, metavar="FILE", default=None),
     Option("--dot", help="Graphviz dot output", dest='format',
            const='dot', action='store_const'),
+    Option("--xdot", help="attempt to call Graphviz xdot", dest='format',
+           const='xdot', action='store_const'),
     Option("--distance", help="Distance matrix graph output (default)",
            dest='format', const='distance', action='store_const'),
     Option("--utf8", help="Use utf-8 Unicode characters",
@@ -133,8 +136,21 @@ class GraphCommand(Command):
                 return 'dot'
             else:
                 return 'distance'
+
+        if format == 'xdot':
+            return 'dot'
+
         return format
 
+    def call_xdot(self, s, output):
+        if output is None:
+            fn = self.write(s, TEMP_FILE)
+        else:
+            fn = self.write(s, output)
+        xdot = os.environ.get('SAMBA_TOOL_XDOT_PATH', '/usr/bin/xdot')
+        subprocess.call([xdot, fn])
+        os.remove(fn)
+
     def calc_distance_color_scheme(self, color, color_scheme, output):
         """Heuristics to work out the colour scheme for distance matrices.
         Returning None means no colour, otherwise it sould be a colour
@@ -211,7 +227,7 @@ class cmd_reps(GraphCommand):
             key=True, talk_to_remote=False,
             sambaopts=None, credopts=None, versionopts=None,
             mode='self', partition=None, color=None, color_scheme=None,
-            utf8=None, format=None):
+            utf8=None, format=None, xdot=False):
         # We use the KCC libraries in readonly mode to get the
         # replication graph.
         lp = sambaopts.get_loadparm()
@@ -377,7 +393,10 @@ class cmd_reps(GraphCommand):
                       shorten_names=shorten_names,
                       key_items=key_items)
 
-        self.write(s, output)
+        if format == 'xdot':
+            self.call_xdot(s, output)
+        else:
+            self.write(s, output)
 
 
 class NTDSConn(object):
@@ -416,7 +435,8 @@ class cmd_ntdsconn(GraphCommand):
             key=True, talk_to_remote=False,
             sambaopts=None, credopts=None, versionopts=None,
             color=None, color_scheme=None,
-            utf8=None, format=None, importldif=None):
+            utf8=None, format=None, importldif=None,
+            xdot=False):
 
         lp = sambaopts.get_loadparm()
         if importldif is None:
@@ -630,7 +650,11 @@ class cmd_ntdsconn(GraphCommand):
                       edge_styles=edge_styles,
                       shorten_names=shorten_names,
                       key_items=key_items)
-        self.write(s, output)
+
+        if format == 'xdot':
+            self.call_xdot(s, output)
+        else:
+            self.write(s, output)
 
 
 class cmd_visualize(SuperCommand):
diff --git a/python/samba/tests/samba_tool/visualize.py b/python/samba/tests/samba_tool/visualize.py
index 43757e7b497..abb6c49b3ad 100644
--- a/python/samba/tests/samba_tool/visualize.py
+++ b/python/samba/tests/samba_tool/visualize.py
@@ -27,6 +27,7 @@ query.
 from __future__ import print_function
 import samba
 import os
+import tempfile
 import re
 from samba.tests.samba_tool.base import SambaToolCmdTest
 from samba.kcc import ldif_import_export
@@ -123,6 +124,50 @@ class SambaToolVisualizeLdif(SambaToolCmdTest):
 
             self.assertStringsEqual(monochrome, uncoloured, strip=True)
 
+    def test_import_ldif_xdot(self):
+        """We can't test actual xdot, but using the environment we can
+        persuade samba-tool that a script we write is xdot and ensure
+        it gets the right text.
+        """
+        result, expected, err = self.runsubcmd("visualize", "ntdsconn",
+                                               '-H', self.dburl,
+                                               '--color=no', '-S',
+                                               '--dot')
+        self.assertCmdSuccess(result, expected, err)
+
+        # not that we're expecting anything here
+        old_xdot_path = os.environ.get('SAMBA_TOOL_XDOT_PATH')
+
+        tmpdir = tempfile.mkdtemp()
+        fake_xdot = os.path.join(tmpdir, 'fake_xdot')
+        content = os.path.join(tmpdir, 'content')
+        f = open(fake_xdot, 'w')
+        print('#!/bin/sh', file=f)
+        print('cp $1 %s' % content, file=f)
+        f.close()
+        os.chmod(fake_xdot, 0o700)
+
+        os.environ['SAMBA_TOOL_XDOT_PATH'] = fake_xdot
+        result, empty, err = self.runsubcmd("visualize", "ntdsconn",
+                                            '--importldif', MULTISITE_LDIF,
+                                            '--color=no', '-S',
+                                            '--xdot')
+
+        f = open(content)
+        xdot = f.read()
+        f.close()
+        os.remove(fake_xdot)
+        os.remove(content)
+        os.rmdir(tmpdir)
+
+        if old_xdot_path is not None:
+            os.environ['SAMBA_TOOL_XDOT_PATH'] = old_xdot_path
+        else:
+            del os.environ['SAMBA_TOOL_XDOT_PATH']
+
+        self.assertCmdSuccess(result, xdot, err)
+        self.assertStringsEqual(expected, xdot, strip=True)
+
     def test_import_ldif(self):
         """Make sure the samba-tool visualize --importldif option gives the
         same output as using the externally generated db from the same
-- 
2.17.0


From 7fd2167ed39bdf6e8077cc40db9d982c51796d5a Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Mon, 12 Mar 2018 12:29:28 +1300
Subject: [PATCH 06/25] samba-tool drs replicate: make pseudo-method a real
 method

This function can't function without a cmd_drs_replicate class, so it might as well be inside

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/netcmd/drs.py | 120 ++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 61 deletions(-)

diff --git a/python/samba/netcmd/drs.py b/python/samba/netcmd/drs.py
index ebfabe186eb..7dabdcc24ab 100644
--- a/python/samba/netcmd/drs.py
+++ b/python/samba/netcmd/drs.py
@@ -306,64 +306,6 @@ class cmd_drs_kcc(Command):
         self.message("Consistency check on %s successful." % DC)
 
 
-
-def drs_local_replicate(self, SOURCE_DC, NC, full_sync=False, single_object=False,
-                        sync_forced=False):
-    '''replicate from a source DC to the local SAM'''
-
-    self.server = SOURCE_DC
-    drsuapi_connect(self)
-
-    self.local_samdb = SamDB(session_info=system_session(), url=None,
-                             credentials=self.creds, lp=self.lp)
-
-    self.samdb = SamDB(url="ldap://%s" % self.server,
-                       session_info=system_session(),
-                       credentials=self.creds, lp=self.lp)
-
-    # work out the source and destination GUIDs
-    res = self.local_samdb.search(base="", scope=ldb.SCOPE_BASE,
-                                  attrs=["dsServiceName"])
-    self.ntds_dn = res[0]["dsServiceName"][0]
-
-    res = self.local_samdb.search(base=self.ntds_dn, scope=ldb.SCOPE_BASE,
-                                  attrs=["objectGUID"])
-    self.ntds_guid = misc.GUID(self.samdb.schema_format_value("objectGUID", res[0]["objectGUID"][0]))
-
-    source_dsa_invocation_id = misc.GUID(self.samdb.get_invocation_id())
-    dest_dsa_invocation_id = misc.GUID(self.local_samdb.get_invocation_id())
-    destination_dsa_guid = self.ntds_guid
-
-    exop = drsuapi.DRSUAPI_EXOP_NONE
-
-    if single_object:
-        exop = drsuapi.DRSUAPI_EXOP_REPL_OBJ
-        full_sync = True
-
-    self.samdb.transaction_start()
-    repl = drs_utils.drs_Replicate("ncacn_ip_tcp:%s[seal]" % self.server, self.lp,
-                                   self.creds, self.local_samdb, dest_dsa_invocation_id)
-
-    # Work out if we are an RODC, so that a forced local replicate
-    # with the admin pw does not sync passwords
-    rodc = self.local_samdb.am_rodc()
-    try:
-        (num_objects, num_links) = repl.replicate(NC,
-                                                  source_dsa_invocation_id, destination_dsa_guid,
-                                                  rodc=rodc, full_sync=full_sync,
-                                                  exop=exop, sync_forced=sync_forced)
-    except Exception as e:
-        raise CommandError("Error replicating DN %s" % NC, e)
-    self.samdb.transaction_commit()
-
-    if full_sync:
-        self.message("Full Replication of all %d objects and %d links from %s to %s was successful."
-                     % (num_objects, num_links, SOURCE_DC, self.local_samdb.url))
-    else:
-        self.message("Incremental replication of %d objects and %d links from %s to %s was successful."
-                     % (num_objects, num_links, SOURCE_DC, self.local_samdb.url))
-
-
 class cmd_drs_replicate(Command):
     """Replicate a naming context between two DCs."""
 
@@ -388,6 +330,62 @@ class cmd_drs_replicate(Command):
         Option("--single-object", help="Replicate only the object specified, instead of the whole Naming Context (only with --local)", action="store_true"),
         ]
 
+    def drs_local_replicate(self, SOURCE_DC, NC, full_sync=False, single_object=False,
+                            sync_forced=False):
+        '''replicate from a source DC to the local SAM'''
+
+        self.server = SOURCE_DC
+        drsuapi_connect(self)
+
+        self.local_samdb = SamDB(session_info=system_session(), url=None,
+                                 credentials=self.creds, lp=self.lp)
+
+        self.samdb = SamDB(url="ldap://%s" % self.server,
+                           session_info=system_session(),
+                           credentials=self.creds, lp=self.lp)
+
+        # work out the source and destination GUIDs
+        res = self.local_samdb.search(base="", scope=ldb.SCOPE_BASE,
+                                      attrs=["dsServiceName"])
+        self.ntds_dn = res[0]["dsServiceName"][0]
+
+        res = self.local_samdb.search(base=self.ntds_dn, scope=ldb.SCOPE_BASE,
+                                      attrs=["objectGUID"])
+        self.ntds_guid = misc.GUID(self.samdb.schema_format_value("objectGUID", res[0]["objectGUID"][0]))
+
+        source_dsa_invocation_id = misc.GUID(self.samdb.get_invocation_id())
+        dest_dsa_invocation_id = misc.GUID(self.local_samdb.get_invocation_id())
+        destination_dsa_guid = self.ntds_guid
+
+        exop = drsuapi.DRSUAPI_EXOP_NONE
+
+        if single_object:
+            exop = drsuapi.DRSUAPI_EXOP_REPL_OBJ
+            full_sync = True
+
+        self.samdb.transaction_start()
+        repl = drs_utils.drs_Replicate("ncacn_ip_tcp:%s[seal]" % self.server, self.lp,
+                                       self.creds, self.local_samdb, dest_dsa_invocation_id)
+
+        # Work out if we are an RODC, so that a forced local replicate
+        # with the admin pw does not sync passwords
+        rodc = self.local_samdb.am_rodc()
+        try:
+            (num_objects, num_links) = repl.replicate(NC,
+                                                      source_dsa_invocation_id, destination_dsa_guid,
+                                                      rodc=rodc, full_sync=full_sync,
+                                                      exop=exop, sync_forced=sync_forced)
+        except Exception as e:
+            raise CommandError("Error replicating DN %s" % NC, e)
+        self.samdb.transaction_commit()
+
+        if full_sync:
+            self.message("Full Replication of all %d objects and %d links from %s to %s was successful."
+                         % (num_objects, num_links, SOURCE_DC, self.local_samdb.url))
+        else:
+            self.message("Incremental replication of %d objects and %d links from %s to %s was successful."
+                         % (num_objects, num_links, SOURCE_DC, self.local_samdb.url))
+
     def run(self, DEST_DC, SOURCE_DC, NC,
             add_ref=False, sync_forced=False, sync_all=False, full_sync=False,
             local=False, local_online=False, async_op=False, single_object=False,
@@ -399,9 +397,9 @@ class cmd_drs_replicate(Command):
         self.creds = credopts.get_credentials(self.lp, fallback_machine=True)
 
         if local:
-            drs_local_replicate(self, SOURCE_DC, NC, full_sync=full_sync,
-                                single_object=single_object,
-                                sync_forced=sync_forced)
+            self.drs_local_replicate(SOURCE_DC, NC, full_sync=full_sync,
+                                     single_object=single_object,
+                                     sync_forced=sync_forced)
             return
 
         if local_online:
-- 
2.17.0


From fa25e420d61ffa8a34e5af860b49ef66f622811c Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Mon, 12 Mar 2018 12:33:01 +1300
Subject: [PATCH 07/25] samba-tool drs replicate: reformat drs_local_replicate
 method

line length.

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/netcmd/drs.py | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/python/samba/netcmd/drs.py b/python/samba/netcmd/drs.py
index 7dabdcc24ab..a8c096b24bd 100644
--- a/python/samba/netcmd/drs.py
+++ b/python/samba/netcmd/drs.py
@@ -330,7 +330,8 @@ class cmd_drs_replicate(Command):
         Option("--single-object", help="Replicate only the object specified, instead of the whole Naming Context (only with --local)", action="store_true"),
         ]
 
-    def drs_local_replicate(self, SOURCE_DC, NC, full_sync=False, single_object=False,
+    def drs_local_replicate(self, SOURCE_DC, NC, full_sync=False,
+                            single_object=False,
                             sync_forced=False):
         '''replicate from a source DC to the local SAM'''
 
@@ -351,7 +352,9 @@ class cmd_drs_replicate(Command):
 
         res = self.local_samdb.search(base=self.ntds_dn, scope=ldb.SCOPE_BASE,
                                       attrs=["objectGUID"])
-        self.ntds_guid = misc.GUID(self.samdb.schema_format_value("objectGUID", res[0]["objectGUID"][0]))
+        self.ntds_guid = misc.GUID(
+            self.samdb.schema_format_value("objectGUID",
+                                           res[0]["objectGUID"][0]))
 
         source_dsa_invocation_id = misc.GUID(self.samdb.get_invocation_id())
         dest_dsa_invocation_id = misc.GUID(self.local_samdb.get_invocation_id())
@@ -364,27 +367,36 @@ class cmd_drs_replicate(Command):
             full_sync = True
 
         self.samdb.transaction_start()
-        repl = drs_utils.drs_Replicate("ncacn_ip_tcp:%s[seal]" % self.server, self.lp,
-                                       self.creds, self.local_samdb, dest_dsa_invocation_id)
+        repl = drs_utils.drs_Replicate("ncacn_ip_tcp:%s[seal]" % self.server,
+                                       self.lp,
+                                       self.creds, self.local_samdb,
+                                       dest_dsa_invocation_id)
 
         # Work out if we are an RODC, so that a forced local replicate
         # with the admin pw does not sync passwords
         rodc = self.local_samdb.am_rodc()
         try:
             (num_objects, num_links) = repl.replicate(NC,
-                                                      source_dsa_invocation_id, destination_dsa_guid,
-                                                      rodc=rodc, full_sync=full_sync,
-                                                      exop=exop, sync_forced=sync_forced)
+                                                      source_dsa_invocation_id,
+                                                      destination_dsa_guid,
+                                                      rodc=rodc,
+                                                      full_sync=full_sync,
+                                                      exop=exop,
+                                                      sync_forced=sync_forced)
         except Exception as e:
             raise CommandError("Error replicating DN %s" % NC, e)
         self.samdb.transaction_commit()
 
         if full_sync:
-            self.message("Full Replication of all %d objects and %d links from %s to %s was successful."
-                         % (num_objects, num_links, SOURCE_DC, self.local_samdb.url))
+            self.message("Full Replication of all %d objects and %d links "
+                         "from %s to %s was successful." %
+                         (num_objects, num_links, SOURCE_DC,
+                          self.local_samdb.url))
         else:
-            self.message("Incremental replication of %d objects and %d links from %s to %s was successful."
-                         % (num_objects, num_links, SOURCE_DC, self.local_samdb.url))
+            self.message("Incremental replication of %d objects and %d links "
+                         "from %s to %s was successful." %
+                         (num_objects, num_links, SOURCE_DC,
+                          self.local_samdb.url))
 
     def run(self, DEST_DC, SOURCE_DC, NC,
             add_ref=False, sync_forced=False, sync_all=False, full_sync=False,
-- 
2.17.0


From 809badd5edda592f3f13bb45cdcebff9f429a29d Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Thu, 15 Mar 2018 12:01:10 +1300
Subject: [PATCH 08/25] kcc.graph_utils: shift debug noise out of verify()

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/kcc/graph.py       | 11 ++++++---
 python/samba/kcc/graph_utils.py | 43 +++++++++++++--------------------
 2 files changed, 24 insertions(+), 30 deletions(-)

diff --git a/python/samba/kcc/graph.py b/python/samba/kcc/graph.py
index fb3ca0c81c2..9dfc541548f 100644
--- a/python/samba/kcc/graph.py
+++ b/python/samba/kcc/graph.py
@@ -184,10 +184,13 @@ def get_spanning_tree_edges(graph, my_site, label=None, verify=False,
                                vertices=graph_nodes, label=label)
 
             if verify:
-                verify_graph('spanning tree edge set %s' % edgeType,
-                             graph_edges, vertices=graph_nodes,
-                             properties=('complete', 'connected'),
-                             debug=DEBUG)
+                errors = verify_graph(graph_edges, vertices=graph_nodes,
+                                      properties=('complete', 'connected'))
+                if errors:
+                    DEBUG('spanning tree edge set %s FAILED' % edgeType)
+                    for p, e in errors:
+                        DEBUG("%18s: %s" % (p, e))
+                    raise KCCError("spanning tree failed")
 
         # Run dijkstra's algorithm with just the red vertices as seeds
         # Seed from the full replicas
diff --git a/python/samba/kcc/graph_utils.py b/python/samba/kcc/graph_utils.py
index 086b6512201..e4fa821c798 100644
--- a/python/samba/kcc/graph_utils.py
+++ b/python/samba/kcc/graph_utils.py
@@ -274,12 +274,8 @@ def verify_graph_directed_double_ring_or_small(edges, vertices, edge_vertices):
     return verify_graph_directed_double_ring(edges, vertices, edge_vertices)
 
 
-def verify_graph(title, edges, vertices=None, directed=False, properties=(),
-                 fatal=True, debug=null_debug):
+def verify_graph(edges, vertices=None, directed=False, properties=()):
     errors = []
-    debug("%sStarting verify_graph for %s%s%s" % (PURPLE, MAGENTA, title,
-                                                  C_NORMAL))
-
     properties = [x.replace(' ', '_') for x in properties]
 
     edge_vertices = set()
@@ -291,31 +287,16 @@ def verify_graph(title, edges, vertices=None, directed=False, properties=(),
         vertices = edge_vertices
     else:
         vertices = set(vertices)
-        if vertices != edge_vertices:
-            debug("vertices in edges don't match given vertices:\n %s != %s" %
-                  (sorted(edge_vertices), sorted(vertices)))
 
     for p in properties:
         fn = 'verify_graph_%s' % p
-        try:
-            f = globals()[fn]
-        except KeyError:
-            errors.append((p, "There is no verification check for '%s'" % p))
+        f = globals()[fn]
         try:
             f(edges, vertices, edge_vertices)
-            debug(" %s%18s:%s verified!" % (DARK_GREEN, p, C_NORMAL))
         except GraphError as e:
             errors.append((p, e))
 
-    if errors:
-        if fatal:
-            raise GraphError("The '%s' graph lacks the following properties:"
-                             "\n%s" %
-                             (title, '\n'.join('%s: %s' % x for x in errors)))
-        debug(("%s%s%s FAILED:" % (MAGENTA, title, RED)))
-        for p, e in errors:
-            debug(" %18s: %s%s%s" % (p, DARK_YELLOW, e, RED))
-        debug(C_NORMAL)
+    return errors
 
 
 def verify_and_dot(basename, edges, vertices=None, label=None,
@@ -325,10 +306,6 @@ def verify_and_dot(basename, edges, vertices=None, label=None,
                    edge_colors=None, edge_labels=None,
                    vertex_colors=None):
 
-    title = '%s %s' % (basename, label or '')
-    if verify:
-        verify_graph(title, edges, vertices, properties=properties,
-                     fatal=fatal, debug=debug)
     if dot_file_dir is not None:
         write_dot_file(basename, edges, vertices=vertices, label=label,
                        dot_file_dir=dot_file_dir,
@@ -336,6 +313,20 @@ def verify_and_dot(basename, edges, vertices=None, label=None,
                        debug=debug, edge_colors=edge_colors,
                        edge_labels=edge_labels, vertex_colors=vertex_colors)
 
+    if verify:
+        errors = verify_graph(edges, vertices,
+                              properties=properties)
+        if errors:
+            title = '%s %s' % (basename, label or '')
+            debug(("%s%s%s FAILED:" % (MAGENTA, title, RED)))
+            for p, e in errors:
+                debug(" %18s: %s%s%s" % (p, DARK_YELLOW, e, RED))
+            if fatal:
+                raise GraphError("The '%s' graph lacks the following "
+                                 "properties:\n%s" %
+                                 (title, '\n'.join('%s: %s' % x
+                                                   for x in errors)))
+
 
 def list_verify_tests():
     for k, v in sorted(globals().items()):
-- 
2.17.0


From 73faa4a76824a950f1d9149b458464c771125382 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Thu, 22 Mar 2018 16:49:29 +1300
Subject: [PATCH 09/25] dsdb/util: use parse_guid_string, not sscanf()

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 source4/dsdb/common/util.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c
index c227c836d04..36c98df24b3 100644
--- a/source4/dsdb/common/util.c
+++ b/source4/dsdb/common/util.c
@@ -47,6 +47,8 @@
 #include "libds/common/flag_mapping.h"
 #include "../lib/util/util_runcmd.h"
 #include "lib/util/access.h"
+#include "lib/util/util_str_hex.h"
+#include "libcli/util/ntstatus.h"
 
 /*
  * This included to allow us to handle DSDB_FLAG_REPLICATED_UPDATE in
@@ -5186,12 +5188,12 @@ _PUBLIC_ NTSTATUS NS_GUID_from_string(const char *s, struct GUID *guid)
 		return NT_STATUS_INVALID_PARAMETER;
 	}
 
-	if (11 == sscanf(s, "%08x-%04x%04x-%02x%02x%02x%02x-%02x%02x%02x%02x",
-			 &time_low, &time_mid, &time_hi_and_version, 
-			 &clock_seq[0], &clock_seq[1],
-			 &node[0], &node[1], &node[2], &node[3], &node[4], &node[5])) {
-	        status = NT_STATUS_OK;
-	}
+	status =  parse_guid_string(s,
+				    &time_low,
+				    &time_mid,
+				    &time_hi_and_version,
+				    clock_seq,
+				    node);
 
 	if (!NT_STATUS_IS_OK(status)) {
 		return status;
-- 
2.17.0


From c6dd8a26c02642153121df6e01ea9115bd283932 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Thu, 22 Mar 2018 17:54:55 +1300
Subject: [PATCH 10/25] util/charset/iconv: use read_hex_bytes rather than
 sscanf

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 lib/util/charset/iconv.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/util/charset/iconv.c b/lib/util/charset/iconv.c
index 02c061ccd09..a03ff20b8cc 100644
--- a/lib/util/charset/iconv.c
+++ b/lib/util/charset/iconv.c
@@ -25,6 +25,8 @@
 #include "lib/util/dlinklist.h"
 #include "lib/util/charset/charset.h"
 #include "lib/util/charset/charset_proto.h"
+#include "libcli/util/ntstatus.h"
+#include "lib/util/util_str_hex.h"
 
 #ifdef strcasecmp
 #undef strcasecmp
@@ -454,8 +456,8 @@ static size_t ucs2hex_pull(void *cd, const char **inbuf, size_t *inbytesleft,
 			 char **outbuf, size_t *outbytesleft)
 {
 	while (*inbytesleft >= 1 && *outbytesleft >= 2) {
-		unsigned int v;
-
+		uint64_t v;
+		NTSTATUS status;
 		if ((*inbuf)[0] != '@') {
 			/* seven bit ascii case */
 			(*outbuf)[0] = (*inbuf)[0];
@@ -471,8 +473,9 @@ static size_t ucs2hex_pull(void *cd, const char **inbuf, size_t *inbytesleft,
 			errno = EINVAL;
 			return -1;
 		}
+		status = read_hex_bytes(&(*inbuf)[1], 4, &v);
 
-		if (sscanf(&(*inbuf)[1], "%04x", &v) != 1) {
+		if (!NT_STATUS_IS_OK(status)) {
 			errno = EILSEQ;
 			return -1;
 		}
-- 
2.17.0


From 5e5509ae6f21115f5c55bccd7633e961821183d9 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Thu, 22 Mar 2018 17:57:05 +1300
Subject: [PATCH 11/25] util_str_hex: use array syntax in guid functions to
 document usage

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 lib/util/util_str_hex.c | 4 ++--
 lib/util/util_str_hex.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/util/util_str_hex.c b/lib/util/util_str_hex.c
index 148735bd5f3..792b4e8420b 100644
--- a/lib/util/util_str_hex.c
+++ b/lib/util/util_str_hex.c
@@ -38,8 +38,8 @@ NTSTATUS parse_guid_string(const char *s,
 			   uint32_t *time_low,
 			   uint32_t *time_mid,
 			   uint32_t *time_hi_and_version,
-			   uint32_t *clock_seq,
-			   uint32_t *node)
+			   uint32_t clock_seq[2],
+			   uint32_t node[6])
 {
 	uint64_t tmp;
 	NTSTATUS status;
diff --git a/lib/util/util_str_hex.h b/lib/util/util_str_hex.h
index 2d0ba65636b..d0d53e3ed05 100644
--- a/lib/util/util_str_hex.h
+++ b/lib/util/util_str_hex.h
@@ -6,5 +6,5 @@ NTSTATUS parse_guid_string(const char *s,
 			   uint32_t *time_low,
 			   uint32_t *time_mid,
 			   uint32_t *time_hi_and_version,
-			   uint32_t *clock_seq,
-			   uint32_t *node);
+			   uint32_t clock_seq[2],
+			   uint32_t node[6]);
-- 
2.17.0


From 4f0839c9fafb209674098bad87fb957015be4798 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Thu, 22 Mar 2018 17:12:49 +1300
Subject: [PATCH 12/25] ndr_misc: read syntax_id using strict util_str_hex
 functions

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 librpc/ndr/ndr_misc.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/librpc/ndr/ndr_misc.c b/librpc/ndr/ndr_misc.c
index fa643c84949..155ab8f21b8 100644
--- a/librpc/ndr/ndr_misc.c
+++ b/librpc/ndr/ndr_misc.c
@@ -23,6 +23,8 @@
 #include "includes.h"
 #include "system/network.h"
 #include "librpc/ndr/libndr.h"
+#include "libcli/util/ntstatus.h"
+#include "lib/util/util_str_hex.h"
 
 _PUBLIC_ void ndr_print_GUID(struct ndr_print *ndr, const char *name, const struct GUID *guid)
 {
@@ -52,21 +54,32 @@ _PUBLIC_ char *ndr_syntax_id_to_string(TALLOC_CTX *mem_ctx, const struct ndr_syn
 
 _PUBLIC_ bool ndr_syntax_id_from_string(const char *s, struct ndr_syntax_id *id)
 {
-	int ret;
 	size_t i;
 	uint32_t time_low;
 	uint32_t time_mid, time_hi_and_version;
 	uint32_t clock_seq[2];
 	uint32_t node[6];
-	uint32_t if_version;
+	uint64_t if_version;
+	NTSTATUS status;
 
-	ret = sscanf(s,
-		     "%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x/0x%08x",
-		     &time_low, &time_mid, &time_hi_and_version,
-		     &clock_seq[0], &clock_seq[1],
-		     &node[0], &node[1], &node[2], &node[3], &node[4], &node[5],
-		     &if_version);
-	if (ret != 12) {
+	status =  parse_guid_string(s,
+				    &time_low,
+				    &time_mid,
+				    &time_hi_and_version,
+				    clock_seq,
+				    node);
+
+	if (!NT_STATUS_IS_OK(status)) {
+		return false;
+	}
+
+	if (strncmp(s + 36, "/0x", 3) != 0) {
+		return false;
+	}
+
+	status = read_hex_bytes(s + 39, 8, &if_version);
+
+	if (!NT_STATUS_IS_OK(status)) {
 		return false;
 	}
 
@@ -78,7 +91,7 @@ _PUBLIC_ bool ndr_syntax_id_from_string(const char *s, struct ndr_syntax_id *id)
 	for (i=0; i<6; i++) {
 		id->uuid.node[i] = node[i];
 	}
-	id->if_version = if_version;
+	id->if_version = (uint32_t)if_version;
 
 	return true;
 }
-- 
2.17.0


From 7d7ec2c2a28c5094a36371d841ecbe489215113d Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Sun, 7 Jan 2018 22:17:43 +1300
Subject: [PATCH 13/25] kcc graphs: site edges in colour, labeled with DNs

This makes it easy to see where the site edges objects are, and
what sites they refer too.

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/kcc/__init__.py  | 9 +++++++--
 python/samba/kcc/graph.py     | 2 +-
 python/samba/kcc/kcc_utils.py | 9 +++++----
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/python/samba/kcc/__init__.py b/python/samba/kcc/__init__.py
index fae9dbd13d3..1b22bf73a9f 100644
--- a/python/samba/kcc/__init__.py
+++ b/python/samba/kcc/__init__.py
@@ -2583,15 +2583,20 @@ class KCC(object):
                                dot_file_dir=self.dot_file_dir)
 
                 dot_edges = []
+                dot_colours = []
                 for link in self.sitelink_table.values():
+                    from hashlib import md5
+                    colour = '#' + md5(link.dnstr).hexdigest()[:6]
                     for a, b in itertools.combinations(link.site_list, 2):
-                        dot_edges.append((str(a), str(b)))
+                        dot_edges.append((a[1], b[1]))
+                        dot_colours.append(colour)
                 properties = ('connected',)
                 verify_and_dot('dsa_sitelink_initial', dot_edges,
                                directed=False,
                                label=self.my_dsa_dnstr, properties=properties,
                                debug=DEBUG, verify=self.verify,
-                               dot_file_dir=self.dot_file_dir)
+                               dot_file_dir=self.dot_file_dir,
+                               edge_colors=dot_colours)
 
             if forget_local_links:
                 for dsa in self.my_site.dsa_table.values():
diff --git a/python/samba/kcc/graph.py b/python/samba/kcc/graph.py
index 9dfc541548f..1e63f7c7629 100644
--- a/python/samba/kcc/graph.py
+++ b/python/samba/kcc/graph.py
@@ -293,7 +293,7 @@ def create_edge(con_type, site_link, guid_to_vertex):
     e = MultiEdge()
     e.site_link = site_link
     e.vertices = []
-    for site_guid in site_link.site_list:
+    for site_guid, site_dn in site_link.site_list:
         if str(site_guid) in guid_to_vertex:
             e.vertices.extend(guid_to_vertex.get(str(site_guid)))
     e.repl_info.cost = site_link.cost
diff --git a/python/samba/kcc/kcc_utils.py b/python/samba/kcc/kcc_utils.py
index 23fbd40e904..1457ea355e0 100644
--- a/python/samba/kcc/kcc_utils.py
+++ b/python/samba/kcc/kcc_utils.py
@@ -2148,8 +2148,8 @@ class SiteLink(object):
                     text = text + "0x%X " % slot
                 text = text + "]"
 
-        for dnstr in self.site_list:
-            text = text + "\n\tsite_list=%s" % dnstr
+        for guid, dn in self.site_list:
+            text = text + "\n\tsite_list=%s (%s)" % (guid, dn)
         return text
 
     def load_sitelink(self, samdb):
@@ -2190,8 +2190,9 @@ class SiteLink(object):
             for value in msg["siteList"]:
                 dsdn = dsdb_Dn(samdb, value.decode('utf8'))
                 guid = misc.GUID(dsdn.dn.get_extended_component('GUID'))
-                if guid not in self.site_list:
-                    self.site_list.append(guid)
+                dnstr = str(dsdn.dn)
+                if (guid, dnstr) not in self.site_list:
+                    self.site_list.append((guid, dnstr))
 
         if "schedule" in msg:
             self.schedule = ndr_unpack(drsblobs.schedule, value)
-- 
2.17.0


From f2cd803bb62e0912e099bf261f17ae16f537a2ae Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Tue, 15 May 2018 14:40:36 +1200
Subject: [PATCH 14/25] kcc graph verifier: use __doc__ description for error
 explanation

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/kcc/graph.py       | 2 +-
 python/samba/kcc/graph_utils.py | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/python/samba/kcc/graph.py b/python/samba/kcc/graph.py
index 1e63f7c7629..d87530d40c3 100644
--- a/python/samba/kcc/graph.py
+++ b/python/samba/kcc/graph.py
@@ -188,7 +188,7 @@ def get_spanning_tree_edges(graph, my_site, label=None, verify=False,
                                       properties=('complete', 'connected'))
                 if errors:
                     DEBUG('spanning tree edge set %s FAILED' % edgeType)
-                    for p, e in errors:
+                    for p, e, doc in errors:
                         DEBUG("%18s: %s" % (p, e))
                     raise KCCError("spanning tree failed")
 
diff --git a/python/samba/kcc/graph_utils.py b/python/samba/kcc/graph_utils.py
index e4fa821c798..291fd486424 100644
--- a/python/samba/kcc/graph_utils.py
+++ b/python/samba/kcc/graph_utils.py
@@ -294,7 +294,7 @@ def verify_graph(edges, vertices=None, directed=False, properties=()):
         try:
             f(edges, vertices, edge_vertices)
         except GraphError as e:
-            errors.append((p, e))
+            errors.append((p, e, f.__doc__))
 
     return errors
 
@@ -319,13 +319,13 @@ def verify_and_dot(basename, edges, vertices=None, label=None,
         if errors:
             title = '%s %s' % (basename, label or '')
             debug(("%s%s%s FAILED:" % (MAGENTA, title, RED)))
-            for p, e in errors:
+            for p, e, doc in errors:
                 debug(" %18s: %s%s%s" % (p, DARK_YELLOW, e, RED))
             if fatal:
                 raise GraphError("The '%s' graph lacks the following "
                                  "properties:\n%s" %
-                                 (title, '\n'.join('%s: %s' % x
-                                                   for x in errors)))
+                                 (title, '\n'.join('%s: %s' % (p, e)
+                                                   for p, e, doc in errors)))
 
 
 def list_verify_tests():
-- 
2.17.0


From 9c40557193e6bcfff0fce1cad8361726e50d1a1c Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed, 16 May 2018 15:53:35 +1200
Subject: [PATCH 15/25] kcc graph verifiers: improve messages

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/kcc/graph_utils.py | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/python/samba/kcc/graph_utils.py b/python/samba/kcc/graph_utils.py
index 291fd486424..3d362f62543 100644
--- a/python/samba/kcc/graph_utils.py
+++ b/python/samba/kcc/graph_utils.py
@@ -65,9 +65,8 @@ def verify_graph_connected(edges, vertices, edge_vertices):
     if not edges:
         if len(vertices) <= 1:
             return
-        raise GraphError("disconnected vertices were found:\n"
-                         "vertices: %s\n edges: %s" %
-                         (sorted(vertices), sorted(edges)))
+        raise GraphError("all vertices are disconnected because "
+                         "there are no edges:")
 
     remaining_edges = list(edges)
     reached = set(remaining_edges.pop())
@@ -87,16 +86,23 @@ def verify_graph_connected(edges, vertices, edge_vertices):
             del remaining_edges[i]
 
     if remaining_edges or reached != set(vertices):
-        raise GraphError("graph is not connected:\n vertices: %s\n edges: %s\n"
-                         " reached: %s\n remaining edges: %s" %
-                         (sorted(vertices), sorted(edges),
-                          sorted(reached), sorted(remaining_edges)))
+        s = ("the graph is not connected, "
+             "as the following vertices are unreachable:\n ")
+        s += '\n '.join(v for v in sorted(vertices)
+                        if v not in reached)
+        raise GraphError(s)
 
 
 def verify_graph_connected_under_edge_failures(edges, vertices, edge_vertices):
     """The graph stays connected when any single edge is removed."""
     for subset in itertools.combinations(edges, len(edges) - 1):
-        verify_graph_connected(subset, vertices, edge_vertices)
+        try:
+            verify_graph_connected(subset, vertices, edge_vertices)
+        except GraphError as e:
+            for edge in edges:
+                if edge not in subset:
+                    raise GraphError("The graph will be disconnected when the "
+                                     "connection from %s to %s fails" % edge)
 
 
 def verify_graph_connected_under_vertex_failures(edges, vertices,
@@ -109,8 +115,7 @@ def verify_graph_connected_under_vertex_failures(edges, vertices,
 
 
 def verify_graph_forest(edges, vertices, edge_vertices):
-    """The graph contains no loops. A forest that is also connected is a
-    tree."""
+    """The graph contains no loops."""
     trees = [set(e) for e in edges]
     while True:
         for a, b in itertools.combinations(trees, 2):
-- 
2.17.0


From a9e96684e38fd1836b054d503be6d33df97acdbf Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Thu, 19 Apr 2018 16:39:06 +1200
Subject: [PATCH 16/25] python kcc/graph_utils: don't debug in colour

this was somewhat useful during the initial development, but is wrong for a library

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/kcc/graph_utils.py | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/python/samba/kcc/graph_utils.py b/python/samba/kcc/graph_utils.py
index 3d362f62543..727b342d4bb 100644
--- a/python/samba/kcc/graph_utils.py
+++ b/python/samba/kcc/graph_utils.py
@@ -22,8 +22,6 @@ from __future__ import print_function
 import os
 import itertools
 
-from samba.kcc.debug import null_debug, PURPLE, MAGENTA, DARK_YELLOW, RED
-from samba.kcc.debug import DARK_GREEN, C_NORMAL, GREY
 from samba.graph import dot_graph
 
 
@@ -323,9 +321,9 @@ def verify_and_dot(basename, edges, vertices=None, label=None,
                               properties=properties)
         if errors:
             title = '%s %s' % (basename, label or '')
-            debug(("%s%s%s FAILED:" % (MAGENTA, title, RED)))
+            debug("%s FAILED:" % title)
             for p, e, doc in errors:
-                debug(" %18s: %s%s%s" % (p, DARK_YELLOW, e, RED))
+                debug(" %18s: %s" % (p, e))
             if fatal:
                 raise GraphError("The '%s' graph lacks the following "
                                  "properties:\n%s" %
@@ -338,6 +336,6 @@ def list_verify_tests():
         if k.startswith('verify_graph_'):
             print(k.replace('verify_graph_', ''))
             if v.__doc__:
-                print('    %s%s%s' % (GREY, v.__doc__.rstrip(), C_NORMAL))
+                print('    %s' % (v.__doc__.rstrip()))
             else:
                 print()
-- 
2.17.0


From 759de619210027f5a9d47d4456e9b8557c63de1d Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Wed, 15 Nov 2017 09:57:52 +1300
Subject: [PATCH 17/25] tests/rodc:kcc: minimal test of KCC

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/tests/samba_tool/rodc.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/python/samba/tests/samba_tool/rodc.py b/python/samba/tests/samba_tool/rodc.py
index 870b5d4339e..298e4ccfea3 100644
--- a/python/samba/tests/samba_tool/rodc.py
+++ b/python/samba/tests/samba_tool/rodc.py
@@ -129,3 +129,7 @@ class RodcCmdTestCase(SambaToolCmdTest):
                                             "sambatool6", "sambatool5",
                                             "--server", os.environ["DC_SERVER"])
         self.assertCmdFail(result, "ensuring rodc prefetch quit on non-replicated user")
+
+    def test_kcc_does_not_crash(self):
+        (result, out, err) = self.runsubcmd("drs", "kcc", os.environ["DC_SERVER"])
+        self.assertCmdSuccess(result, out, err, "ensuring kcc runs on the rodc")
-- 
2.17.0


From 50c2184f865fc1ba97cb3121b8a4aa4a5ed88008 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Thu, 19 Apr 2018 16:43:50 +1200
Subject: [PATCH 18/25] samba-tool dns cleanup_record: add missing
 verbose/quiet options

The code for using them is already there

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/netcmd/dns.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/python/samba/netcmd/dns.py b/python/samba/netcmd/dns.py
index a0e92cf8dd9..ab6bacc6050 100644
--- a/python/samba/netcmd/dns.py
+++ b/python/samba/netcmd/dns.py
@@ -1095,6 +1095,11 @@ class cmd_cleanup_record(Command):
         "credopts": options.CredentialsOptions,
     }
 
+    takes_options = [
+        Option("-v", "--verbose", help="Be verbose", action="store_true"),
+        Option("-q", "--quiet", help="Be quiet", action="store_true"),
+    ]
+
     def run(self, server, dnshostname, sambaopts=None, credopts=None,
             versionopts=None, verbose=False, quiet=False):
         lp = sambaopts.get_loadparm()
-- 
2.17.0


From c3ff6f7552cfd18b758038ae8a66ba8ae2c1c22e Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Thu, 19 Apr 2018 16:56:28 +1200
Subject: [PATCH 19/25] samba-tool: add -v to domain --verbose

Sometimes we accept -v for --verbose, sometimes we don't. Let's be a
bit more consistent.

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/netcmd/domain.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/python/samba/netcmd/domain.py b/python/samba/netcmd/domain.py
index 819ddd680be..9815a86b4fc 100644
--- a/python/samba/netcmd/domain.py
+++ b/python/samba/netcmd/domain.py
@@ -134,7 +134,7 @@ common_join_options = [
            "BIND9_DLZ uses samba4 AD to store zone information, "
            "NONE skips the DNS setup entirely (this DC will not be a DNS server)",
            default="SAMBA_INTERNAL"),
-    Option("--verbose", help="Be verbose", action="store_true")
+    Option("-v", "--verbose", help="Be verbose", action="store_true")
 ]
 
 common_ntvfs_options = [
@@ -752,7 +752,7 @@ class cmd_domain_demote(Command):
         Option("--remove-other-dead-server", help="Dead DC (name or NTDS GUID) "
                "to remove ALL references to (rather than this DC)", type=str),
         Option("--quiet", help="Be quiet", action="store_true"),
-        Option("--verbose", help="Be verbose", action="store_true"),
+        Option("-v", "--verbose", help="Be verbose", action="store_true"),
         ]
 
     takes_optiongroups = {
@@ -1558,7 +1558,7 @@ class cmd_domain_classicupgrade(Command):
         Option("--targetdir", type="string", metavar="DIR",
                   help="Path prefix where the new Samba 4.0 AD domain should be initialised"),
         Option("--quiet", help="Be quiet", action="store_true"),
-        Option("--verbose", help="Be verbose", action="store_true"),
+        Option("-v", "--verbose", help="Be verbose", action="store_true"),
         Option("--dns-backend", type="choice", metavar="NAMESERVER-BACKEND",
                choices=["SAMBA_INTERNAL", "BIND9_FLATFILE", "BIND9_DLZ", "NONE"],
                help="The DNS server backend. SAMBA_INTERNAL is the builtin name server (default), "
@@ -3970,7 +3970,7 @@ class cmd_domain_schema_upgrade(Command):
         Option("-H", "--URL", help="LDB URL for database or target server", type=str,
                metavar="URL", dest="H"),
         Option("--quiet", help="Be quiet", action="store_true"),
-        Option("--verbose", help="Be verbose", action="store_true"),
+        Option("-v", "--verbose", help="Be verbose", action="store_true"),
         Option("--schema", type="choice", metavar="SCHEMA",
                choices=["2012", "2012_R2"],
                help="The schema file to upgrade to. Default is (Windows) 2012_R2.",
@@ -4220,7 +4220,7 @@ class cmd_domain_functional_prep(Command):
         Option("-H", "--URL", help="LDB URL for database or target server", type=str,
                metavar="URL", dest="H"),
         Option("--quiet", help="Be quiet", action="store_true"),
-        Option("--verbose", help="Be verbose", action="store_true"),
+        Option("-v", "--verbose", help="Be verbose", action="store_true"),
         Option("--function-level", type="choice", metavar="FUNCTION_LEVEL",
                choices=["2008_R2", "2012", "2012_R2"],
                help="The schema file to upgrade to. Default is (Windows) 2012_R2.",
-- 
2.17.0


From f53aa31620a6ae1265a625389d38d873d13d3bcc Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Thu, 24 May 2018 17:03:22 +1200
Subject: [PATCH 20/25] samba-tool: add -v to drs --verbose

Sometimes we accept -v for --verbose, sometimes we don't. Let's be a
bit more consistent.

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/netcmd/drs.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/python/samba/netcmd/drs.py b/python/samba/netcmd/drs.py
index a8c096b24bd..21cd1c2ac24 100644
--- a/python/samba/netcmd/drs.py
+++ b/python/samba/netcmd/drs.py
@@ -649,7 +649,7 @@ class cmd_drs_clone_dc_database(Command):
         Option("--targetdir", help="where to store provision (required)", type=str),
         Option("--quiet", help="Be quiet", action="store_true"),
         Option("--include-secrets", help="Also replicate secret values", action="store_true"),
-        Option("--verbose", help="Be verbose", action="store_true")
+        Option("-v", "--verbose", help="Be verbose", action="store_true")
        ]
 
     takes_args = ["domain"]
-- 
2.17.0


From 4c2633e5ce8adb20ae96e28f8ddae053fb2a3343 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Thu, 19 Apr 2018 17:17:28 +1200
Subject: [PATCH 21/25] samba-tool: be consistent in accepting -q for --quiet

Not all commands accept --quiet, and not all of those that do use it.
Some already accept -q, and it is not used anywhere for anything else.

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/netcmd/dbcheck.py |  2 +-
 python/samba/netcmd/domain.py  | 14 +++++++-------
 python/samba/netcmd/drs.py     |  2 +-
 python/samba/netcmd/ntacl.py   |  2 +-
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/python/samba/netcmd/dbcheck.py b/python/samba/netcmd/dbcheck.py
index aec45d26824..210d4a26a1d 100644
--- a/python/samba/netcmd/dbcheck.py
+++ b/python/samba/netcmd/dbcheck.py
@@ -66,7 +66,7 @@ class cmd_dbcheck(Command):
                help="cross naming context boundaries"),
         Option("-v", "--verbose", dest="verbose", action="store_true", default=False,
             help="Print more details of checking"),
-        Option("--quiet", dest="quiet", action="store_true", default=False,
+        Option("-q", "--quiet", action="store_true", default=False,
             help="don't print details of checking"),
         Option("--attrs", dest="attrs", default=None, help="list of attributes to check (space separated)"),
         Option("--reindex", dest="reindex", default=False, action="store_true", help="force database re-index"),
diff --git a/python/samba/netcmd/domain.py b/python/samba/netcmd/domain.py
index 9815a86b4fc..5438537ab1d 100644
--- a/python/samba/netcmd/domain.py
+++ b/python/samba/netcmd/domain.py
@@ -119,7 +119,7 @@ common_provision_join_options = [
            "(default is %s)" % get_default_backend_store()),
     Option("--targetdir", metavar="DIR",
            help="Set target directory (where to store provision)", type=str),
-    Option("--quiet", help="Be quiet", action="store_true"),
+    Option("-q", "--quiet", help="Be quiet", action="store_true"),
 ]
 
 common_join_options = [
@@ -751,7 +751,7 @@ class cmd_domain_demote(Command):
                metavar="URL", dest="H"),
         Option("--remove-other-dead-server", help="Dead DC (name or NTDS GUID) "
                "to remove ALL references to (rather than this DC)", type=str),
-        Option("--quiet", help="Be quiet", action="store_true"),
+        Option("-q", "--quiet", help="Be quiet", action="store_true"),
         Option("-v", "--verbose", help="Be verbose", action="store_true"),
         ]
 
@@ -1045,7 +1045,7 @@ class cmd_domain_level(Command):
     takes_options = [
         Option("-H", "--URL", help="LDB URL for database or target server", type=str,
                metavar="URL", dest="H"),
-        Option("--quiet", help="Be quiet", action="store_true"),
+        Option("-q", "--quiet", help="Be quiet", action="store_true"), # unused
         Option("--forest-level", type="choice", choices=["2003", "2008", "2008_R2", "2012", "2012_R2"],
             help="The forest function level (2003 | 2008 | 2008_R2 | 2012 | 2012_R2)"),
         Option("--domain-level", type="choice", choices=["2003", "2008", "2008_R2", "2012", "2012_R2"],
@@ -1352,7 +1352,7 @@ class cmd_domain_passwordsettings_set(Command):
     takes_options = [
         Option("-H", "--URL", help="LDB URL for database or target server", type=str,
                metavar="URL", dest="H"),
-        Option("--quiet", help="Be quiet", action="store_true"),
+        Option("-q", "--quiet", help="Be quiet", action="store_true"), # unused
         Option("--complexity", type="choice", choices=["on","off","default"],
           help="The password complexity (on | off | default). Default is 'on'"),
         Option("--store-plaintext", type="choice", choices=["on","off","default"],
@@ -1557,7 +1557,7 @@ class cmd_domain_classicupgrade(Command):
                   help="Path to samba classic DC testparm utility from the previous installation.  This allows the default paths of the previous installation to be followed"),
         Option("--targetdir", type="string", metavar="DIR",
                   help="Path prefix where the new Samba 4.0 AD domain should be initialised"),
-        Option("--quiet", help="Be quiet", action="store_true"),
+        Option("-q", "--quiet", help="Be quiet", action="store_true"),
         Option("-v", "--verbose", help="Be verbose", action="store_true"),
         Option("--dns-backend", type="choice", metavar="NAMESERVER-BACKEND",
                choices=["SAMBA_INTERNAL", "BIND9_FLATFILE", "BIND9_DLZ", "NONE"],
@@ -3969,7 +3969,7 @@ class cmd_domain_schema_upgrade(Command):
     takes_options = [
         Option("-H", "--URL", help="LDB URL for database or target server", type=str,
                metavar="URL", dest="H"),
-        Option("--quiet", help="Be quiet", action="store_true"),
+        Option("-q", "--quiet", help="Be quiet", action="store_true"), #unused
         Option("-v", "--verbose", help="Be verbose", action="store_true"),
         Option("--schema", type="choice", metavar="SCHEMA",
                choices=["2012", "2012_R2"],
@@ -4219,7 +4219,7 @@ class cmd_domain_functional_prep(Command):
     takes_options = [
         Option("-H", "--URL", help="LDB URL for database or target server", type=str,
                metavar="URL", dest="H"),
-        Option("--quiet", help="Be quiet", action="store_true"),
+        Option("-q", "--quiet", help="Be quiet", action="store_true"),
         Option("-v", "--verbose", help="Be verbose", action="store_true"),
         Option("--function-level", type="choice", metavar="FUNCTION_LEVEL",
                choices=["2008_R2", "2012", "2012_R2"],
diff --git a/python/samba/netcmd/drs.py b/python/samba/netcmd/drs.py
index 21cd1c2ac24..f852fa17a55 100644
--- a/python/samba/netcmd/drs.py
+++ b/python/samba/netcmd/drs.py
@@ -647,7 +647,7 @@ class cmd_drs_clone_dc_database(Command):
     takes_options = [
         Option("--server", help="DC to join", type=str),
         Option("--targetdir", help="where to store provision (required)", type=str),
-        Option("--quiet", help="Be quiet", action="store_true"),
+        Option("-q", "--quiet", help="Be quiet", action="store_true"),
         Option("--include-secrets", help="Also replicate secret values", action="store_true"),
         Option("-v", "--verbose", help="Be verbose", action="store_true")
        ]
diff --git a/python/samba/netcmd/ntacl.py b/python/samba/netcmd/ntacl.py
index 5aeb667d8fb..5905db622c1 100644
--- a/python/samba/netcmd/ntacl.py
+++ b/python/samba/netcmd/ntacl.py
@@ -51,7 +51,7 @@ class cmd_ntacl_set(Command):
         }
 
     takes_options = [
-        Option("--quiet", help="Be quiet", action="store_true"),
+        Option("-q", "--quiet", help="Be quiet", action="store_true"),
         Option("--xattr-backend", type="choice", help="xattr backend type (native fs or tdb)",
                choices=["native","tdb"]),
         Option("--eadb-file", help="Name of the tdb file where attributes are stored", type="string"),
-- 
2.17.0


From e7f3e4c0fdae5e8ea8feb43595e34a18104f7ec7 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Mon, 12 Mar 2018 11:50:41 +1300
Subject: [PATCH 22/25] samba-tool drs showrepl: remove unused search

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/netcmd/drs.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/python/samba/netcmd/drs.py b/python/samba/netcmd/drs.py
index f852fa17a55..ab849ac113a 100644
--- a/python/samba/netcmd/drs.py
+++ b/python/samba/netcmd/drs.py
@@ -161,7 +161,6 @@ class cmd_drs_showrepl(Command):
 
         # show domain information
         ntds_dn = self.samdb.get_dsServiceName()
-        server_dns = self.samdb.search(base="", scope=ldb.SCOPE_BASE, attrs=["dnsHostName"])[0]['dnsHostName'][0]
 
         (site, server) = drs_parse_ntds_dn(ntds_dn)
         try:
-- 
2.17.0


From 150feef10143bcd9fb0ac150861b0007de0e5c16 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Mon, 12 Mar 2018 12:45:25 +1300
Subject: [PATCH 23/25] samba-tool drs: remove 'server' arg from commands
 without --server

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/netcmd/drs.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/python/samba/netcmd/drs.py b/python/samba/netcmd/drs.py
index ab849ac113a..9b3c6050af9 100644
--- a/python/samba/netcmd/drs.py
+++ b/python/samba/netcmd/drs.py
@@ -148,7 +148,7 @@ class cmd_drs_showrepl(Command):
         return (info_type, info)
 
     def run(self, DC=None, sambaopts=None,
-            credopts=None, versionopts=None, server=None, json=False):
+            credopts=None, versionopts=None, json=False):
 
         self.lp = sambaopts.get_loadparm()
         if DC is None:
@@ -286,7 +286,7 @@ class cmd_drs_kcc(Command):
     takes_args = ["DC?"]
 
     def run(self, DC=None, sambaopts=None,
-            credopts=None, versionopts=None, server=None):
+            credopts=None, versionopts=None):
 
         self.lp = sambaopts.get_loadparm()
         if DC is None:
@@ -400,7 +400,7 @@ class cmd_drs_replicate(Command):
     def run(self, DEST_DC, SOURCE_DC, NC,
             add_ref=False, sync_forced=False, sync_all=False, full_sync=False,
             local=False, local_online=False, async_op=False, single_object=False,
-            sambaopts=None, credopts=None, versionopts=None, server=None):
+            sambaopts=None, credopts=None, versionopts=None):
 
         self.server = DEST_DC
         self.lp = sambaopts.get_loadparm()
@@ -485,7 +485,7 @@ class cmd_drs_bind(Command):
     takes_args = ["DC?"]
 
     def run(self, DC=None, sambaopts=None,
-            credopts=None, versionopts=None, server=None):
+            credopts=None, versionopts=None):
 
         self.lp = sambaopts.get_loadparm()
         if DC is None:
-- 
2.17.0


From 91537dbcafc550cf9bef2907364bfd426c77bb42 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Thu, 19 Apr 2018 14:12:57 +1200
Subject: [PATCH 24/25] python/colour: add colourizing and switch functions

When samba.colour is first imported, the function

colour.c_BLUE("samba")

will give you the string "\033[1;34msamba\033[0m", which will show up
as blue on an ANSI terminal. If you then go:

colour.switch_colour_off()
colour.c_BLUE("samba")

the c_BLUE call will return the uncoloured string "samba".

This is so things like samba-tool can do this sort of thing:

    if not os.isatty(self.outf):
        switch_colour_off()

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/colour.py | 56 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 48 insertions(+), 8 deletions(-)

diff --git a/python/samba/colour.py b/python/samba/colour.py
index b3d9a718e0e..92af2fdef80 100644
--- a/python/samba/colour.py
+++ b/python/samba/colour.py
@@ -18,15 +18,13 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 # The 4 bit colours are available as global variables with names like
-# RED, DARK_RED, REV_RED (for red background), and REV_DARK_RED.
+# RED, DARK_RED, REV_RED (for red background), and REV_DARK_RED. If
+# switch_colour_off() is called, these names will all point to the
+# empty string. switch_colour_on() restores the default values.
 #
 # The 256-colour codes are obtained using xterm_256_color(n), where n
 # is the number of the desired colour.
 
-# C_NORMAL resets to normal, whatever that is
-C_NORMAL = "\033[0m"
-
-UNDERLINE = "\033[4m"
 
 def _gen_ansi_colours():
     g = globals()
@@ -37,11 +35,53 @@ def _gen_ansi_colours():
         g['REV_' + name] = "\033[1;4%dm" % i
         g['REV_DARK_' + name] = "\033[4%dm" % i
 
+    # kcc.debug uses these aliases (which make visual sense)
+    g['PURPLE'] = DARK_MAGENTA
+    g['GREY'] = DARK_WHITE
+
+    # C_NORMAL resets to normal, whatever that is
+    g['C_NORMAL'] = "\033[0m"
+
+    # Non-colour ANSI codes.
+    g['UNDERLINE'] = "\033[4m"
+
+
 _gen_ansi_colours()
 
-# kcc.debug uses these aliases (which make visual sense)
-PURPLE = DARK_MAGENTA
-GREY = DARK_WHITE
+# Generate functions that colour a string. The functions look like
+# this:
+#
+#    c_BLUE("hello")  # "\033[1;34mhello\033[0m" -> blue text
+#    c_DARK_RED(3)    # 3 will be stringified and coloured
+#
+# but if colour is switched off, no colour codes are added.
+#
+#    c_BLUE("hello")  # "hello"
+#
+# The definition of the functions looks a little odd, because we want
+# to bake in the name of the colour but not its actual value.
+
+for _k in list(globals().keys()):
+    if _k.isupper():
+        def _f(s, name=_k):
+            return "%s%s%s" % (globals()[name], s, C_NORMAL)
+        globals()['c_%s' % _k] = _f
+
+del _k, _f
+
+
+def switch_colour_off():
+    """Convert all the ANSI colour codes into empty strings."""
+    g = globals()
+    for k, v in list(g.items()):
+        if k.isupper() and isinstance(v, str) and v.startswith('\033'):
+            g[k] = ''
+
+
+def switch_colour_on():
+    """Regenerate all the ANSI colour codes."""
+    _gen_ansi_colours()
+
 
 def xterm_256_colour(n, bg=False, bold=False):
     weight = '01;' if bold else ''
-- 
2.17.0


From c64510d26b347caaca0b82a13948d2165a37b108 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Date: Thu, 19 Apr 2018 14:15:25 +1200
Subject: [PATCH 25/25] sambatool: heuristics to decided whether colour is
 wanted

The easy cases are --color=yes and --color=no.

With --color=auto, we use color if it seems we're writing to a TTY.

Signed-off-by: Douglas Bagnall <douglas.bagnall at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 python/samba/netcmd/__init__.py | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/python/samba/netcmd/__init__.py b/python/samba/netcmd/__init__.py
index 9df096d5bd3..9b144d97f43 100644
--- a/python/samba/netcmd/__init__.py
+++ b/python/samba/netcmd/__init__.py
@@ -18,6 +18,7 @@
 
 import optparse, samba
 from samba import getopt as options
+from samba import colour
 from ldb import LdbError
 import sys, traceback
 import textwrap
@@ -189,6 +190,29 @@ class Command(object):
         logger.addHandler(logging.StreamHandler(self.errf))
         return logger
 
+    def apply_colour_choice(self, requested):
+        """Heuristics to work out whether the user wants colour output, from a
+        --color=yes|no|auto option. This alters the ANSI 16 bit colour
+        "constants" in the colour module to be either real colours or empty
+        strings.
+        """
+        requested = requested.lower()
+        if requested == 'no':
+            colour.switch_colour_off()
+
+        elif requested == 'yes':
+            colour.switch_colour_on()
+
+        elif requested == 'auto':
+            if (hasattr(self.outf, 'isatty') and self.outf.isatty()):
+                colour.switch_colour_on()
+            else:
+                colour.switch_colour_off()
+
+        else:
+            raise CommandError("Unknown --color option: %s "
+                               "please choose from yes, no, auto")
+
 
 class SuperCommand(Command):
     """A samba-tool command with subcommands."""
-- 
2.17.0



More information about the samba-technical mailing list