Merge ~agherzan/uvtool:ag/libvirtcon into uvtool:main

[Andrei Gherzan]

[Robie Basak (racb)]

Thanks!

[Andrei Gherzan (agherzan)]

Thanks for taking a look.

[Robie Basak (racb)]

FWIW, you probably noticed that the way this is done makes it painful to make the URI a configurable option. This diff reminded me that I thought about this when I wrote it. I imagined that I would need to add some kind of state object to carry user options and pass it around everywhere, to avoid a global variable. Maybe a class, or just a namedtuple or something. To get the initial version out the door I kicked that can down the road, and here we are many years later and you're the first person to notice :-)

[Robie Basak (racb)]

> I imagined that I would need to add some kind of state object to carry user options...

Or indeed something to carry a single persistent libvirt connection around the call stack, which would probably be better.

[Andrei Gherzan (agherzan)]

I agree that some sort of state would be useful here but I don't dislike too much the global variable approach either. It would get a bit convoluted because you would need to pass the state across the tool's invocations which makes it non-trivial. For the sake of the current functionality, I would say the current approach is reasonable. I've done this refactoring to force further developemnt to keep all the commands in sync in terms of the libvirt URI and avoid discrepancies as found with the list command.

diff --git a/uvtool/libvirt/kvm.py b/uvtool/libvirt/kvm.py
index 2fc30ef..218a7f0 100755
--- a/uvtool/libvirt/kvm.py
+++ b/uvtool/libvirt/kvm.py
@@ -57,6 +57,7 @@ DEFAULT_TEMPLATE = '/usr/share/uvtool/libvirt/template.xml'
 DEFAULT_REMOTE_WAIT_SCRIPT = '/usr/share/uvtool/libvirt/remote-wait.sh'
 POOL_NAME = 'uvtool'
 
+LIBVIRT_URI = 'qemu:///system'
 
 class CLIError(Exception):
     """An error that should be reflected back to the CLI user."""
@@ -300,7 +301,7 @@ def create_cow_volume(backing_volume_name, new_volume_name, new_volume_size,
         conn=None):
 
     if conn is None:
-        conn = libvirt.open('qemu:///system')
+        conn = libvirt.open(LIBVIRT_URI)
 
     pool = conn.storagePoolLookupByName(POOL_NAME)
     try:
@@ -320,7 +321,7 @@ def create_cow_volume_by_path(backing_volume_path, new_volume_name,
     """Create a new libvirt qcow2 volume backed by an existing volume path."""
 
     if conn is None:
-        conn = libvirt.open('qemu:///system')
+        conn = libvirt.open(LIBVIRT_URI)
 
     pool = conn.storagePoolLookupByName(POOL_NAME)
 
@@ -349,7 +350,7 @@ def get_boot_item_path(item_name, conn=None):
     this helper gets that path (if one already exists) and
     returns None if it doesn't."""
     if conn is None:
-        conn = libvirt.open('qemu:///system')
+        conn = libvirt.open(LIBVIRT_URI)
 
     pool = conn.storagePoolLookupByName(POOL_NAME)
     try:
@@ -365,7 +366,7 @@ def remove_boot_item(hostname, item, conn=None):
     item_name = get_boot_item_name(hostname, item)
 
     if conn is None:
-        conn = libvirt.open('qemu:///system')
+        conn = libvirt.open(LIBVIRT_URI)
 
     pool = conn.storagePoolLookupByName(POOL_NAME)
     try:
@@ -383,7 +384,7 @@ def get_boot_item(backing_volume_name, hostname, item, conn=None):
     inability to run as root."""
 
     if conn is None:
-        conn = libvirt.open('qemu:///system')
+        conn = libvirt.open(LIBVIRT_URI)
 
     item_name = get_boot_item_name(hostname, item)
     item_path = get_boot_item_path(item_name, conn)
@@ -651,7 +652,7 @@ def create(
             initrd=initrd,
         )
 
-        conn = libvirt.open('qemu:///system')
+        conn = libvirt.open(LIBVIRT_URI)
         domain = conn.defineXML(xml)
         if start:
             try:
@@ -686,7 +687,7 @@ def delete_domain_volumes(conn, domain):
 
 
 def destroy(hostname):
-    conn = libvirt.open('qemu:///system')
+    conn = libvirt.open(LIBVIRT_URI)
     try:
         domain = conn.lookupByName(hostname)
     except libvirt.libvirtError as e:
@@ -920,7 +921,7 @@ def main_destroy(parser, args):
 def main_list(parser, args):
     # TODO Implement this natively while taking into consideration the
     # connection libvirt.
-    subprocess.check_call('virsh --connect qemu:///system -q list --all',
+    subprocess.check_call('virsh --connect {} -q list --all'.format(LIBVIRT_URI),
             shell=True
     )
 
@@ -986,7 +987,7 @@ def main_wait_remote(parser, args):
 
 
 def main_wait(parser, args):
-    conn = libvirt.open('qemu:///system')
+    conn = libvirt.open(LIBVIRT_URI)
     domain = conn.lookupByName(args.name)
     state = domain.state(0)[0]
     if state != libvirt.VIR_DOMAIN_RUNNING: