From 5226bf99f18008cb956a9b2ae0dc5ea2a5fc4a7f Mon Sep 17 00:00:00 2001
From: Mark Reynolds <mreynolds@redhat.com>
Date: Jun 14 2018 00:08:17 +0000
Subject: Ticket 49712 - lib389 CLI tools should return a result code on failures


Description:  I've also included the work for 49775 in this patch since
              there was a lot of overldap.

              For dsctl functions we need to check for True and False in
              order to detect an error.  For dsconf & dsidm we need to
              catch exceptions.  Once an error is detected we return error
              code (1).

              The changes for 49775 was to use the default archive directory
              if one was not specified to db2bak, and use the default ldif
              location for db2ldif.  This how the old tools worked, no
              reason not to carry over this convenience.  Also the format
              used for the file name (Instance name + Date/Time) is the same
              as the old cli tools.

              Also did some pep8 cleanup.

https://pagure.io/389-ds-base/issue/49712

Reviewed by: spichugi(Thanks!)

---

diff --git a/ldap/servers/slapd/back-ldbm/archive.c b/ldap/servers/slapd/back-ldbm/archive.c
index 4a9ce3d..1b06cf9 100644
--- a/ldap/servers/slapd/back-ldbm/archive.c
+++ b/ldap/servers/slapd/back-ldbm/archive.c
@@ -366,6 +366,7 @@ ldbm_back_ldbm2archive(Slapi_PBlock *pb)
             slapi_task_log_notice(task, "mkdir(%s) failed; errno %i (%s)",
                                   directory, errno, msg ? msg : "unknown");
         }
+        return_value = -1;
         goto err;
     }
 
diff --git a/src/lib389/cli/dsconf b/src/lib389/cli/dsconf
index b4dd533..3292219 100755
--- a/src/lib389/cli/dsconf
+++ b/src/lib389/cli/dsconf
@@ -82,7 +82,7 @@ if __name__ == '__main__':
     cli_whoami.create_parser(subparsers)
     cli_referint.create_parser(subparsers)
     cli_automember.create_parser(subparsers)
-    
+
     args = parser.parse_args()
 
     log = reset_get_logger('dsconf', args.verbose)
@@ -111,26 +111,26 @@ if __name__ == '__main__':
     # Connect
     # We don't need a basedn, because the config objects derive it properly
     inst = None
-    if args.verbose:
+    result = False
+    try:
         inst = connect_instance(dsrc_inst=dsrc_inst, verbose=args.verbose)
-        args.func(inst, None, log, args)
-        if not args.json:
+        result = args.func(inst, None, log, args)
+        if args.verbose:
             log.info("Command successful.")
-    else:
-        try:
-            inst = connect_instance(dsrc_inst=dsrc_inst, verbose=args.verbose)
-            args.func(inst, None, log, args)
-            if not args.json:
-                log.info("Command successful.")
-        except ldap.LDAPError as e:
-            #log.debug(e, exc_info=True)
-            if args and args.json:
-                print(e)
-            else:
-                log.error("Error!: %s" % str(e))
+    except Exception as e:
+        log.debug(e, exc_info=True)
+        if args and args.json:
+            print(e)
+        else:
+            log.error("Error!: %s" % str(e))
+        result = False
+
     disconnect_instance(inst)
 
     # Done!
     log.debug("dsconf is brought to you by the letter H and the number 25.")
 
+    if result is False:
+        sys.exit(1)
+
 
diff --git a/src/lib389/cli/dscreate b/src/lib389/cli/dscreate
index 2abb67c..4e20cfc 100755
--- a/src/lib389/cli/dscreate
+++ b/src/lib389/cli/dscreate
@@ -29,13 +29,15 @@ if __name__ == '__main__':
 
     fromfile_parser = subparsers.add_parser('fromfile', help="Create an instance of Directory Server from an inf answer file")
     fromfile_parser.add_argument('file', help="Inf file to use with prepared answers. You can generate an example of this with 'dscreate example'")
