[virt-tools-list] [PATCH virt-manager] virtinst: Add ability of detecting suse distros from a given URL
Cole Robinson
crobinso at redhat.com
Mon May 25 22:23:30 UTC 2015
On 05/25/2015 05:23 AM, Lin Ma wrote:
> SUSE distros include a file called content, Its purpose is similar with .treeinfo,
> Despite SUSE doesn't define the standard format for media content, we can get the
> distro's information to help distro detection from a given URL.
>
> From: Charles Arnold <carnold at suse.com>
> Signed-off-by: Lin Ma <lma at suse.com>
Thanks for the patch.
First, we have a manual URL test suite. See tests/test_urls.py and python
setup.py test_urls --help. You can run the current suse URL tests with: python
setup.py test_urls --only \*suse\*
Please extend that as much as possible to ensure this new code is tested. I
recommend running with --coverage as well, the coverage annotate -d outputdir
to ensure as much of the code is covered as possible.
Also, this patch is quite large, please consider trying to split it into more
manageable chunks which will make review easier. There's multiple new pieces
here like the 'detect from content', s390 and other arch handling, and the
introduction of other distro classes like sled etc.
Finally, try running 'python setup.py pylint', I suspect there's some
violations added here.
Review comments inline
> ---
> virtinst/urlfetcher.py | 169 ++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 161 insertions(+), 8 deletions(-)
>
> diff --git a/virtinst/urlfetcher.py b/virtinst/urlfetcher.py
> index 2b15bf8..bbe1826 100644
> --- a/virtinst/urlfetcher.py
> +++ b/virtinst/urlfetcher.py
> @@ -316,6 +316,94 @@ def _distroFromTreeinfo(fetcher, arch, vmtype=None):
>
> return ob
>
> +def _distroFromContent(fetcher, arch, vmtype=None):
This is a generic sounding name, can you change it to _distroFromSUSEContent
or _distroFromContentFile ?
> + # Parse content file for the 'LABEL' field containing the distribution name
> + # None if no content, GenericDistro if unknown label type.
> + if not fetcher.hasFile("content"):
> + return None
> +
> + distribution = None
> + distro_version = None
> + distro_summary = None
> + distro_distro = None
> + distro_arch = None
> + filename = fetcher.acquireFile("content")
> + cbuf = f = None
> + try:
> + f = open(filename, "r")
> + cbuf = f.read()
> + except:
> + if f:
> + f.close()
> + os.unlink(filename)
> + return None
> + f.close()
> + os.unlink(filename)
Do
try:
cbuf = open(filename).read()
finally:
os.unlink(filename)
file() objects are automatically cleaned up when they go out of scope so they
don't need to be manually released (there's bad example code elsewhere in this
file...)
> +
> + lines = cbuf.splitlines()[1:]
> + for line in lines:
> + if line.startswith("LABEL "):
> + distribution = line.split(' ', 1)
> + elif line.startswith("DISTRO "):
> + distro_distro = line.rsplit(',', 1)
> + elif line.startswith("VERSION "):
> + distro_version = line.split(' ', 1)
> + elif line.startswith("SUMMARY "):
> + distro_summary = line.split(' ', 1)
> + elif line.startswith("BASEARCHS "):
> + distro_arch = line.split(' ', 1)
> + elif line.startswith("DEFAULTBASE "):
> + distro_arch = line.split(' ', 1)
> + elif line.startswith("REPOID "):
> + distro_arch = line.rsplit('/', 1)
> + if distribution and distro_version and distro_arch:
> + break
> +
> + if not distribution:
> + if distro_summary:
> + distribution = distro_summary
> + elif distro_distro:
> + distribution = distro_distro
> + if distro_arch:
> + arch = distro_arch[1].strip()
> + else:
> + if cbuf.find("x86_64") != -1:
> + arch = "x86_64"
> + elif cbuf.find("i586") != -1:
> + arch = "i586"
> + elif cbuf.find("s390x") != -1:
> + arch = "s390x"
> +
> + dclass = GenericDistro
> + if distribution:
> + if re.match(".*SUSE Linux Enterprise Server*", distribution[1]) or \
> + re.match(".*SUSE SLES*", distribution[1]):
> + dclass = SLESDistro
> + if distro_version is None:
> + distro_version = ['VERSION', distribution[1].strip().rsplit(' ')[4]]
> + elif re.match(".*SUSE Linux Enterprise Desktop*", distribution[1]):
> + dclass = SLEDDistro
> + if distro_version is None:
> + distro_version = ['VERSION', distribution[1].strip().rsplit(' ')[4]]
> + elif re.match(".*openSUSE.*", distribution[1]):
> + dclass = OpensuseDistro
> + if distro_version is None:
> + distro_version = ['VERSION', distribution[0].strip().rsplit(':')[4]]
> + # For tumbleweed we only have an 8 character date string so default to 13.2
> + if distro_version[1] and len(distro_version[1]) == 8:
> + distro_version = ['VERSION', '13.2']
> +
> + if distro_version is None:
> + return None
> +
> + ob = dclass(fetcher, arch, vmtype)
> + if dclass != GenericDistro:
> + ob.content = distro_version
> +
> + # Explictly call this, so we populate os_type/variant info
> + ob.isValidStore()
> +
> + return ob
>
> def getDistroStore(guest, fetcher):
> stores = []
> @@ -332,6 +420,10 @@ def getDistroStore(guest, fetcher):
> if dist:
> return dist
>
> + dist = _distroFromContent(fetcher, arch, _type)
> + if dist:
> + return dist
> +
> stores = _allstores[:]
>
> # If user manually specified an os_distro, bump it's URL class
> @@ -830,29 +922,69 @@ class SLDistro(RHELDistro):
>
> class SuseDistro(Distro):
> name = "SUSE"
> - urldistro = "suse"
> - os_variant = "linux"
>
> _boot_iso_paths = ["boot/boot.iso"]
>
> def __init__(self, *args, **kwargs):
> + self.content = None
Please call this 'self.version_from_content' to make it more clear
> Distro.__init__(self, *args, **kwargs)
> if re.match(r'i[4-9]86', self.arch):
> self.arch = 'i386'
>
> - # Tested with Opensuse >= 10.2, 11, and sles 10
> - self._hvm_kernel_paths = [("boot/%s/loader/linux" % self.arch,
> - "boot/%s/loader/initrd" % self.arch)]
> + oldkern = "linux"
> + oldinit = "initrd"
> + if self.arch == "x86_64":
> + oldkern += "64"
> + oldinit += "64"
> +
> + if self.arch == "s390x" or \
> + self.arch == "ppc64" or self.arch == "ppc64le":
> +
> + self._hvm_kernel_paths = [ ("boot/%s/linux" % self.arch,
> + "boot/%s/initrd" % self.arch) ]
> + # No Xen on s390x and ppc
> + self._xen_kernel_paths = []
> + else:
> + # Tested with Opensuse >= 10.2, 11, and sles 10
> + self._hvm_kernel_paths = [ ("boot/%s/loader/linux" % self.arch,
> + "boot/%s/loader/initrd" % self.arch) ]
> + # Tested with Opensuse 10.0
> + self._hvm_kernel_paths.append(("boot/loader/%s" % oldkern,
> + "boot/loader/%s" % oldinit))
>
> - # Matches Opensuse > 10.2 and sles 10
> - self._xen_kernel_paths = [("boot/%s/vmlinuz-xen" % self.arch,
> - "boot/%s/initrd-xen" % self.arch)]
> + # Matches Opensuse > 10.2 and sles 10
> + self._xen_kernel_paths = [ ("boot/%s/vmlinuz-xen" % self.arch,
> + "boot/%s/initrd-xen" % self.arch) ]
>
> def isValidStore(self):
> + # self.content is the VERSION line from the contents file
> + if self.content is None or self.content[1] is None:
> + return False
> + distro_version = self.content[1].strip()
> + version = distro_version.split('.', 1)[0].strip()
> + if int(version) >= 10:
> + if self.os_variant.startswith(("sles", "sled")):
> + sp_version = None
> + if len(distro_version.split('.', 1)) == 2:
> + sp_version = 'sp' + distro_version.split('.', 1)[1].strip()
> + self.os_variant += version
> + if sp_version:
> + self.os_variant += sp_version
> + else:
> + self.os_variant += distro_version
> + else:
> + self.os_variant += "9"
> +
Can you separate this version wrangling into a separate private function like
'_osVariantFromVersion' or something.
> if not self.fetcher.hasFile("directory.yast"):
> return False
>
I'm guessing this directory.yast check is redundant now since we require the
'content' file to exist.
> self.os_variant = self._detect_osdict_from_url()
Then we end up overwriting os_variant here... but hopefully the test suite
will exercise all this stuff.
> +
> + # Reset kernel name for sle11 source on s390x
> + if self.arch == "s390x":
> + if self.os_variant == "sles11" or self.os_variant == "sled11":
> + self._hvm_kernel_paths = [ ("boot/%s/vmrdr.ikr" % self.arch,
> + "boot/%s/initrd" % self.arch) ]
> return True
>
> def _get_method_arg(self):
> @@ -872,6 +1004,27 @@ class SuseDistro(Distro):
> return osobj.name
> return self.os_variant
>
> +class SLESDistro(SuseDistro):
> +
> + urldistro = "sles"
> + os_variant = "sles"
> +
> +class SLEDDistro(SuseDistro):
> +
> + urldistro = "sled"
> + os_variant = "sled"
> +
> +# Suse image store is harder - we fetch the kernel RPM and a helper
> +# RPM and then munge bits together to generate a initrd
> +class OpensuseDistro(SuseDistro):
> +
> + urldistro = "opensuse"
> + os_variant = "opensuse"
> +
> +class OESDistro(SuseDistro):
> +
> + urldistro = "oes"
> + os_variant = "oes"
>
These os_variant values are meant to be valid libosinfo IDs... but I don't
think unversioned "opensuse" or "sled" is a valid libosinfo name, is it? Even
if they are unconditionally overwritten, we shouldn't have invalid values here.
Thanks,
Cole
More information about the virt-tools-list
mailing list