[PATCH] Refactorization of LNST Pool Wizard

[Jiri Prochazka]

MessageDispatcherLite.py
+ Remove unused import
+ Make pep8 compliant
+ Rewrite one error message

lnst-pool-wizard
+ Make pep8 compliant
+ Print error messages on stderr
+ Changes correspoding with Wizard.py refactorization

Wizard.py
+ Make pep8 compliant
+ Convert public methods to private
+ 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(a)redhat.com&gt;
---
 lnst-pool-wizard                         |  55 +++--
 lnst/Controller/MessageDispatcherLite.py |   9 +-
 lnst/Controller/Wizard.py                | 349 ++++++++++++++++++++-----------
 3 files changed, 264 insertions(+), 149 deletions(-)

diff --git a/lnst-pool-wizard b/lnst-pool-wizard
index c788688..d17d23a 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\n"
     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
index 2973763..623c232 100644
--- a/lnst/Controller/MessageDispatcherLite.py
+++ b/lnst/Controller/MessageDispatcherLite.py
@@ -15,7 +15,9 @@ 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__()
 
@@ -25,9 +27,8 @@ class MessageDispatcherLite(ConnectionHandler):
     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)
+        if send_data(soc, data) is False:
+            raise NetTestError("Connection error from slave")
 
     def wait_for_result(self, machine_id):
         wait = True
@@ -47,7 +48,7 @@ class MessageDispatcherLite(ConnectionHandler):
 
             if connected_slaves != remaining_slaves:
                 disconnected_slaves = set(connected_slaves) -\
-                                      set(remaining_slaves)
+                    set(remaining_slaves)
                 msg = "Slaves " + str(list(disconnected_slaves)) + \
                       " disconnected from the controller."
                 raise NetTestError(msg)
diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py
index c38aae5..7db8459 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,149 @@ published by the Free Software Foundation; see COPYING for details.
 __author__ = """
 jprochaz(a)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/")
+
+
 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()
+        # Invalid pool dir entered, query for correct one
+        elif not self._check_pool_dir(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
+        # Invalid pool_dir entered, abort wizard
+        if not self._check_pool_dir(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("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)
+            sys.stderr.write("Hostname '%s' is not translatable into a valid
"
+                             "IP address\n" % hostname)
             return False
 
-    def _write_to_xml(self, msg, hostname, port, mode):
+    def _check_pool_dir(self, pool_dir):
+        """ Checks users access to selected directory
+        @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 os.access(pool_dir, os.W_OK):
+            return True
+        else:
+            sys.stderr.write("Directory '%s' is not writable\n"
+                             % pool_dir)
+            return False
+
+    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 +165,138 @@ 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)
+        mkdir_p(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)
+
+        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_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
+
+    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 == "":
+                return DefaultPoolDir
+
+            pool_dir = os.path.expanduser(pool_dir)
+            # check if dir is writable
+            if self._check_pool_dir(pool_dir):
+                print("Pool directory set to '%s'" % pool_dir)
+                return 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")
 
-        print "File '%s.xml' successfuly created." % output_file
 
 class WizardException(Exception):
     pass
-- 
2.4.3

[Jan Tluka]

See comments inline. The patch looks good however there are still some minor
issues around.

Tue, Jul 21, 2015 at 11:13:11AM CEST, jprochaz(a)redhat.com wrote:
> MessageDispatcherLite.py
> + Remove unused import
> + Make pep8 compliant
> + Rewrite one error message 
Why we need it? Since Wizard class now communicates with lnst-slave
directly we don't need it. Nack, unless there's valid reason to keep the
DispatcherLite.

