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

[Jiri Denemark]

On Fri, Aug 31, 2012 at 12:51:51 +0800, Daniel Veillard wrote:
> 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);
...
>   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'/>

This is most likely because the above condition is missing the (type is none)
part, i.e., it should be

    if (def->nseclabels == 1 &&
        !def->seclabels[0]->model &&
        (def->seclabels[0]->type == VIR_DOMAIN_SECLABEL_NONE ||
         (def->seclabels[0]->type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
          !def->seclabels[0]->baselabel &&
          (flags & VIR_DOMAIN_XML_INACTIVE))) &&
        host->nsecModels > 0) {
        ...

Heh, that looks awful :-) Actually

    if (def->nseclabels == 1 &&
        !def->seclabels[0]->model &&
        host->nsecModels > 0) {
        if (def->seclabels[0]->type == VIR_DOMAIN_SECLABEL_NONE ||
            (def->seclabels[0]->type == VIR_DOMAIN_SECLABEL_DYNAMIC &&
             !def->seclabels[0]->baselabel &&
             (flags & VIR_DOMAIN_XML_INACTIVE))) {
             def->seclabels[0]->model = strdup(host->secModels[0].model);
        } else {
            virReportError(VIR_ERR_XML_ERROR, "%s", "missing security model");
            goto error;
        }
    }

would be a bit more readable and would also avoid confusing error message when
the model is missing but only one seclabel is used. I will try to do some
testing with this changes...

Jirka