From: Michal Privoznik <mprivozn@redhat.com>

Prior to v7.10.0-rc1~26 seclabels defaulted to
VIR_DOMAIN_SECLABEL_DYNAMIC (type='dynamic'). But after switching
the parser to virXMLPropEnum() the type is overwritten to
VIR_DOMAIN_SECLABEL_DEFAULT because the first thing that the
helper function does is to set variable that holds the result to
zero. Switch to virXMLPropEnumDefault() to restore the previous
behavior.

Fixes: f7ff8556ad9ba8d81408e31649071941a6a849a3
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/conf/domain_conf.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 84425ff39d..b1ee519ff6 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7076,14 +7076,13 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
     if (!(seclabel = virSecurityLabelDefNew(model)))
         return NULL;
 
-    /* set default value */
-    seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC;
-
-    if (virXMLPropEnum(ctxt->node, "type",
-                       virDomainSeclabelTypeFromString,
-                       VIR_XML_PROP_NONZERO,
-                       &seclabel->type) < 0)
+    if (virXMLPropEnumDefault(ctxt->node, "type",
+                              virDomainSeclabelTypeFromString,
+                              VIR_XML_PROP_NONZERO,
+                              &seclabel->type,
+                              VIR_DOMAIN_SECLABEL_DYNAMIC) < 0) {
         return NULL;
+    }
 
     if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC ||
         seclabel->type == VIR_DOMAIN_SECLABEL_NONE)
-- 
2.52.0

On Thu, Mar 26, 2026 at 12:51:23 +0100, Michal Privoznik via Devel wrote:
> From: Michal Privoznik <mprivozn@redhat.com>
> 
> Prior to v7.10.0-rc1~26 seclabels defaulted to

Given that it's ~5 years since I broke this ...

> VIR_DOMAIN_SECLABEL_DYNAMIC (type='dynamic'). But after switching
> the parser to virXMLPropEnum() the type is overwritten to
> VIR_DOMAIN_SECLABEL_DEFAULT because the first thing that the
> helper function does is to set variable that holds the result to
> zero. Switch to virXMLPropEnumDefault() to restore the previous
> behavior.

.... I wonder what the implications actually are (I didn't check). The
commit message explains what happened w.r.t the behaviour of
virXMLPropEnum() but don't detail what the effect of the change was so I
wonder why we didn't see any problem with this or why we now want to
change it back besides "it was like that before".

Did you happen to figure out what the change actually impacted?

On 3/31/26 14:57, Peter Krempa wrote:
> On Thu, Mar 26, 2026 at 12:51:23 +0100, Michal Privoznik via Devel wrote:
>> From: Michal Privoznik <mprivozn@redhat.com>
>>
>> Prior to v7.10.0-rc1~26 seclabels defaulted to
> 
> Given that it's ~5 years since I broke this ...
> 
>> VIR_DOMAIN_SECLABEL_DYNAMIC (type='dynamic'). But after switching
>> the parser to virXMLPropEnum() the type is overwritten to
>> VIR_DOMAIN_SECLABEL_DEFAULT because the first thing that the
>> helper function does is to set variable that holds the result to
>> zero. Switch to virXMLPropEnumDefault() to restore the previous
>> behavior.
> 
> .... I wonder what the implications actually are (I didn't check). The
> commit message explains what happened w.r.t the behaviour of
> virXMLPropEnum() but don't detail what the effect of the change was so I
> wonder why we didn't see any problem with this or why we now want to
> change it back besides "it was like that before".
> 
> Did you happen to figure out what the change actually impacted?
> 

Yeah, I was thinking about that too. But I think we're on the safe side
because of extensive checks the function does. I mean, the moment you
want relabel='yes' you also need either type='static' or type='dynamic'.
These then get properly formatted into saved domain XML.

There's only one corner case (through which I encountered this bug). In
the original bug report, Rich specified "<seclabel model="selinux"
relabel="no"/>". I did not realize it was disk's seclabel and thought
it's the top level domain seclabel. Now, our RNG doesn't consider this
valid, nevertheless, ignoring XML validation error, I've realized the
seclabel was parsed and stored in virDomainDef, yet not formatted back
(thanks to short circuit at the very beginning of
virSecurityLabelDefFormat()).

Long story short, the only case in which we can have "troubles" is when
users do NOT want us to use certain secdriver AND they pass incomplete
XML that our RNG rejects.

Michal