> lnst-pool-wizard
> + Make pep8 compliant
> + Print error messages on stderr
> + Changes correspoding with Wizard.py refactorization
>
> Wizard.py
> + Make pep8 compliant
> + Convert public methods to private
> + 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(a)redhat.com&gt;
> ---
>  lnst-pool-wizard                         |  55 +++--
>  lnst/Controller/MessageDispatcherLite.py |   9 +-
>  lnst/Controller/Wizard.py                | 349 ++++++++++++++++++++-----------
>  3 files changed, 264 insertions(+), 149 deletions(-)
>
> diff --git a/lnst-pool-wizard b/lnst-pool-wizard
> index c788688..d17d23a 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\n"
>      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
> index 2973763..623c232 100644
> --- a/lnst/Controller/MessageDispatcherLite.py
> +++ b/lnst/Controller/MessageDispatcherLite.py
> @@ -15,7 +15,9 @@ 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__()
>  
> @@ -25,9 +27,8 @@ class MessageDispatcherLite(ConnectionHandler):
>      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)
> +        if send_data(soc, data) is False:
> +            raise NetTestError("Connection error from slave")
>  
>      def wait_for_result(self, machine_id):
>          wait = True
> @@ -47,7 +48,7 @@ class MessageDispatcherLite(ConnectionHandler):
>  
>              if connected_slaves != remaining_slaves:
>                  disconnected_slaves = set(connected_slaves) -\
> -                                      set(remaining_slaves)
> +                    set(remaining_slaves)
>                  msg = "Slaves " + str(list(disconnected_slaves)) + \
>                        " disconnected from the controller."
>                  raise NetTestError(msg)
> diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py
> index c38aae5..7db8459 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,149 @@ published by the Free Software Foundation; see COPYING for details.
>  __author__ = """
>  jprochaz(a)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/")
> +
> +
>  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()
> +        # Invalid pool dir entered, query for correct one
> +        elif not self._check_pool_dir(pool_dir):
> +            pool_dir = self._query_pool_dir() 
IMO, this is not the best approach. E.g. user won't specify a pool dir,
so the user is prompted to enter directory. But there's no check of it.

Also if user specifies the pool dir, it's checked and if it fails then
the user is prompted back but there's no check after that.

Please fix this.

>  
>          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
> +        # Invalid pool_dir entered, abort wizard
> +        if not self._check_pool_dir(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("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)
> +            sys.stderr.write("Hostname '%s' is not translatable into a valid
> "
> +                             "IP address\n" % hostname)
>              return False
>  
> -    def _write_to_xml(self, msg, hostname, port, mode):
> +    def _check_pool_dir(self, pool_dir):
> +        """ Checks users access to selected directory
> +        @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 os.access(pool_dir, os.W_OK):
> +            return True
> +        else:
> +            sys.stderr.write("Directory '%s' is not writable\n"
> +                             % pool_dir)
> +            return False
> +
> +    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 +165,138 @@ 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) 
How about adding driver information as well?

> -        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)
> +        mkdir_p(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)
> +
> +        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_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
> +
> +    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 == "":
> +                return DefaultPoolDir
> +
> +            pool_dir = os.path.expanduser(pool_dir)
> +            # check if dir is writable
> +            if self._check_pool_dir(pool_dir):
> +                print("Pool directory set to '%s'" % pool_dir)
> +                return 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")
>  
> -        print "File '%s.xml' successfuly created." % output_file
>  
>  class WizardException(Exception):
>      pass
> -- 
> 2.4.3
>
> _______________________________________________
> LNST-developers mailing list
> LNST-developers(a)lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/lnst-developers

[Ondrej Lichtner]

On Fri, Jul 24, 2015 at 01:05:14PM +0200, Jan Tluka wrote:
>  See comments inline. The patch looks good however there are still
> some minor
>  issues around.
>  
>  Tue, Jul 21, 2015 at 11:13:11AM CEST, jprochaz(a)redhat.com wrote:
>  >MessageDispatcherLite.py
>  >+ Remove unused import
>  >+ Make pep8 compliant
>  >+ Rewrite one error message
>  
>  Why we need it? Since Wizard class now communicates with lnst-slave
>  directly we don't need it. Nack, unless there's valid reason to keep the
>  DispatcherLite. 
I agree... MessageDispatcherLite should be removed.

