virt-install: Drop --os-variant suboption parsing

Not sure I want to go down that route if we can avoid it. Instead
just fold the full_id support into the existing option handling.
Streamline the OSVariantData usage throughout the cli tools

tests/clitest.py

@@ -825,6 +825,7 @@ c.add_invalid("--arch i686 --install fedora26", grep="does not have a URL locati
 c.add_invalid("-c foo --cdrom bar", grep="Cannot specify both -c")  # check for ambiguous -c and --cdrom collision
 c.add_invalid("-c qemu:///system", grep="looks like a libvirt URI")  # error for the ambiguous -c vs --connect
 c.add_invalid("--location /", grep="Error validating install location")  # detect_distro failure
+c.add_invalid("--os-variant foo://bar", grep="Unknown libosinfo ID")  # bad full id
 
 c = vinst.add_category("single-disk-install", "--nographics --noautoconsole --disk %(EXISTIMG1)s")
 c.add_valid("--hvm --import")  # FV Import install
@@ -897,7 +898,7 @@ c.add_invalid("--file /foo/bar/baz --pxe")  # Trying to use unmanaged storage wi
 c = vinst.add_category("kvm-generic", "--connect %(URI-KVM)s --noautoconsole")
 c.add_compare("--os-variant fedora-unknown --file %(EXISTIMG1)s --location %(TREEDIR)s --extra-args console=ttyS0 --cpu host --channel none --console none --sound none --redirdev none --boot cmdline='foo bar baz'", "kvm-fedoralatest-url", prerun_check=has_old_osinfo)  # Fedora Directory tree URL install with extra-args
 c.add_compare("--test-media-detection %(TREEDIR)s --arch x86_64 --hvm", "test-url-detection")  # --test-media-detection
-c.add_compare("--os-variant full_id=http://fedoraproject.org/fedora/20 --disk %(EXISTIMG1)s,device=floppy --disk %(NEWIMG1)s,size=.01,format=vmdk --location %(TREEDIR)s --extra-args console=ttyS0 --quiet", "quiet-url", prerun_check=has_old_osinfo)  # Quiet URL install should make no noise
+c.add_compare("--os-variant http://fedoraproject.org/fedora/20 --disk %(EXISTIMG1)s,device=floppy --disk %(NEWIMG1)s,size=.01,format=vmdk --location %(TREEDIR)s --extra-args console=ttyS0 --quiet", "quiet-url", prerun_check=has_old_osinfo)  # Quiet URL install should make no noise
 c.add_compare("--cdrom %(EXISTIMG2)s --file %(EXISTIMG1)s --os-variant win2k3 --sound --controller usb", "kvm-win2k3-cdrom")  # HVM windows install with disk
 c.add_compare("--os-variant ubuntusaucy --nodisks --boot cdrom --virt-type qemu --cpu Penryn --input tablet", "qemu-plain")  # plain qemu
 c.add_compare("--os-variant fedora20 --nodisks --boot network --nographics --arch i686", "qemu-32-on-64", prerun_check=has_old_osinfo)  # 32 on 64
@@ -1202,7 +1203,7 @@ c = vixml.add_category("add/rm devices OS KVM", "--connect %(URI-KVM)s test --pr
 c.add_compare("--add-device --disk %(EXISTIMG1)s", "kvm-add-disk-os-from-xml")  # Guest OS (none) from XML
 c.add_compare("--add-device --disk %(EXISTIMG1)s --os-variant fedora28", "kvm-add-disk-os-from-cmdline")  # Guest OS (fedora) provided on command line
 c.add_compare("--add-device --network default", "kvm-add-network-os-from-xml")  # Guest OS information taken from the guest XML
-c.add_compare("--add-device --network default --os-variant full_id=http://fedoraproject.org/fedora/28", "kvm-add-network-os-from-cmdline")  # Guest OS information provided on the command line
+c.add_compare("--add-device --network default --os-variant http://fedoraproject.org/fedora/28", "kvm-add-network-os-from-cmdline")  # Guest OS information provided on the command line
 
 
 
@@ -1314,7 +1315,6 @@ _add_argcomplete_cmd("virt-install --disk source.seclab", "source.seclabel.relab
 _add_argcomplete_cmd("virt-install --check d", "disk_size")
 _add_argcomplete_cmd("virt-install --location k", "kernel")
 _add_argcomplete_cmd("virt-install --install i", "initrd")
-_add_argcomplete_cmd("virt-install --os-variant nam", "name")
 _add_argcomplete_cmd("virt-install --test-stub", None,
         nogrep="--test-stub-command")
 _add_argcomplete_cmd("virt-install --unattended ", "profile=")  # will list all --unattended subprops

tests/xmlparse.py

@@ -123,7 +123,7 @@ class XMLParseTest(unittest.TestCase):
 
         check = self._make_checker(guest._metadata.libosinfo)  # pylint: disable=protected-access
         check("os_id", "http://fedoraproject.org/fedora/17")
-        guest.set_os_full_id("http://fedoraproject.org/fedora/10")
+        guest.set_os_name("fedora10")
         check("os_id", "http://fedoraproject.org/fedora/10")
         self.assertEqual(guest.osinfo.name, "fedora10")
         guest.set_os_name("generic")

virt-install

@@ -360,48 +360,35 @@ def show_warnings(options, guest, installer, osdata):
 # Guest building helpers #
 ##########################
 
-def get_location_for_os(guest, osname, osdata):
+def get_location_for_os(guest, osname):
     osinfo = virtinst.OSDB.lookup_os(osname, raise_error=True)
     location = osinfo.get_location(guest.os.arch)
     print_stdout(_("Using {osname} --location {url}").format(
         osname=osname, url=location))
-    if osdata.default_auto:
-        osdata.default_auto = False
-        osdata.name = osname
     return location
 
 
-def build_installer(options, guest, osdata):
+def build_installer(options, guest, installdata):
     cdrom = None
     location = None
     location_kernel = None
     location_initrd = None
-    install_bootdev = None
-    installdata = None
-    install_kernel = None
-    install_initrd = None
-    install_kernel_args = None
-    install_os = None
-
-    if options.install:
-        installdata = cli.parse_install(options.install)
-
     extra_args = options.extra_args
-    if installdata:
+
-        install_bootdev = installdata.bootdev
+    install_bootdev = installdata.bootdev
-        install_kernel = installdata.kernel
+    install_kernel = installdata.kernel
-        install_initrd = installdata.initrd
+    install_initrd = installdata.initrd
-        install_kernel_args = installdata.kernel_args
+    install_kernel_args = installdata.kernel_args
-        install_os = installdata.os
+    install_os = installdata.os
-        if installdata.kernel_args:
+    if installdata.kernel_args:
-            if installdata.kernel_args_overwrite:
+        if installdata.kernel_args_overwrite:
-                install_kernel_args = installdata.kernel_args
+            install_kernel_args = installdata.kernel_args
-            else:
+        else:
-                extra_args = [installdata.kernel_args]
+            extra_args = [installdata.kernel_args]
 
     no_install = None
     if install_os:
-        location = get_location_for_os(guest, install_os, osdata)
+        location = get_location_for_os(guest, install_os)
     elif options.location:
         (location,
          location_kernel,
@@ -412,7 +399,7 @@ def build_installer(options, guest, osdata):
             no_install = True
     elif options.pxe:
         install_bootdev = "network"
-    elif installdata:
+    elif installdata.is_set:
         pass
     elif (options.import_install or
           options.xmlonly or
@@ -510,8 +497,11 @@ def installer_detect_distro(guest, installer, osdata):
     try:
         # This also validates the install location
         autodistro = installer.detect_distro(guest)
-        if osdata.is_auto and autodistro:
+        name = osdata.name
-            guest.set_os_name(autodistro)
+        if osdata.is_auto:
+            name = autodistro
+        if name:
+            guest.set_os_name(name)
     except ValueError as e:
         fail(_("Error validating install location: %s") % str(e))
 
@@ -530,12 +520,14 @@ def build_guest_instance(conn, options):
     # we are operating on any arch/os/type values passed in with --boot
     guest.set_capabilities_defaults()
 
-    osdata = cli.parse_os_variant(options.os_variant)
+    installdata = cli.parse_install(options.install)
-    installer = build_installer(options, guest, osdata)
+    installer = build_installer(options, guest, installdata)
 
-    # Set osname after
+    # Set guest osname, from commandline or detected from media
+    osdata = cli.parse_os_variant(options.os_variant)
+    if installdata.os:
+        osdata.set_installdata_name(installdata.os)
     guest.set_default_os_name()
-    osdata.set_os_name(guest)
     installer_detect_distro(guest, installer, osdata)
 
     set_cli_defaults(options, guest)

virt-xml

@@ -60,7 +60,8 @@ def set_os_variant(options, guest):
         return
 
     osdata = cli.parse_os_variant(options.os_variant)
-    osdata.set_os_name(guest)
+    if osdata.name:
+        guest.set_os_name(osdata.name)
 
 
 def get_domain_and_guest(conn, domstr):

virtinst/cli.py

@@ -27,6 +27,7 @@ from .connection import VirtinstConnection
 from .devices import (Device, DeviceController, DeviceDisk, DeviceGraphics,
         DeviceInterface, DevicePanic)
 from .nodedev import NodeDevice
+from .osdict import OSDB
 from .storage import StoragePool, StorageVolume
 from .unattended import UnattendedData
 
@@ -462,7 +463,7 @@ def get_meter():
 ###########################
 
 def _get_completer_parsers():
-    return VIRT_PARSERS + [ParserCheck, ParserLocation, ParserOSVariant,
+    return VIRT_PARSERS + [ParserCheck, ParserLocation,
             ParserUnattended, ParserInstall]
 
 
@@ -1586,10 +1587,12 @@ class InstallData:
         self.kernel_args = None
         self.kernel_args_overwrite = None
         self.os = None
+        self.is_set = False
 
 
 def parse_install(optstr):
     installdata = InstallData()
+    installdata.is_set = bool(optstr)
     parser = ParserInstall(optstr or None)
     parser.parse(installdata)
     return installdata
@@ -1629,56 +1632,42 @@ def parse_location(optstr):
 ########################
 
 class OSVariantData(object):
-    def __init__(self):
+    def __init__(self, os_variant):
+        self._rawstr = os_variant
+        self._default_auto = True
         self._name = None
-        self.full_id = None
+        if not self._rawstr:
-        self.is_none = False
+            return
-        self._explicit_auto = False
-        self.default_auto = False
 
-    def _set_name(self, val):
+        self._default_auto = False
-        if val == "auto":
+        if self.is_none or self.is_auto:
-            self._explicit_auto = True
+            return
-        elif val == "none":
+        if "://" in self._rawstr:
-            self.is_none = True
+            osobj = OSDB.lookup_os_by_full_id(self._rawstr, raise_error=True)
         else:
-            self._name = val
+            osobj = OSDB.lookup_os(self._rawstr, raise_error=True)
-    def _get_name(self):
+        self._name = osobj.name
-        return self._name
+
-    name = property(_get_name, _set_name)
+    def set_installdata_name(self, name):
+        # osname set via --install os=X, but if --os-variant also
+        # explicitly set, we don't want to overwrite it
+        if self._default_auto:
+            self._default_auto = False
+            self._name = name
 
     @property
+    def is_none(self):
+        return self._rawstr == "none"
+    @property
     def is_auto(self):
-        if self._name:
+        return self._rawstr == "auto" or self._default_auto
-            return False
+    @property
-        return self._explicit_auto or self.default_auto
+    def name(self):
-
+        return self._name
-    def set_os_name(self, guest):
-        if self.full_id:
-            guest.set_os_full_id(self.full_id)
-        elif self.name:
-            guest.set_os_name(self.name)
-
-
-class ParserOSVariant(VirtCLIParser):
-    cli_arg_name = "os_variant"
-    remove_first = "name"
-
-    @classmethod
-    def _init_class(cls, **kwargs):
-        VirtCLIParser._init_class(**kwargs)
-        cls.add_arg("name", "name")
-        cls.add_arg("full_id", "full_id")
 
 
 def parse_os_variant(optstr):
-    parsedata = OSVariantData()
+    return OSVariantData(optstr)
-    if optstr:
-        parser = ParserOSVariant(optstr)
-        parser.parse(parsedata)
-    else:
-        parsedata.default_auto = True
-    return parsedata
 
 
 ######################
@@ -1706,7 +1695,8 @@ class ParserMetadata(VirtCLIParser):
         inst.set_os_name(val)
 
     def set_os_full_id_cb(self, inst, val, virtarg):
-        inst.set_os_full_id(val)
+        osobj = OSDB.lookup_os_by_full_id(val, raise_error=True)
+        inst.set_os_name(osobj.name)
 
 
 ####################

virtinst/guest.py

@@ -462,14 +462,6 @@ class Guest(XMLBuilder):
         logging.debug("Setting Guest osinfo name %s", obj)
         self._set_os_obj(obj)
 
-    def set_os_full_id(self, full_id):
-        obj = OSDB.lookup_os_by_full_id(full_id)
-        if obj is None:
-            raise ValueError(_("Unknown libosinfo ID '%s'") % full_id)
-
-        logging.debug("Setting Guest osinfo full_id %s", obj)
-        self._set_os_obj(obj)
-
     def set_default_os_name(self):
         self.set_os_name("generic")
 

virtinst/osdict.py

@@ -210,10 +210,12 @@ class _OSDB(object):
     # Public APIs #
     ###############
 
-    def lookup_os_by_full_id(self, full_id):
+    def lookup_os_by_full_id(self, full_id, raise_error=False):
         for osobj in self._all_variants.values():
             if osobj.full_id == full_id:
                 return osobj
+        if raise_error:
+            raise ValueError(_("Unknown libosinfo ID '%s'") % full_id)
 
     def lookup_os(self, key, raise_error=False):
         if key in self._aliases: