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@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