-    fromfile_parser.add_argument('-n', '--dryrun', help="Validate system and configurations only. Do not alter the system.", action='store_true', default=False)
+    fromfile_parser.add_argument('-n', '--dryrun', help="Validate system and configurations only. Do not alter the system.",
+                                 action='store_true', default=False)
     fromfile_parser.add_argument('--IsolemnlyswearthatIamuptonogood', dest="ack",
-                        help="""You are here likely here by mistake! You want setup-ds.pl!
+                                 help="""You are here likely here by mistake! You want setup-ds.pl!
 By setting this value you acknowledge and take responsibility for the fact this command is UNTESTED and NOT READY. You are ON YOUR OWN!
 """,
-                        action='store_true', default=False)
-    fromfile_parser.add_argument('-c', '--containerised', help="Indicate to the installer that this is running in a container. Used to disable systemd native components, even if they are installed.", action='store_true', default=False)
+                                 action='store_true', default=False)
+    fromfile_parser.add_argument('-c', '--containerised', help="Indicate to the installer that this is running in a container. Used to disable systemd native components, even if they are installed.",
+                                 action='store_true', default=False)
     fromfile_parser.set_defaults(func=cli_instance.instance_create)
 
     example_parser = subparsers.add_parser('example', help="Display an example ini answer file, with comments")
@@ -62,19 +64,12 @@ By setting this value you acknowledge and take responsibility for the fact this 
 
     result = False
 
-    if args.verbose:
+    try:
         result = args.func(inst, log, args)
-    else:
-        try:
-            result = args.func(inst, log, args)
-        except Exception as e:
-            log.debug(e, exc_info=True)
-            log.error("Error: %s" % str(e))
-
-    if result is True:
-        log.info('FINISH: Command succeeded')
-    elif result is False:
-        log.info('FAIL: Command failed. See output for details.')
+    except Exception as e:
+        log.debug(e, exc_info=True)
+        log.error("Error: %s" % str(e))
+        result = False
 
     # Done!
     log.debug("dscreate is brought to you by the letter S and the number 22.")
diff --git a/src/lib389/cli/dsctl b/src/lib389/cli/dsctl
index 2f13e3f..83b07c3 100755
--- a/src/lib389/cli/dsctl
+++ b/src/lib389/cli/dsctl
@@ -86,23 +86,14 @@ if __name__ == '__main__':
     inst.allocate(insts[0])
     log.debug('Instance allocated')
 
-    if args.verbose:
+    try:
         result = args.func(inst, log, args)
-        log.info("Command successful.")
-    else:
-        try:
-            result = args.func(inst, log, args)
-            log.info("Command successful.")
-        except Exception as e:
-            log.debug(e, exc_info=True)
-            log.error("Error: %s" % str(e))
+    except Exception as e:
+        log.debug(e, exc_info=True)
+        log.error("Error: %s" % str(e))
+        result = False
     disconnect_instance(inst)
 
-    if result is True:
-        log.info('FINISH: Command succeeded')
-    elif result is False:
-        log.info('FAIL: Command failed. See output for details.')
-
     # Done!
     log.debug("dsctl is brought to you by the letter R and the number 27.")
 
diff --git a/src/lib389/cli/dsidm b/src/lib389/cli/dsidm
index 15d0ad5..8d1a17d 100755
--- a/src/lib389/cli/dsidm
+++ b/src/lib389/cli/dsidm
@@ -104,21 +104,20 @@ if __name__ == '__main__':
 
     # Connect
     inst = None
-    if args.verbose:
+    result = False
+    try:
         inst = connect_instance(dsrc_inst=dsrc_inst, verbose=args.verbose)
-        args.func(inst, dsrc_inst['basedn'], log, args)
-        log.info("Command successful.")
-    else:
-        try:
-            inst = connect_instance(dsrc_inst=dsrc_inst, verbose=args.verbose)
-            args.func(inst, dsrc_inst['basedn'], log, args)
+        result = args.func(inst, dsrc_inst['basedn'], log, args)
+        if args.verbose:
             log.info("Command successful.")
-        except Exception as e:
-            log.debug(e, exc_info=True)
-            log.error("Error: %s" % str(e))
-    disconnect_instance(inst)
+    except Exception as e:
+        log.debug(e, exc_info=True)
+        log.error("Error: %s" % str(e))
+        result = False
 
+    disconnect_instance(inst)
 
     log.debug("dsidm is brought to you by the letter E and the number 26.")
 
-
+    if result is False:
+        sys.exit(1)
diff --git a/src/lib389/lib389/__init__.py b/src/lib389/lib389/__init__.py
index 92a3805..be2a844 100644
--- a/src/lib389/lib389/__init__.py
+++ b/src/lib389/lib389/__init__.py
@@ -1663,7 +1663,7 @@ class DirSrv(SimpleLDAPObject, object):
 
     def get_ldaps_uri(self):
         """Return what our ldaps (TLS) uri would be for this instance
-        
+
         :returns: The string of the servers ldaps (TLS) uri.
         """
         return 'ldaps://%s:%s' % (self.host, self.sslport)
@@ -1674,7 +1674,7 @@ class DirSrv(SimpleLDAPObject, object):
 
     def get_ldap_uri(self):
         """Return what our ldap uri would be for this instance
-        
+
         :returns: The string of the servers ldap uri.
         """
         return 'ldap://%s:%s' % (self.host, self.port)
@@ -2777,8 +2777,13 @@ class DirSrv(SimpleLDAPObject, object):
         if encrypt:
             cmd.append('-E')
 
-        result = subprocess.check_output(cmd)
-        u_result = ensure_str(result)
+        try:
+            result = subprocess.check_output(cmd, stderr=subprocess.STDOUT, encoding='utf-8')
+            u_result = ensure_str(result)
+        except subprocess.CalledProcessError as e:
+            self.log.debug("Command: {} failed with the return code {} and the error {}".format(
+                           format_cmd_list(cmd), e.returncode, e.output))
+            return False
 
         self.log.debug("ldif2db output: BEGIN")
         for line in u_result.split("\n"):
@@ -2830,12 +2835,24 @@ class DirSrv(SimpleLDAPObject, object):
             cmd.append('-E')
         if repl_data:
             cmd.append('-r')
-        if outputfile:
+        if outputfile is not None:
             cmd.append('-a')
             cmd.append(outputfile)
-
-        result = subprocess.check_output(cmd)
-        u_result = ensure_str(result)
+        else:
+            # No output file specified.  Use the default ldif location/name
+            cmd.append('-a')
+            if bename:
+                ldifname = "/" + self.serverid + "-" + bename + "-" + datetime.now().strftime("%Y_%m_%d_%H_%M_%S")
+            else:
+                ldifname = "/" + self.serverid + "-" + datetime.now().strftime("%Y_%m_%d_%H_%M_%S")
+            cmd.append(self.get_ldif_dir() + ldifname)
+        try:
+            result = subprocess.check_output(cmd, stderr=subprocess.STDOUT, encoding='utf-8')
+            u_result = ensure_str(result)
+        except subprocess.CalledProcessError as e:
+            self.log.debug("Command: {} failed with the return code {} and the error {}".format(
+                           format_cmd_list(cmd), e.returncode, e.output))
+            return False
 
         self.log.debug("db2ldif output: BEGIN")
         for line in u_result.split("\n"):
@@ -2861,13 +2878,18 @@ class DirSrv(SimpleLDAPObject, object):
             self.log.error("bak2db: backup directory missing")
             return False
 
-        result = subprocess.check_output([
-            prog,
-            'archive2db',
-            '-a', archive_dir,
-            '-D', self.get_config_dir()
-        ])
-        u_result = ensure_str(result)
+        try:
+            result = subprocess.check_output([
+                prog,
+                'archive2db',
+                '-a', archive_dir,
+                '-D', self.get_config_dir()
+            ], stderr=subprocess.STDOUT, encoding='utf-8')
+            u_result = ensure_str(result)
+        except subprocess.CalledProcessError as e:
+            self.log.debug("Command: {} failed with the return code {} and the error {}".format(
+                           format_cmd_list(cmd), e.returncode, e.output))
+            return False
 
         self.log.debug("bak2db output: BEGIN")
         for line in u_result.split("\n"):
@@ -2888,17 +2910,24 @@ class DirSrv(SimpleLDAPObject, object):
             self.log.error("db2bak: Can not operate while directory server is running")
             return False
 
-        if not archive_dir:
-            self.log.error("db2bak: archive directory missing")
-            return False
+        if archive_dir is None:
+            # Use the instance name and date/time as the default backup name
+            archive_dir = self.get_bak_dir() + "/" + self.serverid + "-" + datetime.now().strftime("%Y_%m_%d_%H_%M_%S")
+        elif archive_dir[0] != "/":
+            # Relative path, append it to the bak directory
+            archive_dir = self.get_bak_dir() + "/" + archive_dir
 
-        result = subprocess.check_output([
-            prog,
-            'db2archive',
-            '-a', archive_dir,
-            '-D', self.get_config_dir()
-        ])
-        u_result = ensure_str(result)
+        try:
+            result = subprocess.check_output([
+                prog,
+                'db2archive',
+                '-a', archive_dir,
+                '-D', self.get_config_dir()
+            ], stderr=subprocess.STDOUT, encoding='utf-8')
+            u_result = ensure_str(result)
+        except subprocess.CalledProcessError as e:
+            self.log.debug("Command: {} failed with the return code {} and the error {}".format(
+                        format_cmd_list(cmd), e.returncode, e.output))
 
         self.log.debug("db2bak output: BEGIN")
         for line in u_result.split("\n"):
@@ -2937,7 +2966,6 @@ class DirSrv(SimpleLDAPObject, object):
         cmd.append('-D')
         cmd.append(self.get_config_dir())
 
-
         if bename:
             cmd.append('-n')
             cmd.append(bename)
@@ -2957,15 +2985,20 @@ class DirSrv(SimpleLDAPObject, object):
             cmd.append('-T')
             cmd.append(vlvTag)
 
-        result = subprocess.check_output(cmd)
-        u_result = ensure_str(result)
+        try:
+            result = subprocess.check_output(cmd, stderr=subprocess.STDOUT, encoding='utf-8')
+            u_result = ensure_str(result)
+        except subprocess.CalledProcessError as e:
+            self.log.debug("Command: {} failed with the return code {} and the error {}".format(
+                           format_cmd_list(cmd), e.returncode, e.output))
+            return False
 
         self.log.debug("db2index output: BEGIN")
         for line in u_result.split("\n"):
             self.log.debug(line)
         self.log.debug("db2index output: END")
 
-        return result
+        return True
 
     def dbscan(self, bename=None, index=None, key=None, width=None, isRaw=False):
         """Wrapper around dbscan tool that analyzes and extracts information
diff --git a/src/lib389/lib389/cli_base/__init__.py b/src/lib389/lib389/cli_base/__init__.py
index 2026b1e..397778f 100644
--- a/src/lib389/lib389/cli_base/__init__.py
+++ b/src/lib389/lib389/cli_base/__init__.py
@@ -17,6 +17,7 @@ from lib389.properties import *
 
 MAJOR, MINOR, _, _, _ = sys.version_info
 
+
 # REALLY PYTHON 3? REALLY???
 def _input(msg):
     if MAJOR >= 3:
@@ -43,6 +44,7 @@ def _get_arg(args, msg=None, hidden=False, confirm=False):
         else:
             return _input("%s : " % msg)
 
+
 def _get_args(args, kws):
     kwargs = {}
     while len(kws) > 0:
@@ -57,6 +59,7 @@ def _get_args(args, kws):
                 kwargs[kw] = _input("%s : " % msg)
     return kwargs
 
+
 # This is really similar to get_args, but generates from an array
 def _get_attributes(args, attrs):
     kwargs = {}
@@ -81,6 +84,7 @@ def _warn(data, msg=None):
         raise Exception("Not sure if want")
     return data
 
+
 # We'll need another of these that does a "connect via instance name?"
 def connect_instance(dsrc_inst, verbose):
     dsargs = dsrc_inst['args']
@@ -99,14 +103,17 @@ def connect_instance(dsrc_inst, verbose):
             starttls=dsrc_inst['starttls'], connOnly=True)
     return ds
 
