cli: Skip MAC collision error with --check mac_in_use=

There's valid cases where a VM can be defined with a conflicting MAC
address. Prior to  ebd6091cc8 and related refactorings we were more
lax here if the conflicting VM wasn't running, but now we are blocking
some valid usage.

Hoist the validation check up to cli.py and add --check mac_in_use=off
to skip the validation. Advertise it like we do for other checks, so
now a collision error will look something like:

The MAC address '22:11:11:11:11:11' is in use by another virtual
machine. (Use --check mac_in_use=off or --check all=off to override)

Reported-by: Pino Toscano <ptoscano@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>

tests/test_cli.py

@@ -1105,11 +1105,12 @@ c.add_valid("--network bridge:mybr0,model=e1000")  # --network bridge:
 c.add_valid("--network network:default --mac RANDOM")  # VirtualNetwork with a random macaddr
 c.add_valid("--vnc --keymap=local")  # --keymap local
 c.add_valid("--panic 0x505")  # ISA panic with iobase specified
+c.add_valid("--mac 22:11:11:11:11:11 --check mac_in_use=off")  # colliding mac, but check is skipped
+c.add_invalid("--mac 22:11:11:11:11:11")  # Colliding macaddr will error
 c.add_invalid("--graphics vnc --vnclisten 1.2.3.4")  # mixing old and new
 c.add_invalid("--network=FOO")  # Nonexistent network
 c.add_invalid("--mac 1234")  # Invalid mac
 c.add_invalid("--network user --bridge foo0")  # Mixing bridge and network
-c.add_invalid("--connect %(URI-TEST-FULL)s --mac 22:22:33:12:34:AB")  # Colliding macaddr
 
 c = vinst.add_category("storage-back-compat", "--pxe --noautoconsole")
 c.add_valid("--file %(EXISTIMG1)s --nonsparse --file-size 4")  # Existing file, other opts
@@ -1321,6 +1322,8 @@ c.add_valid("--original-xml " + _CLONE_UNMANAGED + " --auto-clone")  # Auto flag
 c.add_valid("--connect %(URI-TEST-FULL)s -o test-clone --auto-clone --clone-running --nonsparse")  # Auto flag, actual VM, skip state check
 c.add_valid("--connect %(URI-TEST-FULL)s -o test-clone-simple -n newvm --preserve-data --file %(EXISTIMG1)s")  # Preserve data shouldn't complain about existing volume
 c.add_valid("-n clonetest --original-xml " + _CLONE_UNMANAGED + " --file %(EXISTIMG3)s --file %(EXISTIMG4)s --check path_exists=off")  # Skip existing file check
+c.add_valid("-n clonetest --original-xml " + _CLONE_UNMANAGED + " --auto-clone --mac 22:11:11:11:11:11 --check all=off")  # Colliding mac but we skip the check
+c.add_invalid("-n clonetest --original-xml " + _CLONE_UNMANAGED + " --auto-clone --mac 22:11:11:11:11:11", grep="--check mac_in_use=off")  # Colliding mac should fail
 c.add_invalid("--auto-clone")  # Just the auto flag
 c.add_invalid("-o test --clone-running --file foo")  # Didn't specify new name
 c.add_invalid("-o test --clone-running --auto-clone -n test")  # new name raises error

tests/test_xmlconfig.py

@@ -268,6 +268,9 @@ class TestXMLMisc(unittest.TestCase):
         self.assertTrue(qemumac != testmac)
         self.assertTrue(len(randommac) == len(testmac))
 
+        # Ensure is_conflict_net doesn't error on None
+        virtinst.DeviceInterface.is_conflict_net(self.conn, None)
+
     def test_support_misc(self):
         try:
             self.conn.lookupByName("foobar-idontexist")

virtManager/device/netlist.py

@@ -298,6 +298,7 @@ class vmmNetworkList(vmmGObjectUI):
 
     def validate_device(self, net):
         self._check_network_is_running(net)
+        virtinst.DeviceInterface.is_conflict_net(net.conn, net.macaddr)
         net.validate()
 
     def reset_state(self):

virtinst/cli.py

