[libvirt] [PATCH v2] conf: Fix parsing of seclabels without model

[Daniel Veillard]

On Thu, Aug 30, 2012 at 09:39:03PM -0300, Marcelo Cerri wrote:
> With this patch libvirt tries to assign a model to a single seclabel
> when model is missing. Libvirt will look up at host's capabilities and
> assign the first model to seclabel.
> 
> This patch fixes:
> 
> 1. The problem with existing guests that have a seclabel defined in its XML.
> 2. A XML parse error when a guest is restored.
> 
> Signed-off-by: Marcelo Cerri <mhcerri at linux.vnet.ibm.com>
> ---
>  src/conf/domain_conf.c | 64 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 27 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 224aec5..42c3900 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3102,22 +3102,10 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
>          def->baselabel = p;
>      }
>  
> -    /* Only parse model, if static labelling, or a base
> -     * label is set, or doing active XML
> -     */
> -    if (def->type == VIR_DOMAIN_SECLABEL_STATIC ||
> -        def->baselabel ||
> -        (!(flags & VIR_DOMAIN_XML_INACTIVE) &&
> -         def->type != VIR_DOMAIN_SECLABEL_NONE)) {
> -
> -        p = virXPathStringLimit("string(./@model)",
> -                                VIR_SECURITY_MODEL_BUFLEN-1, ctxt);
> -        if (p == NULL && def->type != VIR_DOMAIN_SECLABEL_NONE) {
> -            virReportError(VIR_ERR_XML_ERROR,
> -                           "%s", _("missing security model"));
> -        }
> -        def->model = p;
> -    }
> +    /* Always parse model */
> +    p = virXPathStringLimit("string(./@model)",
> +                            VIR_SECURITY_MODEL_BUFLEN-1, ctxt);
> +    def->model = p;
>  
>      return def;
>  
> @@ -3129,10 +3117,12 @@ error:
>  static int
>  virSecurityLabelDefsParseXML(virDomainDefPtr def,
>                              xmlXPathContextPtr ctxt,
> +                            virCapsPtr caps,
>                              unsigned int flags)
>  {
>      int i = 0, n;
>      xmlNodePtr *list = NULL, saved_node;
> +    virCapsHostPtr host = &caps->host;
>  
>      /* Check args and save context */
>      if (def == NULL || ctxt == NULL)
> @@ -3159,18 +3149,38 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def,
>      ctxt->node = saved_node;
>      VIR_FREE(list);
>  
> -    /* Checking missing model information
> -     * when there is more than one seclabel */
> -    if (n > 1) {
> -        for(; n; n--) {
> -            if (def->seclabels[n - 1]->model == NULL) {
> -                virReportError(VIR_ERR_XML_ERROR, "%s",
> -                                     _("missing security model "
> -                                       "when using multiple labels"));
> -                goto error;
> -            }
> +    /* libvirt versions prior to 0.10.0 support just a single seclabel element
> +     * in guest's XML and model attribute can be suppressed if type is none or
> +     * type is dynamic, baselabel is not defined and INACTIVE flag is set.
> +     *
> +     * To avoid compatibility issues, for this specific case the first model
> +     * defined in host's capabilities is used as model for the seclabel.
> +     */
> +    if (def->nseclabels == 1 &&
> +        def->seclabels[0]->model == NULL &&
> +        def->seclabels[0]->type != VIR_DOMAIN_SECLABEL_STATIC &&
> +        def->seclabels[0]->baselabel == NULL &&
> +        (flags & VIR_DOMAIN_XML_INACTIVE) &&
> +        host->nsecModels > 0) {
> +
> +        /* Copy model from host. */
> +        def->seclabels[0]->model = strdup(host->secModels[0].model);
> +        if (def->seclabels[0]->model == NULL) {
> +            virReportOOMError();
> +            goto error;
>          }
>      }
> +
> +    /* Checking missing model information */
> +    for(; n; n--) {
> +        if (def->seclabels[n - 1]->model == NULL) {
> +            virReportError(VIR_ERR_XML_ERROR, "%s",
> +                                 _("missing security model "
> +                                   "when using multiple labels"));
> +            goto error;
> +        }
> +    }
> +
>      return 0;
>  
>  error:
> @@ -8166,7 +8176,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>  
>      /* analysis of security label, done early even though we format it
>       * late, so devices can refer to this for defaults */
> -    if (virSecurityLabelDefsParseXML(def, ctxt, flags) == -1)
> +    if (virSecurityLabelDefsParseXML(def, ctxt, caps, flags) == -1)
>          goto error;
>  
>      /* Extract domain memory */

  Fails "make check"

thinkpad:~/libvirt/tests -> export VIR_TEST_DEBUG=2
thinkpad:~/libvirt/tests -> ./qemuxml2argvtest
...
219) QEMU XML-2-ARGV seclabel-none
... libvir: Domain Config error : XML error: missing security model when
using multiple labels
FAILED
...
thinkpad:~/libvirt/tests -> grep -C2 seclabel qemuxml2argvdata/qemuxml2argv-seclabel-none.xml
    <memballoon model='virtio'/>
  </devices>
  <seclabel type='none'/>
</domain>
thinkpad:~/libvirt/tests ->

  So the new code following your patch is unable to parse the construct
   <seclabel type='none'/>

 I just can't accept that patch, it's broken, and make check points it
out very clearly !

 Please don't send patches that break "make check" or "make syntax-check"

That said we still need to fix the issue I see:

define a guest with
  <seclabel type='dynamic' relabel='yes'/>
start it with git head

downgrade to 0.9.13

libvirtd will fail reconnecting to the guest and show it as shutoff.
We need to fix that scenario before pushing 0.10.1, but breaking
<seclabel type='none'/> would be an even worse regression I think.

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/