+
 def disconnect_instance(inst):
     if inst is not None:
         inst.close()
 
+
 def populate_attr_arguments(parser, attributes):
     for attr in attributes:
         parser.add_argument('--%s' % attr, nargs='?', help="Value of %s" % attr)
 
+
 def _generic_list(inst, basedn, log, manager_class, args=None):
     mc = manager_class(inst, basedn)
     ol = mc.list()
@@ -128,30 +135,33 @@ def _generic_list(inst, basedn, log, manager_class, args=None):
         if args and args.json:
             print(json.dumps(json_result))
 
+
 # Display these entries better!
 def _generic_get(inst, basedn, log, manager_class, selector, args=None):
     mc = manager_class(inst, basedn)
     if args and args.json:
          o = mc.get(selector, json=True)
-         print(o);
+         print(o)
     else:
         o = mc.get(selector)
         o_str = o.display()
         log.info(o_str)
 
+
 def _generic_get_dn(inst, basedn, log, manager_class, dn, args=None):
     mc = manager_class(inst, basedn)
     o = mc.get(dn=dn)
     o_str = o.display()
     log.info(o_str)
 
+
 def _generic_create(inst, basedn, log, manager_class, kwargs, args=None):
     mc = manager_class(inst, basedn)
     o = mc.create(properties=kwargs)
     o_str = o.__unicode__()