@@ -311,18 +311,35 @@ def check_path_search(conn, path):
         path, searchdata.user, searchdata.fixlist)  # pragma: no cover
 
 
+def _optional_fail(msg, checkname, warn_on_skip=True):
+    """
+    Handle printing a message with an associated --check option
+    """
+    do_check = get_global_state().get_validation_check(checkname)
+    if do_check:
+        fail(msg + (_(" (Use --check %s=off or "
+            "--check all=off to override)") % checkname))
+
+    log.debug("Skipping --check %s error condition '%s'",
+        checkname, msg)
+    if warn_on_skip:
+        log.warning(msg)
+
+
+def validate_mac(conn, macaddr):
+    """
+    There's legitimate use cases for creating/cloning VMs with duplicate
+    MACs, so we do the collision check here but allow it to be skipped
+    with --check
+    """
+    try:
+        DeviceInterface.is_conflict_net(conn, macaddr)
+        return
+    except Exception as e:
+        _optional_fail(str(e), "mac_in_use")
+
+
 def validate_disk(dev, warn_overwrite=False):
-    def _optional_fail(msg, checkname, warn_on_skip=True):
-        do_check = get_global_state().get_validation_check(checkname)
-        if do_check:
-            fail(msg + (_(" (Use --check %s=off or "
-                "--check all=off to override)") % checkname))
-
-        log.debug("Skipping --check %s error condition '%s'",
-            checkname, msg)
-        if warn_on_skip:
-            log.warning(msg)
-
     def check_path_exists(dev):
         """
         Prompt if disk file already exists and preserve mode is not used
@@ -1586,6 +1603,8 @@ class ParserCheck(VirtCLIParser):
                     cb=cls.set_cb, lookup_cb=None)
         cls.add_arg("path_exists", None, is_onoff=True,
                     cb=cls.set_cb, lookup_cb=None)
+        cls.add_arg("mac_in_use", None, is_onoff=True,
+                    cb=cls.set_cb, lookup_cb=None)
         cls.add_arg("all", "all_checks", is_onoff=True)
 
     def set_cb(self, inst, val, virtarg):

virtinst/cloner.py

@@ -171,10 +171,7 @@ class Cloner(object):
 
     # MAC address for the new guest clone
     def set_clone_macs(self, mac):
-        maclist = xmlutil.listify(mac)
-        for m in maclist:
-            DeviceInterface.is_conflict_net(self.conn, m)
-        self._clone_macs = maclist
+        self._clone_macs = xmlutil.listify(mac)
     def get_clone_macs(self):
         return self._clone_macs
     clone_macs = property(get_clone_macs, set_clone_macs)

virtinst/devices/interface.py

@@ -159,6 +159,9 @@ class DeviceInterface(Device):
         """
         Raise RuntimeError if the passed mac conflicts with a defined VM
         """
+        if not searchmac:
+            return
+
         vms = conn.fetch_all_domains()
         for vm in vms:
             for nic in vm.devices.interface:
@@ -252,12 +255,6 @@ class DeviceInterface(Device):
     # Build API #
     #############
 
-    def validate(self):
-        if not self.macaddr:
-            return
-
-        self.is_conflict_net(self.conn, self.macaddr)
-
     def set_default_source(self):
         if self.conn.is_qemu_unprivileged() or self.conn.is_test():
             self.type = self.TYPE_USER

virtinst/virtclone.py

@@ -51,6 +51,8 @@ def get_clone_macaddr(new_mac, design):
         return
     design.clone_macs = new_mac
 
+    for mac in design.clone_macs:
+        cli.validate_mac(design.conn, mac)
 
 
 def get_clone_diskfile(new_diskfiles, design, preserve, auto_clone):

virtinst/virtinstall.py

@@ -588,6 +588,8 @@ def build_guest_instance(conn, options):
         # cli specific disk validation
         for disk in guest.devices.disk:
             cli.validate_disk(disk)
+        for net in guest.devices.interface:
+            cli.validate_mac(net.conn, net.macaddr)
 
     validate_required_options(options, guest, installer)
     show_guest_warnings(options, guest, osdata)