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@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list