-
     log.info('Successfully created %s' % o_str)
 
+
 def _generic_delete(inst, basedn, log, object_class, dn, args=None):
     # Load the oc direct
     o = object_class(inst, dn)
@@ -190,6 +200,7 @@ class LogCapture(logging.Handler):
     def flush(self):
         self.outputs = []
 
+
 class FakeArgs(object):
     def __init__(self):
         pass
@@ -207,6 +218,7 @@ log_verbose_handler.setFormatter(
     logging.Formatter('%(levelname)s: %(message)s')
 )
 
+
 def reset_get_logger(name, verbose=False):
     """Reset the python logging system for STDOUT, and attach a new
     console logger with cli expected formatting.
diff --git a/src/lib389/lib389/cli_conf/backend.py b/src/lib389/lib389/cli_conf/backend.py
index ac171ba..f378474 100644
--- a/src/lib389/lib389/cli_conf/backend.py
+++ b/src/lib389/lib389/cli_conf/backend.py
@@ -7,8 +7,6 @@
 # --- END COPYRIGHT BLOCK ---
 
 from lib389.backend import Backend, Backends
-import argparse
-
 from lib389.cli_base import (
     populate_attr_arguments,
     _generic_list,
@@ -26,24 +24,28 @@ SINGULAR = Backend
 MANY = Backends
 RDN = 'cn'
 
+
 def backend_list(inst, basedn, log, args):
     _generic_list(inst, basedn, log.getChild('backend_list'), MANY, args)
 
+
 def backend_get(inst, basedn, log, args):
-    rdn = _get_arg( args.selector, msg="Enter %s to retrieve" % RDN)
+    rdn = _get_arg(args.selector, msg="Enter %s to retrieve" % RDN)
     _generic_get(inst, basedn, log.getChild('backend_get'), MANY, rdn, args)
 
+
 def backend_get_dn(inst, basedn, log, args):
-    dn = _get_arg( args.dn, msg="Enter dn to retrieve")
+    dn = _get_arg(args.dn, msg="Enter dn to retrieve")
     _generic_get_dn(inst, basedn, log.getChild('backend_get_dn'), MANY, dn, args)
 
+
 def backend_create(inst, basedn, log, args):
-    args.json = True
     kwargs = _get_attributes(args, Backend._must_attributes)
     _generic_create(inst, basedn, log.getChild('backend_create'), MANY, kwargs, args)
 
+
 def backend_delete(inst, basedn, log, args, warn=True):
-    dn = _get_arg( args.dn, msg="Enter dn to delete")
+    dn = _get_arg(args.dn, msg="Enter dn to delete")
     if warn:
         _warn(dn, msg="Deleting %s %s" % (SINGULAR.__name__, dn))
     _generic_delete(inst, basedn, log.getChild('backend_delete'), SINGULAR, dn, args)
diff --git a/src/lib389/lib389/cli_conf/directory_manager.py b/src/lib389/lib389/cli_conf/directory_manager.py
index c3015d9..a7fb091 100644
--- a/src/lib389/lib389/cli_conf/directory_manager.py
+++ b/src/lib389/lib389/cli_conf/directory_manager.py
@@ -7,19 +7,17 @@
 # --- END COPYRIGHT BLOCK ---
 
 from lib389.idm.directorymanager import DirectoryManager
-
 from lib389.cli_base import _get_arg
 
-import argparse
 
 def password_change(inst, basedn, log, args):
     # Due to an issue, we can't use extended op, so we have to
     # submit the password directly to the field.
-
     password = _get_arg(args.password, msg="Enter new directory manager password", hidden=True, confirm=True)
     dm = DirectoryManager(inst)
     dm.change_password(password)
 
+
 def create_parsers(subparsers):
     directory_manager_parser = subparsers.add_parser('directory_manager', help="Manage the directory manager account")
 
diff --git a/src/lib389/lib389/cli_conf/health.py b/src/lib389/lib389/cli_conf/health.py
index 21c41bc..040d856 100644
--- a/src/lib389/lib389/cli_conf/health.py
+++ b/src/lib389/lib389/cli_conf/health.py
@@ -6,8 +6,6 @@
 # See LICENSE for details.
 # --- END COPYRIGHT BLOCK ---
 
-import argparse
-
 from lib389.backend import Backend, Backends
 from lib389.config import Encryption, Config
 from lib389 import plugins
@@ -24,6 +22,7 @@ CHECK_OBJECTS = [
     plugins.ReferentialIntegrityPlugin
 ]
 
+
 def _format_check_output(log, result):
     log.info("==== DS Lint Error: %s ====" % result['dsle'])
     log.info(" Severity: %s " % result['severity'])
@@ -35,6 +34,7 @@ def _format_check_output(log, result):
     log.info(" Resolution:")
     log.info(result['fix'])
 
+
 def health_check_run(inst, basedn, log, args):
     log.info("Beginning lint report, this could take a while ...")
     report = []
@@ -55,6 +55,7 @@ def health_check_run(inst, basedn, log, args):
     for item in report:
         _format_check_output(log, item)
 
+
 def create_parser(subparsers):
     run_healthcheck_parser = subparsers.add_parser('healthcheck', help="Run a healthcheck report on your Directory Server instance. This is a safe, read only operation.")
     run_healthcheck_parser.set_defaults(func=health_check_run)
diff --git a/src/lib389/lib389/cli_conf/plugin.py b/src/lib389/lib389/cli_conf/plugin.py
index 81f088c..3794988 100644
--- a/src/lib389/lib389/cli_conf/plugin.py
+++ b/src/lib389/lib389/cli_conf/plugin.py
@@ -7,8 +7,6 @@
 # --- END COPYRIGHT BLOCK ---
 
 from lib389.plugins import Plugin, Plugins
-import argparse
-
 from lib389.cli_base import (
     _generic_list,
     _generic_get,
@@ -29,26 +27,30 @@ RDN = 'cn'
 def plugin_list(inst, basedn, log, args):
     _generic_list(inst, basedn, log.getChild('plugin_list'), MANY, args)
 
+
 def plugin_get(inst, basedn, log, args):
-    rdn = _get_arg( args.selector, msg="Enter %s to retrieve" % RDN)
+    rdn = _get_arg(args.selector, msg="Enter %s to retrieve" % RDN)
     _generic_get(inst, basedn, log.getChild('plugin_get'), MANY, rdn, args)
 
+
 def plugin_get_dn(inst, basedn, log, args):
-    dn = _get_arg( args.dn, msg="Enter dn to retrieve")
+    dn = _get_arg(args.dn, msg="Enter dn to retrieve")
     _generic_get_dn(inst, basedn, log.getChild('plugin_get_dn'), MANY, dn, args)
 
+
 # Plugin enable
 def plugin_enable(inst, basedn, log, args):
-    dn = _get_arg( args.dn, msg="Enter plugin dn to enable")
+    dn = _get_arg(args.dn, msg="Enter plugin dn to enable")
     mc = MANY(inst, basedn)
     o = mc.get(dn=dn)
     o.enable()
     o_str = o.display()
     log.info('Enabled %s', o_str)
 
+
 # Plugin disable
 def plugin_disable(inst, basedn, log, args, warn=True):
-    dn = _get_arg( args.dn, msg="Enter plugin dn to disable")
+    dn = _get_arg(args.dn, msg="Enter plugin dn to disable")
     if warn:
         _warn(dn, msg="Disabling %s %s" % (SINGULAR.__name__, dn))
     mc = MANY(inst, basedn)
@@ -57,32 +59,38 @@ def plugin_disable(inst, basedn, log, args, warn=True):
     o_str = o.display()
     log.info('Disabled %s', o_str)
 
+
 # Plugin configure?
 def plugin_configure(inst, basedn, log, args):
     pass
 
+
 def generic_show(inst, basedn, log, args):
     """Display plugin configuration."""
     plugin = args.plugin_cls(inst)
     log.info(plugin.display())
 
+
 def generic_enable(inst, basedn, log, args):
     plugin = args.plugin_cls(inst)
     plugin.enable()
     log.info("Enabled %s", plugin.rdn)
 
+
 def generic_disable(inst, basedn, log, args):
     plugin = args.plugin_cls(inst)
     plugin.disable()
     log.info("Disabled %s", plugin.rdn)
 
+
 def generic_status(inst, basedn, log, args):
     plugin = args.plugin_cls(inst)
-    if plugin.status() == True:
+    if plugin.status() is True:
         log.info("%s is enabled", plugin.rdn)
     else:
         log.info("%s is disabled", plugin.rdn)
 
+
 def add_generic_plugin_parsers(subparser, plugin_cls):
     show_parser = subparser.add_parser('show', help='display plugin configuration')
     show_parser.set_defaults(func=generic_show, plugin_cls=plugin_cls)
diff --git a/src/lib389/lib389/cli_ctl/dbtasks.py b/src/lib389/lib389/cli_ctl/dbtasks.py
index e41768c..1b6c370 100644
--- a/src/lib389/lib389/cli_ctl/dbtasks.py
+++ b/src/lib389/lib389/cli_ctl/dbtasks.py
@@ -6,26 +6,50 @@
 # See LICENSE for details.
 # --- END COPYRIGHT BLOCK ---
 
+
 def dbtasks_db2index(inst, log, args):
-    inst.db2index(bename=args.backend)
+    if not inst.db2index(bename=args.backend):
+        log.fatal("db2index failed")
+        return False
+    else:
+        log.info("db2index successful")
+
 
 def dbtasks_db2bak(inst, log, args):
     # Needs an output name?
-    inst.db2bak(args.archive)
-    log.info("db2bak successful")
+    if not inst.db2bak(args.archive):
+        log.fatal("db2bak failed")
+        return False
+    else:
+        log.info("db2bak successful")
+
 
 def dbtasks_bak2db(inst, log, args):
     # Needs the archive to restore.
-    inst.bak2db(args.archive)
-    log.info("bak2db successful")
+    if not inst.bak2db(args.archive):
+        log.fatal("bak2db failed")
+        return False
+    else:
+        log.info("bak2db successful")
+
 
 def dbtasks_db2ldif(inst, log, args):
-    inst.db2ldif(bename=args.backend, encrypt=args.encrypted, repl_data=args.replication, outputfile=args.ldif, suffixes=None, excludeSuffixes=None)
-    log.info("db2ldif successful")
+    if not inst.db2ldif(bename=args.backend, encrypt=args.encrypted, repl_data=args.replication,
+                        outputfile=args.ldif, suffixes=None, excludeSuffixes=None):
+        log.fatal("db2ldif failed")
+        return False
+    else:
+        log.info("db2ldif successful")
+
 
 def dbtasks_ldif2db(inst, log, args):
-    inst.ldif2db(bename=args.backend, encrypt=args.encrypted, import_file=args.ldif, suffixes=None, excludeSuffixes=None)
-    log.info("ldif2db successful")
+    if not inst.ldif2db(bename=args.backend, encrypt=args.encrypted, import_file=args.ldif,
+                        suffixes=None, excludeSuffixes=None):
+        log.fatal("ldif2db failed")
+        return False
+    else:
+        log.info("ldif2db successful")
+
 
 def create_parser(subcommands):
     db2index_parser = subcommands.add_parser('db2index', help="Initialise a reindex of the server database. The server must be stopped for this to proceed.")
@@ -34,13 +58,15 @@ def create_parser(subcommands):
     db2index_parser.set_defaults(func=dbtasks_db2index)
 
     db2bak_parser = subcommands.add_parser('db2bak', help="Initialise a BDB backup of the database. The server must be stopped for this to proceed.")
-    db2bak_parser.add_argument('archive', help="The destination for the archive. This will be created during the db2bak process.")
+    db2bak_parser.add_argument('archive', help="The destination for the archive. This will be created during the db2bak process.",
+                               nargs='?', default=None)
     db2bak_parser.set_defaults(func=dbtasks_db2bak)
 
     db2ldif_parser = subcommands.add_parser('db2ldif', help="Initialise an LDIF dump of the database. The server must be stopped for this to proceed.")
     db2ldif_parser.add_argument('backend', help="The backend to output as an LDIF. IE userRoot")
-    db2ldif_parser.add_argument('ldif', help="The path to the ldif output location.")
-    db2ldif_parser.add_argument('--replication', help="Export replication information, suitable for importing on a new consumer or backups.", default=False, action='store_true')
+    db2ldif_parser.add_argument('ldif', help="The path to the ldif output location.", nargs='?', default=None)
+    db2ldif_parser.add_argument('--replication', help="Export replication information, suitable for importing on a new consumer or backups.",
+                                default=False, action='store_true')
     db2ldif_parser.add_argument('--encrypted', help="Export encrypted attributes", default=False, action='store_true')
     db2ldif_parser.set_defaults(func=dbtasks_db2ldif)
 
diff --git a/src/lib389/lib389/cli_ctl/instance.py b/src/lib389/lib389/cli_ctl/instance.py
index 507a457..1f35f07 100644
--- a/src/lib389/lib389/cli_ctl/instance.py
+++ b/src/lib389/lib389/cli_ctl/instance.py
@@ -18,6 +18,7 @@ import sys
 
 from lib389.instance.options import General2Base, Slapd2Base
 
+
 def instance_list(inst, log, args):
     instances = inst.list(all=True)
 
@@ -31,26 +32,31 @@ def instance_list(inst, log, args):
         log.info(e)
         log.info("Perhaps you need to be a different user?")
 
+
 def instance_restart(inst, log, args):
     inst.restart(post_open=False)
 
+
 def instance_start(inst, log, args):
     inst.start(post_open=False)
 
+
 def instance_stop(inst, log, args):
     inst.stop()
 
+
 def instance_status(inst, log, args):
     if inst.status() is True:
         log.info("Instance is running")
     else:
         log.info("Instance is not running")
 
+
 def instance_create(inst, log, args):
     if args.containerised:
         log.debug("Containerised features requested.")
     sd = SetupDs(args.verbose, args.dryrun, log, args.containerised)
-    ### If args.file is not set, we need to interactively get answers!
+    ### TODO - If args.file is not set, we need to interactively get answers!
     if sd.create_from_inf(args.file):
         # print("Sucessfully created instance")
         return True
@@ -58,6 +64,7 @@ def instance_create(inst, log, args):
         # print("Failed to create instance")
         return False
 
+
 def instance_example(inst, log, args):
     print("""
 ; --- BEGIN COPYRIGHT BLOCK ---
