2015-09-01 17:30 GMT+02:00 Jan Tluka <jtluka@redhat.com>:
Fri, Aug 28, 2015 at 11:17:22AM CEST, jprochaz@redhat.com wrote:
>This patch adds import of libvirt library
>This patch adds new method _query_libvirt_domain:
>  - This method needs imported libvirt library to work
>  - It tries to connect to hypervisor, if it succeeds, it checks if
>    guest with entered name exists and tries to recover it's IP
>    from DHCP lease in "default" network. If it fails, it queries
>    user for new libvirt domain, otherwise it returns the string with
>    the domain
>
>Signed-off-by: Jiri Prochazka <jprochaz@redhat.com>
>---
> lnst/Controller/Wizard.py | 40 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
>diff --git a/lnst/Controller/Wizard.py b/lnst/Controller/Wizard.py
>index 570f0d1..137e758 100644
>--- a/lnst/Controller/Wizard.py
>+++ b/lnst/Controller/Wizard.py
>@@ -10,6 +10,7 @@ __author__ = """
> jprochaz@redhat.com (Jiri Prochazka)
> """
>
>+import libvirt

Make this import conditional please, there might be users that do
not have the module available.

> import socket
> import sys
> import time
>@@ -27,6 +28,9 @@ PATH_NOT_DIR = 3
>
>
> class Wizard:
>+    def __init__(self):
>+        # used for storing libvirt connection
>+        self._conn = None
>
>     def interactive(self, pool_dir=None):
>         """ Starts Wizard in an interactive mode
>@@ -375,3 +379,39 @@ class Wizard:
>                     return port
>                 except:
>                     sys.stderr.write("Invalid port entered\n")
>+
>+    def _query_libvirt_domain(self):
>+        """ Queries user for libvirt_domain
>+        @note Virtual host must be running under libvirt
>+              and has to have an IP from "default" network
>+              DHCP server
>+        @return String representing libvirt_domain of host
>+        """
>+        if self._conn is None:
>+            self._conn = libvirt.openReadOnly("qemu:///system")
>+
>+        if self._conn is None:
>+            sys.stderr.write("Failed to open connection to hypervisor\n")
>+            return (None, None)

Can you move this code one level up to virtual()? It looks a bit weird
here.

>+
>+        while True:
>+            libvirt_domain = raw_input("Enter libvirt domain of virtual host: ")
>+            if libvirt_domain == "":
>+                sys.stderr.write("No domain entered\n")
>+                continue
>+            try:
>+                self._conn.lookupByName(libvirt_domain)
>+            except:
>+                continue

                  ^^^ Please be friendly to user, print at least something,
ideally exception name but anything simple would work, too.
​The libvirt itself prints error message for user - ​
 
​"libvirt: QEMU Driver error : Domain not found: no domain with matching name 'xyz'"​

>+
>+            for lease in self._conn.networkLookupByName("default").DHCPLeases():
>+                if lease["hostname"] == libvirt_domain:
>+                    return (lease["ipaddr"], libvirt_domain)
>+
>+            sys.stderr.write("Couldn't find any IP associated with selected libvirt_domain\n")

Which libvirt_domain? Please write the value of libvirt_domain.

>+            answer = raw_input("Do you want to add hostname manually? [Y/n]: ")
>+            if answer.lower() == "y" or answer == "":
>+                hostname = self._query_hostname()
>+                return libvirt_domain, hostname
>+            else:
>+                continue

I don't get it. So if I do not answer Y to the question I will be
silently ignored? I'd rather expect to be asked to enter the hostname directly
and not whether I want to add it manually. Is there option to have it
automagically? I think the question is unnecessary here.

>--
>2.4.3
>
>_______________________________________________
>LNST-developers mailing list
>LNST-developers@lists.fedorahosted.org
>https://lists.fedorahosted.org/mailman/listinfo/lnst-developers