>  
>  >
>  >lnst-pool-wizard
>  >+ Make pep8 compliant
>  >+ Print error messages on stderr
>  >+ Changes correspoding with Wizard.py refactorization
>  >
>  >Wizard.py
>  >+ Make pep8 compliant
>  >+ Convert public methods to private
>  >+ 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(a)redhat.com&gt;
>  >---
>  > lnst-pool-wizard                         |  55 +++--
>  > lnst/Controller/MessageDispatcherLite.py |   9 +-
>  > lnst/Controller/Wizard.py                | 349 ++++++++++++++++++++-----------
>  > 3 files changed, 264 insertions(+), 149 deletions(-)
>  >
>  >diff --git a/lnst-pool-wizard b/lnst-pool-wizard
>  >index c788688..d17d23a 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\n"
>  >     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
>  >index 2973763..623c232 100644
>  >--- a/lnst/Controller/MessageDispatcherLite.py
>  >+++ b/lnst/Controller/MessageDispatcherLite.py
>  >@@ -15,7 +15,9 @@ 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__()
>  > 
>  >@@ -25,9 +27,8 @@ class MessageDispatcherLite(ConnectionHandler):
>  >     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)
>  >+        if send_data(soc, data) is False:
>  >+            raise NetTestError("Connection error from slave")
>  > 
>  >     def wait_for_result(self, machine_id):
>  >         wait = True
>  >@@ -47,7 +48,7 @@ class MessageDispatcherLite(ConnectionHandler):
>  > 
>  >             if connected_slaves != remaining_slaves:
>  >                 disconnected_slaves = set(connected_slaves) -\
>  >-                                      set(remaining_slaves)
>  >+                    set(remaining_slaves)
>  >                 msg = "Slaves " + str(list(disconnected_slaves)) + \
>  >                       " disconnected from the controller."
>  >                 raise NetTestError(msg)
>  >diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py
>  >index c38aae5..7db8459 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,149 @@ published by the Free Software Foundation; see COPYING for
> details.
>  > __author__ = """
>  > jprochaz(a)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/")
>  >+
>  >+
>  > 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()
>  >+        # Invalid pool dir entered, query for correct one
>  >+        elif not self._check_pool_dir(pool_dir):
>  >+            pool_dir = self._query_pool_dir()
>  
>  IMO, this is not the best approach. E.g. user won't specify a pool dir,
>  so the user is prompted to enter directory. But there's no check of it.
>  
>  Also if user specifies the pool dir, it's checked and if it fails then
>  the user is prompted back but there's no check after that.
>  
>  Please fix this.
>  
>  > 
>  >         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
>  >+        # Invalid pool_dir entered, abort wizard
>  >+        if not self._check_pool_dir(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("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)
>  >+            sys.stderr.write("Hostname '%s' is not translatable into a
> valid "
>  >+                             "IP address\n" % hostname)
>  >             return False
>  > 
>  >-    def _write_to_xml(self, msg, hostname, port, mode):
>  >+    def _check_pool_dir(self, pool_dir):
>  >+        """ Checks users access to selected directory
>  >+        @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 os.access(pool_dir, os.W_OK):
>  >+            return True
>  >+        else:
>  >+            sys.stderr.write("Directory '%s' is not writable\n"
>  >+                             % pool_dir)
>  >+            return False
>  >+
>  >+    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 +165,138 @@ 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)
>  
>  How about adding driver information as well?
>  
>  >-        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)
>  >+        mkdir_p(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)
>  >+
>  >+        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_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
>  >+
>  >+    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 == "":
>  >+                return DefaultPoolDir
>  >+
>  >+            pool_dir = os.path.expanduser(pool_dir)
>  >+            # check if dir is writable
>  >+            if self._check_pool_dir(pool_dir):
>  >+                print("Pool directory set to '%s'" % pool_dir)
>  >+                return 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")
>  > 
>  >-        print "File '%s.xml' successfuly created." % output_file
>  > 
>  > class WizardException(Exception):
>  >     pass
>  >-- 
>  >2.4.3
>  >
>  >_______________________________________________
>  >LNST-developers mailing list
>  &gt;LNST-developers(a)lists.fedorahosted.org
>  >https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
>  _______________________________________________
>  LNST-developers mailing list
>  LNST-developers(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/lnst-developers

[Jiri Prochazka]

I'm sorry, the MessageDispatcher class is not used in the refactorized 
version, but I forgot to remove it from repo and from commit message. :-/

The next patch should be final.

Sorry again,
Jiri

On 07/27/2015 08:43 AM, Ondrej Lichtner wrote:
>  On Fri, Jul 24, 2015 at 01:05:14PM +0200, Jan Tluka wrote:
> > See comments inline. The patch looks good however there are still some minor
> > issues around.
> >
> > Tue, Jul 21, 2015 at 11:13:11AM CEST, jprochaz(a)redhat.com wrote:
> >> MessageDispatcherLite.py
> >> + Remove unused import
> >> + Make pep8 compliant
> >> + Rewrite one error message
> > Why we need it? Since Wizard class now communicates with lnst-slave
> > directly we don't need it. Nack, unless there's valid reason to keep the
> > DispatcherLite.
>  I agree... MessageDispatcherLite should be removed.
>
> >> lnst-pool-wizard
> >> + Make pep8 compliant
> >> + Print error messages on stderr
> >> + Changes correspoding with Wizard.py refactorization
> >>
> >> Wizard.py
> >> + Make pep8 compliant
> >> + Convert public methods to private
> >> + 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(a)redhat.com&gt;
> >> ---
> >> lnst-pool-wizard                         |  55 +++--
> >> lnst/Controller/MessageDispatcherLite.py |   9 +-
> >> lnst/Controller/Wizard.py                | 349 ++++++++++++++++++++-----------
> >> 3 files changed, 264 insertions(+), 149 deletions(-)
> >>
> >> diff --git a/lnst-pool-wizard b/lnst-pool-wizard
> >> index c788688..d17d23a 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\n"
> >>      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
> >> index 2973763..623c232 100644
> >> --- a/lnst/Controller/MessageDispatcherLite.py
> >> +++ b/lnst/Controller/MessageDispatcherLite.py
> >> @@ -15,7 +15,9 @@ 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__()
> >>
> >> @@ -25,9 +27,8 @@ class MessageDispatcherLite(ConnectionHandler):
> >>      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)
> >> +        if send_data(soc, data) is False:
> >> +            raise NetTestError("Connection error from slave")
> >>
> >>      def wait_for_result(self, machine_id):
> >>          wait = True
> >> @@ -47,7 +48,7 @@ class MessageDispatcherLite(ConnectionHandler):
> >>
> >>              if connected_slaves != remaining_slaves:
> >>                  disconnected_slaves = set(connected_slaves) -\
> >> -                                      set(remaining_slaves)
> >> +                    set(remaining_slaves)
> >>                  msg = "Slaves " + str(list(disconnected_slaves)) + \
> >>                        " disconnected from the controller."
> >>                  raise NetTestError(msg)
> >> diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py
> >> index c38aae5..7db8459 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,149 @@ published by the Free Software Foundation; see COPYING for
> details.
> >> __author__ = """
> >> jprochaz(a)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/")
> >> +
> >> +
> >> 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()
> >> +        # Invalid pool dir entered, query for correct one
> >> +        elif not self._check_pool_dir(pool_dir):
> >> +            pool_dir = self._query_pool_dir()
> > IMO, this is not the best approach. E.g. user won't specify a pool dir,
> > so the user is prompted to enter directory. But there's no check of it.
> >
> > Also if user specifies the pool dir, it's checked and if it fails then
> > the user is prompted back but there's no check after that.
> >
> > Please fix this.
> >
> >>          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
> >> +        # Invalid pool_dir entered, abort wizard
> >> +        if not self._check_pool_dir(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("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)
> >> +            sys.stderr.write("Hostname '%s' is not translatable
> into a valid "
> >> +                             "IP address\n" % hostname)
> >>              return False
> >>
> >> -    def _write_to_xml(self, msg, hostname, port, mode):
> >> +    def _check_pool_dir(self, pool_dir):
> >> +        """ Checks users access to selected directory
> >> +        @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 os.access(pool_dir, os.W_OK):
> >> +            return True
> >> +        else:
> >> +            sys.stderr.write("Directory '%s' is not
> writable\n"
> >> +                             % pool_dir)
> >> +            return False
> >> +
> >> +    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 +165,138 @@ 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)
> > How about adding driver information as well?
> >
> >> -        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)
> >> +        mkdir_p(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)
> >> +
> >> +        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_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
> >> +
> >> +    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 == "":
> >> +                return DefaultPoolDir
> >> +
> >> +            pool_dir = os.path.expanduser(pool_dir)
> >> +            # check if dir is writable
> >> +            if self._check_pool_dir(pool_dir):
> >> +                print("Pool directory set to '%s'" %
> pool_dir)
> >> +                return 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")
> >>
> >> -        print "File '%s.xml' successfuly created." %
> output_file
> >>
> >> class WizardException(Exception):
> >>      pass
> >> -- 
> >> 2.4.3
> >>
> >> _______________________________________________
> >> LNST-developers mailing list
> >> LNST-developers(a)lists.fedorahosted.org
> >> https://lists.fedorahosted.org/mailman/listinfo/lnst-developers
> > _______________________________________________
> > LNST-developers mailing list
> > LNST-developers(a)lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/lnst-developers