@@ -85,22 +92,28 @@ def instance_example(inst, log, args):
     s2b = Slapd2Base(log)
     print(g2b.collect_help())
     print(s2b.collect_help())
+    return True
+
 
 def instance_remove(inst, log, args):
     if not args.ack:
         log.info("""Not removing: if you are sure, add --doit""")
-        sys.exit(0)
+        return True
     else:
         log.info("""
 About to remove instance %s!
 If this is not what you want, press ctrl-c now ...
         """ % inst.serverid)
-    for i in range(1,6):
+    for i in range(1, 6):
         log.info('%s ...' % (5 - int(i)))
         time.sleep(1)
     log.info('Removing instance ...')
-    remove_ds_instance(inst)
-    log.info('Completed instance removal')
+    try:
+        remove_ds_instance(inst)
+        log.info('Completed instance removal')
+    except:
+        log.fatal('Instance removal failed')
+        return False
 
 
 def create_parser(subcommands):
@@ -121,7 +134,8 @@ def create_parser(subcommands):
     status_parser.set_defaults(func=instance_status)
 
     remove_parser = subcommands.add_parser('remove', help="Destroy an instance of Directory Server, and remove all data.")
-    remove_parser.add_argument('--doit', dest="ack", help="By default we do a dry run. This actually initiates the removal.", action='store_true', default=False)
+    remove_parser.add_argument('--doit', dest="ack", help="By default we do a dry run. This actually initiates the removal.",
+                               action='store_true', default=False)
     remove_parser.set_defaults(func=instance_remove)
 
 
