[virt-tools-list] [virt-install PATCH] Support multiple seclabels

[Cole Robinson]

Hi Martin, sorry for lack of review for so long.

Unfortunately this changes public API, for example changing secmodel to be a
list will break virt-manager, do a 'git grep secmodel' in virt-manager to see
where it is used.

You can work around it by adding a new 'secmodels' element which is a list,
and turn secmodel into a property() which just does

    return self.secmodels and self.secmodels[0] or None

virt-manager and virtinst will be joined Real Soon Now so we can remove that
wart, but for the time being that bit is required.

Patch looks great otherwise, just one small nit mentioned inline below.

On 11/02/2012 06:55 PM, Martin Kletzander wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=832339
> 
> Until now, virt-install supported only one seclabel and it was the
> first one libvirt reported in capabilities.  This patch adds support
> for more of them and also adds a functionality to try to match the
> right one from the label given.  This is done by checking how many
> colons the label has (precisely said, to how many parts it is split
> by the colons).
> ---
>  tests/capabilities.py          |  4 ++--
>  virtinst/CapabilitiesParser.py |  6 +++---
>  virtinst/Seclabel.py           | 33 +++++++++++++++++++++++++++++----
>  virtinst/VirtualDisk.py        |  8 ++------
>  4 files changed, 36 insertions(+), 15 deletions(-)
> 
> diff --git a/tests/capabilities.py b/tests/capabilities.py
> index 8ce9652..8180665 100644
> --- a/tests/capabilities.py
> +++ b/tests/capabilities.py
> @@ -55,8 +55,8 @@ class TestCapabilities(unittest.TestCase):
>                  self.assertEqual(host_features[n], caps.host.features[n])
> 
>          if secmodel:
> -            self.assertEqual(secmodel[0], caps.host.secmodel.model)
> -            self.assertEqual(secmodel[1], caps.host.secmodel.doi)
> +            self.assertEqual(secmodel[0], caps.host.secmodel[0].model)
> +            self.assertEqual(secmodel[1], caps.host.secmodel[0].doi)
> 
>          map(self._compareGuest, guests, caps.guests)
> 
> diff --git a/virtinst/CapabilitiesParser.py b/virtinst/CapabilitiesParser.py
> index 4c1398c..7b56e35 100644
> --- a/virtinst/CapabilitiesParser.py
> +++ b/virtinst/CapabilitiesParser.py
> @@ -1,7 +1,7 @@
>  #
>  # Some code for parsing libvirt's capabilities XML
>  #
> -# Copyright 2007  Red Hat, Inc.
> +# Copyright 2007, 2012  Red Hat, Inc.
>  # Mark McLoughlin <markmc at redhat.com>
>  #
>  # This program is free software; you can redistribute it and/or modify
> @@ -260,7 +260,7 @@ class Host(object):
>      def __init__(self, node=None):
>          self.cpu = CPU()
>          self.topology = None
> -        self.secmodel = None
> +        self.secmodel = []
> 
>          if not node is None:
>              self.parseXML(node)
> @@ -285,7 +285,7 @@ class Host(object):
>                  self.topology = Topology(child)
> 
>              if child.name == "secmodel":
> -                self.secmodel = SecurityModel(child)
> +                self.secmodel.append(SecurityModel(child))
> 
>              if child.name == "cpu":
>                  self.cpu = CPU(child)
> diff --git a/virtinst/Seclabel.py b/virtinst/Seclabel.py
> index 7682664..0925dac 100644
> --- a/virtinst/Seclabel.py
> +++ b/virtinst/Seclabel.py
> @@ -1,5 +1,5 @@
>  #
> -# Copyright 2010  Red Hat, Inc.
> +# Copyright 2010, 2012  Red Hat, Inc.
>  # Cole Robinson <crobinso at redhat.com>
>  #
>  # This program is free software; you can redistribute it and/or modify
> @@ -32,6 +32,11 @@ class Seclabel(XMLBuilderDomain.XMLBuilderDomain):
> 
>      MODEL_DEFAULT = "default"
> 
> +    SECLABEL_MODEL_SELINUX = "selinux"
> +    SECLABEL_MODEL_DAC = "dac"
> +    SECLABEL_MODEL_NONE = "none"
> +    SECLABEL_MODELS = [ SECLABEL_MODEL_SELINUX, SECLABEL_MODEL_DAC, SECLABEL_MODEL_NONE ]
> +
>      _dumpxml_xpath = "/domain/seclabel"
>      def __init__(self, conn, parsexml=None, parsexmlnode=None, caps=None):
>          XMLBuilderDomain.XMLBuilderDomain.__init__(self, conn, parsexml,
> @@ -50,7 +55,10 @@ class Seclabel(XMLBuilderDomain.XMLBuilderDomain):
>          self.type = self.SECLABEL_TYPE_DEFAULT
> 
>      def _get_default_model(self):
> -        return self._get_caps().host.secmodel.model
> +        for model in self.SECLABEL_MODELS:
> +            if model in [x.model for x in self._get_caps().host.secmodels]:
> +                return model
> +        raise RuntimeError("No supported model found in capabilities")
> 
>      def get_type(self):
>          return self._type
> @@ -100,8 +108,6 @@ class Seclabel(XMLBuilderDomain.XMLBuilderDomain):
>          typ = self.type
>          relabel = self.relabel
> 
> -        if model == self.MODEL_DEFAULT:
> -            model = self._get_default_model()
>          if typ == self.SECLABEL_TYPE_DEFAULT:
>              typ = self.SECLABEL_TYPE_DYNAMIC
> 
> @@ -113,6 +119,25 @@ class Seclabel(XMLBuilderDomain.XMLBuilderDomain):
>                  raise RuntimeError("A label must be specified for static "
>                                     "security type.")
> 
> +        if model == self.MODEL_DEFAULT:
> +            if not self.label and not self.imagelabel:
> +                model = self._get_default_model()
> +            else:
> +                lab_len = imglab_len = None
> +                if self.label:
> +                    lab_len = min(3, len(self.label.split(':')))
> +                if self.imagelabel:
> +                    imglab_len = min(3, len(self.imagelabel.split(':')))
> +                if lab_len and imglab_len and lab_len != imglab_len:
> +                    raise ValueError("Label and Imagelabel are incompatible")
> +
> +                lab_len = lab_len or imglab_len
> +                if lab_len == 3:
> +                    self.model = self.SECLABEL_MODEL_SELINUX
> +                elif lab_len == 2:
> +                    self.model = self.SECLABEL_MODEL_DAC
> +                else:
> +                    raise ValueError("Unknown model type for label '%s'" % self.label)
> 

I'd break this guessing block out into a separate function, like

def _guess_secmodel(label, imagelabel)

Thanks,
Cole