Comments inline

2015-08-17 10:31 GMT+02:00 Ondrej Lichtner <olichtne@redhat.com>:
I found 2 more problems, see inline.

On Tue, Aug 11, 2015 at 12:07:43PM +0200, Jan Tluka wrote:
> Just one issue found, see inline, but otherwise looks fine.
> Much more readable and predictable ;-) Nice work!
>
> Mon, Aug 10, 2015 at 05:39:32PM CEST, jprochaz@redhat.com wrote:
> >Removed MessageDispatcherLite.py
> >
> >lnst-pool-wizard
> >+ Make pep8 compliant
> >+ Print error messages on stderr
> >+ Changes correspoding with Wizard.py refactorization
> >+ Remove blank line in help
> >
> >Wizard.py
> >+ Make pep8 compliant
> >+ Most of the methods reworked to be more clean and readable
> >+ Setup for implementation of support for virtual machines
> >
> >Closes #129
> >
> >Signed-off-by: Jiri Prochazka <jprochaz@redhat.com>
> >---
> > lnst-pool-wizard                         |  55 ++--
> > lnst/Controller/MessageDispatcherLite.py |  66 -----
> > lnst/Controller/Wizard.py                | 422 +++++++++++++++++++++----------
> > 3 files changed, 332 insertions(+), 211 deletions(-)
> > delete mode 100644 lnst/Controller/MessageDispatcherLite.py
> >
> >diff --git a/lnst-pool-wizard b/lnst-pool-wizard
> >index c788688..d7ad506 100755
> >--- a/lnst-pool-wizard
> >+++ b/lnst-pool-wizard
> >@@ -17,48 +17,67 @@ import getopt
> > RETVAL_PASS = 0
> > RETVAL_ERR = 1
> >
> >-def help(retval = 0):
> >+
> >+def help(retval=0):
> >     print "Usage:\n"\
> >           " lnst-pool-wizard [mode] [hostname[:port]]\n"\
> >           "\n"\
> >           "Modes:\n"\
> >-          " -h, --help                display this help text and exit\n"\
> >-          " -i, --interactive         start wizard in interactive mode (this "\
> >-                                      "is default mode)\n"\
> >-          " -n, --noninteractive      start wizard in noninteractive mode\n"\
> >+          " -h, --help                 display this help text and exit\n"\
> >+          " -p, --pool_dir <directory> set the pool dir (works both in) "\
> >+          "interactive and noninteractive mode\n"\
> >+          " -i, --interactive          start wizard in interactive mode (this "\
> >+          "is default mode)\n"\
> >+          " -n, --noninteractive       start wizard in noninteractive mode\n"\
> >           "Examples:\n"\
> >           " lnst-pool-wizard --interactive\n"\
> >           " lnst-pool-wizard --noninteractive 192.168.122.2\n"\
> >-          " lnst-pool-wizard -n 192.168.122.2:8888 192.168.122.3:9999 192.168.122.4\n"
> >+          " lnst-pool-wizard -n 192.168.122.2:8888 192.168.122.4\n"\
> >+          " lnst-pool-wizard -p \".pool/\" -n 192.168.1.1:8877 192.168.122.4"
> >     sys.exit(retval)
> >
> >
> > def main():
> >     try:
> >         opts, args = getopt.getopt(
> >-        sys.argv[1:],
> >-        "hin",
> >-        ["help", "interactive", "noninteractive"]
> >+            sys.argv[1:],
> >+            "hinp:",
> >+            ["help", "interactive", "noninteractive", "pool_dir="]
> >         )
> >     except getopt.GetoptError as err:
> >-        print str(err)
> >+        sys.stderr.write(str(err))
> >         help(RETVAL_ERR)
> >-    wizard = Wizard()
> >-    for opt, arg in opts:
> >+
> >+    pool_dir= None
> >+    mode = "interactive"
> >+
> >+    for opt, arg  in opts:
> >         if opt in ("-h", "--help"):
> >             help()
> >         elif opt in ("-i", "--interactive"):
> >-            wizard.interactive()
> >-            sys.exit(RETVAL_PASS)
> >+            mode = "interactive"
> >         elif opt in ("-n", "--noninteractive"):
> >             if not args:
> >-                print "No hostname entered!"
> >+                sys.stderr.write("No hostnames entered\n")
> >                 return RETVAL_ERR
> >-            wizard.noninteractive(args)
> >-            sys.exit(RETVAL_PASS)
> >+            else:
> >+                hostlist = args
> >+            mode = "noninteractive"
> >+        elif opt in ("-p", "--pool_dir"):
> >+            if not arg:
> >+                sys.stderr.write("No pool directory specified\n")
> >+            else:
> >+                pool_dir = arg
> >         else:
> >             help(RETVAL_ERR)
> >-    wizard.interactive()
> >+
> >+    w = Wizard()
> >+
> >+    if mode == "noninteractive":
> >+        w.noninteractive(hostlist, pool_dir)
> >+    else:
> >+        w.interactive(pool_dir)
> >+
> >     sys.exit(RETVAL_PASS)
> >
> >
> >diff --git a/lnst/Controller/MessageDispatcherLite.py b/lnst/Controller/MessageDispatcherLite.py
> >deleted file mode 100644
> >index 2973763..0000000
> >--- a/lnst/Controller/MessageDispatcherLite.py
> >+++ /dev/null
> >@@ -1,66 +0,0 @@
> >-"""
> >-This module defines MessageDispatcherLite class which is derived from
> >-lnst.NetTestController.MessageDispatcher.
> >-
> >-Copyright 2011 Red Hat, Inc.
> >-Licensed under the GNU General Public License, version 2 as
> >-published by the Free Software Foundation; see COPYING for details.
> >-"""
> >-
> >-__author__ = """
> >-jpirko@redhat.com (Jiri Pirko)
> >-"""
> >-
> >-from lnst.Common.ConnectionHandler import send_data
> >-from lnst.Common.ConnectionHandler import ConnectionHandler
> >-from lnst.Controller.NetTestController import NetTestError
> >-
> >-class MessageDispatcherLite(ConnectionHandler):
> >-    def __init__(self):
> >-        super(MessageDispatcherLite, self).__init__()
> >-
> >-    def add_slave(self, machine, connection):
> >-        self.add_connection(1, connection)
> >-
> >-    def send_message(self, machine_id, data):
> >-        soc = self.get_connection(1)
> >-
> >-        if send_data(soc, data) == False:
> >-            msg = "Connection error from slave %s" % str(1)
> >-            raise NetTestError(msg)
> >-
> >-    def wait_for_result(self, machine_id):
> >-        wait = True
> >-        while wait:
> >-            connected_slaves = self._connection_mapping.keys()
> >-
> >-            messages = self.check_connections()
> >-
> >-            remaining_slaves = self._connection_mapping.keys()
> >-
> >-            for msg in messages:
> >-                if msg[1]["type"] == "result" and msg[0] == 1:
> >-                    wait = False
> >-                    result = msg[1]["result"]
> >-                else:
> >-                    self._process_message(msg)
> >-
> >-            if connected_slaves != remaining_slaves:
> >-                disconnected_slaves = set(connected_slaves) -\
> >-                                      set(remaining_slaves)
> >-                msg = "Slaves " + str(list(disconnected_slaves)) + \
> >-                      " disconnected from the controller."
> >-                raise NetTestError(msg)
> >-
> >-        return result
> >-
> >-    def _process_message(self, message):
> >-        if message[1]["type"] == "log":
> >-            pass
> >-        else:
> >-            msg = "Unknown message type: %s" % message[1]["type"]
> >-            raise NetTestError(msg)
> >-
> >-    def disconnect_slave(self, machine_id):
> >-        soc = self.get_connection(machine_id)
> >-        self.remove_connection(soc)
> >diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py
> >index c38aae5..e4ddc54 100644
> >--- a/lnst/Controller/Wizard.py
> >+++ b/lnst/Controller/Wizard.py
> >@@ -1,7 +1,7 @@
> > """
> >-Wizard class for creating machine pool .xml files
> >+Wizard class for creating slave machine config files
> >
> >-Copyright 2014 Red Hat Inc.
> >+Copyright 2015 Red Hat Inc.
> > Licensed under the GNU General Public Licence, version 2 as
> > published by the Free Software Foundation; see COPYING for details.
> > """
> >@@ -9,150 +9,196 @@ published by the Free Software Foundation; see COPYING for details.
> > __author__ = """
> > jprochaz@redhat.com (Jiri Prochazka)
> > """
> >+
> > import socket
> > import sys
> >+import time
> > import os
> >-from lnst.Controller.Machine import Machine
> >-from lnst.Controller.MessageDispatcherLite import MessageDispatcherLite
> >-from lnst.Common.NetUtils import test_tcp_connection
> > from lnst.Common.Utils import mkdir_p
> > from lnst.Common.Config import DefaultRPCPort
> >+from lnst.Common.ConnectionHandler import send_data, recv_data
> > from xml.dom.minidom import getDOMImplementation
> >
> >+DefaultPoolDir = os.path.expanduser("~/.lnst/pool/")
> >+PATH_IS_DIR_ACCESSIBLE = 0
> >+PATH_IS_DIR_NOT_ACCESSIBLE = 1
> >+PATH_DOES_NOT_EXIST = 2
> >+PATH_NOT_DIR = 3
> >+
> >+
> > class Wizard:
> >-    def __init__(self):
> >-        self._msg_dispatcher = MessageDispatcherLite()
> >-        self._pool_dir = os.path.expanduser("~/.lnst/pool")
> >
> >-    def interactive(self):
> >+    def interactive(self, pool_dir=None):
> >+        """ Starts Wizard in an interactive mode
> >+        @param pool_dir Path to pool directory (optional)
> >         """
> >-        Starts Wizard in interactive mode. Wizard requests hostname and port
> >-        from user, tests connectivity to entered host, then he tries to connect
> >-        and configure slave on host machine, and finally he requests list of
> >-        ethernet interfaces in state DOWN. Then he writes them into .xml file
> >-        named by user. User can choose which interfaces should be added to the
> >-        .xml file.
> >-        """
> >-
> >-        pool_dir = raw_input("Enter path to pool directory "\
> >-                             "(default: ~/.lnst/pool): ")
> >-        if pool_dir != "":
> >-            self._pool_dir = os.path.expanduser(pool_dir)
> >-            print "Pool directory set to %s" % self._pool_dir
> >+        if pool_dir is None:
> >+            pool_dir = self._query_pool_dir()
> >+        else:
> >+            rv = self._check_path(pool_dir)
> >+            if rv == PATH_IS_DIR_ACCESSIBLE:
> >+                print("Pool directory set to '%s'" % pool_dir)
> >+            elif rv == PATH_DOES_NOT_EXIST:
> >+                sys.stderr.write("Path '%s' does not exist\n" % pool_dir)
> >+                pool_dir = self._query_pool_dir()
> >+            elif rv == PATH_NOT_DIR:
> >+                sys.stderr.write("Path '%s' exists but is not a directory\n"
> >+                                 % pool_dir)
> >+                pool_dir = self._query_pool_dir()
> >+            elif rv == PATH_IS_DIR_NOT_ACCESSIBLE:
> >+                sys.stderr.write("Directory '%s' is not writable\n" % pool_dir)
> >+                pool_dir = self._query_pool_dir()
> >
> >         while True:
> >-            while True:
> >-                hostname = raw_input("Enter hostname: ")
> >-                try:
> >-                # Tests if hostname is translatable into IP address
> >-                    socket.gethostbyname(hostname)
> >-                    break
> >-                except:
> >-                    sys.stderr.write("Hostname is not translatable into valid"\
> >-                                     " IP address.\n")
> >+            hostname = self._query_hostname()
> >+            port = self._query_port()
> >+
> >+            sock = self._get_connection(hostname, port)
> >+            if sock is None:
> >+                if self._query_continuation():
> >                     continue
> >-            while True:
> >-                port = raw_input("Enter port(default: 9999): ")
> >-                if port == "":
> >-                    port = DefaultRPCPort
> >-                try:
> >-                    port = int(port)
> >+                else:
> >                     break
> >-                except:
> >-                    sys.stderr.write("Invalid port.")
> >-                    continue
> >-            msg = self._get_suitable_interfaces(socket.gethostbyname(hostname),\
> >-                                             port)
> >-            if msg:
> >-                self._write_to_xml(msg, hostname, port, "interactive")
> >-            next_machine = raw_input("Do you want to add another machine? "\
> >-                                     "[Y/n]: ")
> >-            if next_machine.lower() == 'y' or next_machine == "":
> >+
> >+            machine_interfaces = self._get_machine_interfaces(sock)
> >+            sock.close()
> >+
> >+            if machine_interfaces == {}:
> >+                sys.stderr.write("No suitable interfaces found on the host "
> >+                                 "'%s:%s'\n" % (hostname, port))
> >+            elif machine_interfaces is not None:
> >+                filename = self._query_filename(hostname)
> >+                self._create_xml(machine_interfaces, hostname,
> >+                                 port, pool_dir, filename, "interactive")
> >+            if self._query_continuation():
> >                 continue
> >             else:
> >                 break
> >-            return
> >
> >-    def noninteractive(self, hostlist):
> >-        """
> >-        Starts Wizard in noninteractive mode. Wizard gets list of hosts and
> >-        ports as arguments. He tries to connect and get info about suitable
> >-        interfaces for each of the hosts. Noninteractive mode does not prompt
> >-        user about anything, it automatically adds all suitable interfaces into
> >-        .xml file named the same as the hostname of the selected machine.
> >+    def noninteractive(self, hostlist, pool_dir):
> >+        """ Starts Wizard in noninteractive mode
> >+        @param hostlist List of hosts (mandatory)
> >+        @param pool_dir Path to pool_directory (optional)
> >         """
> >-        self._mode = "noninteractive"
> >+        if pool_dir is None:
> >+            pool_dir = DefaultPoolDir
> >+
> >+        rv = self._check_path(pool_dir)
> >+        if rv == PATH_IS_DIR_ACCESSIBLE:
> >+            print("Pool directory set to '%s'" % pool_dir)
> >+        elif rv == PATH_DOES_NOT_EXIST:
> >+            sys.stderr.write("Path '%s' does not exist\n" % pool_dir)
> >+            pool_dir = self._create_dir(pool_dir)
> >+            if pool_dir is None:
> >+                sys.stderr.write("Pool wizard aborted\n")
> >+                return
> >+        elif rv == PATH_NOT_DIR:
> >+            sys.stderr.write("Path '%s' exists but is not a directory\n"
> >+                             % pool_dir)
> >+            sys.stderr.write("Pool wizard aborted\n")
> >+            return
> >+        elif rv == PATH_IS_DIR_NOT_ACCESSIBLE:
> >+            sys.stderr.write("Directory '%s' is not writable\n" % pool_dir)
> >+            sys.stderr.write("Pool wizard aborted\n")
> >+            return
> >+
> >
> >         for host in hostlist:
> >-            # Checks if port was entered along with hostname
> >+            print("Processing host '%s'" % host)
> >+            # Check if port was entered along with hostname
> >             if host.find(":") != -1:
> >-               hostname = host.split(':')[0]
> >-               try:
> >-                   port = int(host.split(':')[1])
> >-               except:
> >-                   port = DefaultRPCPort
> >+                hostname = host.split(":")[0]
> >+                if hostname == "":
> >+                    msg = "'%s' does not contain valid hostname\n" % host
> >+                    sys.stderr.write(msg)
> >+                    sys.stderr.write("Skipping host '%s'\n" % host)
> >+                    continue
> >+                try:
> >+                    port = int(host.split(":")[1])
> >+                except:
> >+                    port = DefaultRPCPort
> >+                    msg = "Invalid port entered, "\
> >+                          "using '%s' instead\n" % port
> >+                    sys.stderr.write(msg)
> >             else:
> >                 hostname = host
> >                 port = DefaultRPCPort
> >-            msg = self._get_suitable_interfaces(hostname, port)
> >-            if not msg:
> >+
> >+            if not self._check_hostname(hostname):
> >+                sys.stderr.write("Hostname '%s' is not translatable into a "
> >+                                 "valid IP address\n" % hostname)
> >+                sys.stderr.write("Skipping host '%s'\n" % host)
> >                 continue
> >-            self._write_to_xml(msg, hostname, port, "noninteractive")
> >
> >-    def _get_suitable_interfaces(self, hostname, port):
> >-        """
> >-        Calls all functions, which are used by both interactive and
> >-        noninteractive mode to get list of suitable interfaces. The list is
> >-        saved to variable msg.
> >-        """
> >-        if not test_tcp_connection(hostname, port):
> >-            sys.stderr.write("Host destination '%s:%s' unreachable.\n"
> >-                              % (hostname, port))
> >-            return False
> >-        if not self._connect_and_configure_machine(hostname, port):
> >-            return False
> >-        msg =  self._get_device_ifcs(hostname, port)
> >-        self._msg_dispatcher.disconnect_slave(1)
> >-        return msg
> >+            sock = self._get_connection(hostname, port)
> >+            if sock is None:
> >+                sys.stderr.write("Skipping host '%s'\n" % host)
> >+                continue
> >
> >-    def _get_device_ifcs(self, hostname, port):
> >-        """
> >-        Sends RPC call request to Slave to call function get_devices, which
> >-        returns list of ethernet devices which are in state DOWN.
> >-        """
> >-        msg = self._machine._rpc_call("get_devices")
> >-        if msg == {}:
> >-            sys.stderr.write("No suitable interfaces found on the slave "\
> >-                             "'%s:%s'.\n" % (hostname, port))
> >-            return False
> >-        return msg
> >+            machine_interfaces = self._get_machine_interfaces(sock)
> >+            sock.close()
> >
> >-    def _connect_and_configure_machine(self, hostname, port):
> >-        """
> >-        Connects to Slave and configures it
> >+            if machine_interfaces is {}:
> >+                sys.stderr.write("No suitable interfaces found on the host "
> >+                                 "'%s:%s'\n" % (hostname, port))
> >+                continue
> >+            elif machine_interfaces is None:
> >+                continue
> >+            else:
> >+                filename = hostname + ".xml"
> >+                self._create_xml(machine_interfaces, hostname,
> >+                                 port, pool_dir, filename, "noninteractive")
> >+
> >+    def _check_hostname(self, hostname):
> >+        """ Checks hostnames translatibility
> >+        @param hostname Hostname which is checked whether it's valid
> >+        @return True if valid hostname was entered, False otherwise
> >         """
> >         try:
> >-            self._machine = Machine(1, hostname, None, port)
> >-            self._machine.set_rpc(self._msg_dispatcher)
> >-            self._machine.configure("MachinePoolWizard")
> >+            # Test if hostname is translatable into IP address
> >+            socket.gethostbyname(hostname)
> >             return True
> >         except:
> >-            sys.stderr.write("Remote machine '%s:%s' configuration failed!\n"
> >-                              % (hostname, port))
> >-            self._msg_dispatcher.disconnect_slave(1)
> >             return False
> >
> >-    def _write_to_xml(self, msg, hostname, port, mode):
> >+    def _check_path(self, pool_dir):
> >+        """ Checks if path exists, is dir and is accessible by user
> >+        @param pool_dir Path to checked directory
> >+        @return True if user can write in entered directory, False otherwise
> >         """
> >-        Used for writing desired output into .xml file. In interactive mode,
> >-        user is prompted for every interface, in noninteractive mode all
> >-        interfaces are automatically added to the .xml file.
> >+        if not os.path.exists(pool_dir):
> >+            return PATH_DOES_NOT_EXIST
> >+        if os.path.isdir(pool_dir):
> >+            if os.access(pool_dir, os.W_OK):
> >+                return PATH_IS_DIR_ACCESSIBLE
> >+            else:
> >+                return PATH_IS_DIR_NOT_ACCESSIBLE
> >+        else:
> >+            return PATH_NOT_DIR
> >+
> >+    def _create_dir(self, pool_dir):
> >+        """ Creates specified directory
> >+        @param pool_dir Directory to be created
> >+        @return Path to dir which was created, None if no directory was created
> >+        """
> >+        try:
> >+            mkdir_p(pool_dir)
> >+            print("Dir '%s' has been created" % pool_dir)
> >+            return pool_dir
> >+        except:
> >+            sys.stderr.write("Failed creating dir")
> >+            return None
> >+
> >+    def _create_xml(self, machine_interfaces, hostname,
> >+                    port, pool_dir, filename, mode):
> >+        """ Creates slave machine XML file
> >+        @param machine_interfaces Dictionary with machine's interfaces
> >+        @param hostname Hostname of the machine
> >+        @param port Port on which LNST listens on the machine
> >+        @param pool_dir Path to directory where XML file will be created
> >+        @param filename Name of the XML file
> >+        @param mode Mode in which wizard was started
> >         """
> >-        if mode == "interactive":
> >-            output_file = raw_input("Enter the name of the output .xml file "\
> >-                                    "(without .xml, default is hostname.xml): ")
> >-        if mode == "noninteractive" or output_file == "":
> >-            output_file = hostname
> >
> >         impl = getDOMImplementation()
> >         doc = impl.createDocument(None, "slavemachine", None)
> >@@ -166,42 +212,164 @@ class Wizard:
> >         interfaces_el = doc.createElement("interfaces")
> >         top_el.appendChild(interfaces_el)
> >
> >-        devices_added = 0
> >-        for interface in msg.itervalues():
> >-            if mode == 'interactive':
> >+        interfaces_added = 0
> >+        for iface in machine_interfaces.itervalues():
> >+            if mode == "interactive":
> >                 answer = raw_input("Do you want to add interface '%s' (%s) "
> >-                                   "to the recipe? [Y/n]" % (interface['name'],
> >-                                                          interface['hwaddr']))
> >-            if mode == "noninteractive" or answer.lower() == 'y'\
> >+                                   "to the recipe? [Y/n]: " % (iface["name"],
> >+                                                               iface["hwaddr"]))
> >+            if mode == "noninteractive" or answer.lower() == "y"\
> >                or answer == "":
> >-                devices_added += 1
> >+                interfaces_added += 1
> >                 eth_el = doc.createElement("eth")
> >-                eth_el.setAttribute("id", interface['name'])
> >+                eth_el.setAttribute("id", iface["name"])
> >                 eth_el.setAttribute("label", "default_network")
> >                 interfaces_el.appendChild(eth_el)
> >                 params_el = doc.createElement("params")
> >                 eth_el.appendChild(params_el)
> >                 param_el = doc.createElement("param")
> >                 param_el.setAttribute("name", "hwaddr")
> >-                param_el.setAttribute("value", interface['hwaddr'])
> >+                param_el.setAttribute("value", iface["hwaddr"])
> >                 params_el.appendChild(param_el)
> >-        if devices_added == 0:
> >-            sys.stderr.write("You didn't add any interface, no file '%s.xml' "\
> >-                  "will be created!\n" % output_file)
> >+        if interfaces_added == 0:
> >+            sys.stderr.write("You didn't add any interface, no file "
> >+                             "'%s' will be created\n" % filename)
> >             return
> >
> >-        mkdir_p(self._pool_dir)
> >
> >         try:
> >-            f = open(self._pool_dir + "/" + output_file + ".xml", 'w')
> >+            f = open(pool_dir + "/" + filename, "w")
> >             f.write(doc.toprettyxml())
> >             f.close()
> >         except:
> >-            sys.stderr.write("File '%s.xml' could not be opened "\
> >-                             "or data written." % output_file+"\n")
> >-            raise WizardException()
> >+            msg = "File '%s/%s' could not be opened or data written\n"\
> >+                  % (pool_dir, filename)
> >+            raise WizardException(msg)
>
> You raise an exception but this is not handled here nor in
> lnst-pool-wizard. Either remove it completely or add try-except block
> somewhere in the code.
>
> >+
> >+        print("File '%s' successfuly created" % filename)
> >+
> >+    def _get_connection(self, hostname, port):
> >+        """ Connects to machine
> >+        @param hostname Hostname of the machine
> >+        @param port Port of the machine
> >+        @return Connected socket if connection was successful, None otherwise
> >+        """
> >+        try:
> >+            sock = socket.create_connection((hostname, port))
> >+            return sock
> >+        except socket.error:
> >+            sys.stderr.write("Connection to remote host '%s:%s' failed\n"
> >+                             % (hostname, port))
> >+            return None
> >+
> >+    def _get_machine_interfaces(self, sock):
> >+        """ Gets machine interfaces via RPC call
> >+        @param sock Socket used for connecting to machine
> >+        @return Dictionary with machine interfaces or None if RPC call fails
> >+        """
> >+        msg = {"type": "command", "method_name": "get_devices", "args": {}}
> >+        if not send_data(sock, msg):
> >+            sys.stderr.write("Could not send request to slave machine\n")
> >+            return None
> >+
> >+        while True:
> >+            data = recv_data(sock)
> >+            if data["type"] == "result":
> >+                return data["result"]
> >+
> >+    def _query_continuation(self):
> >+        """ Queries user for adding next machine
> >+        @return True if user wants to add another machine, False otherwise
> >+        """
> >+        answer = raw_input("Do you want to add another machine? [Y/n]: ")
> >+        if answer.lower() == "y" or answer == "":
> >+            return True
> >+        else:
> >+            return False
> >+
> >+    def _query_dir_creation(self, pool_dir):
> >+        """ Queries user for creating specified directory
> >+        @return True if user wants to create the directory, False otherwise
> >+        """
> >+        answer = raw_input("Create dir '%s'? [Y/n]: " % pool_dir)
> >+        if answer.lower() == 'y' or answer == "":
> >+            return True
> >+        else:
> >+            return False
> >+
> >+    def _query_filename(self, hostname):
> >+        """ Queries user for name of the file
> >+        @hostname Hostname of the machine which is used as default filename
> >+        @return Name of the file with .xml extension
> >+        """
> >+        output_file = raw_input("Enter the name of the output .xml file "
> >+                                "(without .xml, default is '%s.xml'): "
> >+                                % hostname)
> >+        if output_file == "":
> >+            return hostname + ".xml"
> >+        else:
> >+            return output_file + ".xml"
> >+
> >+    def _query_hostname(self):
> >+        """ Queries user for hostname
> >+        @return Valid (is translatable to an IP address) hostname
> >+        """
> >+        while True:
> >+            hostname = raw_input("Enter hostname: ")
> >+            if hostname == "":
> >+                sys.stderr.write("No hostname entered\n")
> >+                continue
> >+            if self._check_hostname(hostname):
> >+                return hostname
> >+            else:
> >+                sys.stderr.write("Hostname '%s' is not translatable into a "
> >+                                 "valid IP address\n" % hostname)
> >+

Trailing whitespace here...
​Will be fixed in next patch​
 

> >+    def _query_pool_dir(self):
> >+        """ Queries user for pool directory
> >+        @return Valid (is writable by user) path to directory
> >+        """
> >+        while True:
> >+            pool_dir = raw_input("Enter path to a pool directory "
> >+                                 "(default: '%s'): " % DefaultPoolDir)
> >+            if pool_dir == "":
> >+                pool_dir = DefaultPoolDir
> >+
> >+            pool_dir = os.path.expanduser(pool_dir)
> >+            rv = self._check_path(pool_dir)
> >+            if rv == PATH_IS_DIR_ACCESSIBLE:
> >+                print("Pool directory set to '%s'" % pool_dir)
> >+                return pool_dir
> >+            elif rv == PATH_DOES_NOT_EXIST:
> >+                sys.stderr.write("Path '%s' does not exist\n"
> >+                                 % pool_dir)
> >+                if self._query_dir_creation(pool_dir):
> >+                    created_dir = self._create_dir(pool_dir)
> >+                    if created_dir is not None:
> >+                        return created_dir
> >+
> >+            elif rv == PATH_NOT_DIR:
> >+                sys.stderr.write("Path '%s' exists but is not a directory\n"
> >+                                 % pool_dir)
> >+            elif rv == PATH_IS_DIR_NOT_ACCESSIBLE:
> >+                sys.stderr.write("Directory '%s' is not writable\n"
> >+                                 % pool_dir)
> >+
> >+    def _query_port(self):
> >+        """ Queries user for port
> >+        @return Integer representing port
> >+        """
> >+        while True:
> >+            port = raw_input("Enter port (default: %d): " % DefaultRPCPort)
> >+            if port == "":
> >+                return DefaultRPCPort
> >+            else:
> >+                try:
> >+                    port = int(port)
> >+                    return port
> >+                except:
> >+                    sys.stderr.write("Invalid port entered\n")
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
What happens in this case? The _query_port method will return None to
the calling function which is "interactive()" where the result is not
checked and passed directly to _get_connection where you'll try to
connect to a None port.

​In case invalid port is entered, the except part prints error message and then continues in while loop, meaning that it queries user for port again, until he enters a valid one. If I'm not mistaken, the method does not return None in any scenario.​

> >
> >-        print "File '%s.xml' successfuly created." % output_file
> >
> > class WizardException(Exception):
> >     pass
> >--
> >2.4.3
> >
> >_______________________________________________
> >LNST-developers mailing list
> >LNST-developers@lists.fedorahosted.org
> >https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
> _______________________________________________
> LNST-developers mailing list
> LNST-developers@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/lnst-developers