diff --git a/src/lib389/lib389/cli_idm/__init__.py b/src/lib389/lib389/cli_idm/__init__.py
index d7ad00b..cb4fd11 100644
--- a/src/lib389/lib389/cli_idm/__init__.py
+++ b/src/lib389/lib389/cli_idm/__init__.py
@@ -89,7 +89,7 @@ def _generic_get(inst, basedn, log, manager_class, selector, args=None):
     mc = manager_class(inst, basedn)
     if args and args.json:
         o = mc.get(selector, json=True)
-        print(o);
+        print(o)
     else:
         o = mc.get(selector)
         o_str = o.display()
diff --git a/src/lib389/lib389/instance/options.py b/src/lib389/lib389/instance/options.py
index 1dee6f5..a3ea78d 100644
--- a/src/lib389/lib389/instance/options.py
+++ b/src/lib389/lib389/instance/options.py
@@ -42,18 +42,20 @@ format_keys = [
 
 ds_paths = Paths()
 
+
 class Options2(object):
     # This stores the base options in a self._options dict.
     # It provides a number of options for:
     # - dict overlay
     # - parsing the config parser types.
+
     def __init__(self, log):
         # 'key' : (default, helptext, valid_func )
-        self._options = {} # this takes the default
-        self._type = {} # Lists the type the item should be.
-        self._helptext = {} # help text for the option, MANDATORY.
-        self._example_comment = {} # If this is a commented value in the example.
-        self._valid_func = {} # options verification function.
+        self._options = {}  # this takes the default
+        self._type = {}  # Lists the type the item should be.
+        self._helptext = {}  # help text for the option, MANDATORY.
+        self._example_comment = {}  # If this is a commented value in the example.
+        self._valid_func = {}  # options verification function.
         self._section = None
         self.log = log
 
@@ -102,6 +104,7 @@ class Options2(object):
 # Base, example dicts of the general, backend (userRoot) options.
 #
 
+
 class General2Base(Options2):
     def __init__(self, log):
         super(General2Base, self).__init__(log)
@@ -139,9 +142,10 @@ class General2Base(Options2):
 
 #
 # This module contains the base options and configs for Director Server
-# setup and install. This allows 
+# setup and install. This allows
 #
 
+
 class Slapd2Base(Options2):
     def __init__(self, log):
         super(Slapd2Base, self).__init__(log)
diff --git a/src/lib389/lib389/instance/remove.py b/src/lib389/lib389/instance/remove.py
index 25d56a3..9a376a6 100644
--- a/src/lib389/lib389/instance/remove.py
+++ b/src/lib389/lib389/instance/remove.py
@@ -10,6 +10,7 @@ import os
 import shutil
 import subprocess
 
+
 def remove_ds_instance(dirsrv):
     """
     This will delete the instance as it is define. This must be a local instance.