7:53 a.m.
Commit 6992994 started filling the listen attribute
of the parent <graphics> elements from type='network' listens.
When this XML is passed to UpdateDevice, parsing fails:
XML error: graphics listen attribute 10.20.30.40 must match
address attribute of first listen element (found none)
Ignore the address in the parent <graphics> attribute
when no type='address' listens are found,
the same we ignore the address for the <listen> subelements
when parsing inactive XML.
---
src/conf/domain_conf.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d95dd3e..d458195 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9614,12 +9614,16 @@ virDomainGraphicsDefParseXML(xmlNodePtr node,
break;
}
}
- if (!matched) {
+ if (found && !matched) {
virReportError(VIR_ERR_XML_ERROR,
_("graphics listen attribute %s must match
address "
"attribute of first listen element (found
%s)"),
- listenAddr, found ? found : "none");
+ listenAddr, found);
goto error;
+ } else if (!found && !matched) {
+ /* quietly ignore listen address if none of the listens
+ * are of type address */
+ VIR_FREE(listenAddr);
}
}
}
--
2.0.5
8:57 a.m.
On 02/26/2015 08:53 AM, Ján Tomko wrote:
Commit 6992994 started filling the listen attribute
of the parent <graphics> elements from type='network' listens.
When this XML is passed to UpdateDevice, parsing fails:
XML error: graphics listen attribute 10.20.30.40 must match
address attribute of first listen element (found none)
Note that the listen attribute of <graphics> won't be filled in if you
request the *inactive* xml, and so there will be no error when it is fed
back to updatedevice. I can't think of any examples right now, but have
a very definite memory that there are several items in the config like
this - if you feed the status xml output back to update/define "you're
gonna have a bad time". That's why virsh edit asks for the INACTIVE xml.
Did you see this when trying to do an update-device manually, or as the
result of some management application that has forgotten to add the
INACTIVE flag to its request for XML?
If the latter, then I guess I can live with ignoring the error in order
to preserve backward compatibility with the broken application.
Reluctant ACK.
Ignore the address in the parent <graphics> attribute
when no type='address' listens are found,
the same we ignore the address for the <listen> subelements
when parsing inactive XML.
---
src/conf/domain_conf.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d95dd3e..d458195 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9614,12 +9614,16 @@ virDomainGraphicsDefParseXML(xmlNodePtr node,
break;
}
}
- if (!matched) {
+ if (found && !matched) {
virReportError(VIR_ERR_XML_ERROR,
_("graphics listen attribute %s must match
address "
"attribute of first listen element (found
%s)"),
- listenAddr, found ? found : "none");
+ listenAddr, found);
goto error;
+ } else if (!found && !matched) {
You are guaranteed that if found == NULL, then match == false, so the
2nd term is redundant.
(you never get into this code if listenAddr == NULL, so unless found !=
NULL the comparison that results in setting match = true will never be
true).
+ /* quietly ignore listen address if none of the
listens
+ * are of type address */
+ VIR_FREE(listenAddr);
}
}
}
9:11 a.m.
Hi,
----- Original Message -----
From: "Laine Stump" <laine(a)laine.org>
To: libvir-list(a)redhat.com
Cc: "Ján Tomko" <jtomko(a)redhat.com>, fromani(a)redhat.com
Sent: Thursday, February 26, 2015 3:57:22 PM
Subject: Re: [PATCH] Ignore listen attribute of <graphics> for type network
listens
On 02/26/2015 08:53 AM, Ján Tomko wrote:
> Commit 6992994 started filling the listen attribute
> of the parent <graphics> elements from type='network' listens.
>
> When this XML is passed to UpdateDevice, parsing fails:
> XML error: graphics listen attribute 10.20.30.40 must match
> address attribute of first listen element (found none)
Note that the listen attribute of <graphics> won't be filled in if you
request the *inactive* xml, and so there will be no error when it is fed
back to updatedevice. I can't think of any examples right now, but have
a very definite memory that there are several items in the config like
this - if you feed the status xml output back to update/define "you're
gonna have a bad time". That's why virsh edit asks for the INACTIVE xml.
Did you see this when trying to do an update-device manually, or as the
result of some management application that has forgotten to add the
INACTIVE flag to its request for XML?
I don't know about Jan, but I experienced this failure myself with a quite
recent libvirt snapshot (around Feb 20, so with the aforementioned commit)
using oVirt/VDSM.
When we connect to display through oVirt, before to fire up we set up the
password through virDomainUpdateDeviceFlags. This fails for me on libvirt 1.2.13
from master with Commit 6992994
Let me summarize the flow:
we create and feed createXML with:
<graphics autoport="yes" keymap="en-us"
passwd="*****" passwdValidTo="1970-01-01T00:00:01" port="-1"
tlsPort="-1" type="spice">
<channel mode="secure" name="main"/>
<channel mode="secure" name="inputs"/>
<channel mode="secure" name="cursor"/>
<channel mode="secure"
name="playback"/>
<channel mode="secure" name="record"/>
<channel mode="secure" name="display"/>
<channel mode="secure"
name="usbredir"/>
<channel mode="secure"
name="smartcard"/>
<listen network="vdsm-ovirtmgmt"
type="network"/>
</graphics>
we get back using domainGetXmlDesc:
<graphics type='spice' tlsPort='5900' autoport='yes'
listen='192.168.1.51' keymap='en-us' passwd='*****'
passwdValidTo='1970-01-01T00:00:01'>
<listen type='network' address='192.168.1.51'
network='vdsm-ovirtmgmt'/>
<channel name='main' mode='secure'/>
<channel name='display' mode='secure'/>
<channel name='inputs' mode='secure'/>
<channel name='cursor' mode='secure'/>
<channel name='playback' mode='secure'/>
<channel name='record' mode='secure'/>
<channel name='smartcard' mode='secure'/>
<channel name='usbredir' mode='secure'/>
</graphics>
we try to feed using virDomainUpdateDeviceFlags:
<graphics autoport="yes" connected="disconnect"
keymap="en-us" listen="192.168.1.51" passwd="SOMETHING"
passwdValidTo="2015-02-26T09:56:40" tlsPort="5900"
type="spice">
<listen address="192.168.1.51" network="vdsm-ovirtmgmt"
type="network"/>
<channel mode="secure" name="main"/>
<channel mode="secure" name="display"/>
<channel mode="secure" name="inputs"/>
<channel mode="secure" name="cursor"/>
<channel mode="secure" name="playback"/>
<channel mode="secure" name="record"/>
<channel mode="secure" name="smartcard"/>
<channel mode="secure" name="usbredir"/>
</graphics>
it chokes out with
File "/usr/lib64/python2.7/site-packages/libvirt.py", line 2647, in
updateDeviceFlags
if ret == -1: raise libvirtError ('virDomainUpdateDeviceFlags() failed',
dom=self)
libvirtError: XML error: graphics listen attribute 192.168.1.51 must match address
attribute of first listen element (found none)
We use flags=0 here:
self._dom.updateDeviceFlags(graphics.toxml(), 0)
(_dom is a virDomain, graphics.toxml() produces the above)
Now, I understand that here we should use a proper flag, but I'm
confused on which should we use. The docs
http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainUpdateDevice...
doesn't seem to match with I just learned from your email:
http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainDeviceModify...
Which one do we need here? VIR_DOMAIN_DEVICE_MODIFY_CONFIG?
If the latter, then I guess I can live with ignoring the error in
order
to preserve backward compatibility with the broken application.
Reluctant ACK.
Well, I appreciate the backward compatibility effort, but I also
definitely want to fix the oVirt code! :)
Thanks,
--
Francesco Romani
RedHat Engineering Virtualization R & D
Phone: 8261328
IRC: fromani
9:28 a.m.
On 02/26/2015 10:11 AM, Francesco Romani wrote:
Hi,
----- Original Message -----
> From: "Laine Stump" <laine(a)laine.org>
> To: libvir-list(a)redhat.com
> Cc: "Ján Tomko" <jtomko(a)redhat.com>, fromani(a)redhat.com
> Sent: Thursday, February 26, 2015 3:57:22 PM
> Subject: Re: [PATCH] Ignore listen attribute of <graphics> for type network
listens
>
> On 02/26/2015 08:53 AM, Ján Tomko wrote:
>> Commit 6992994 started filling the listen attribute
>> of the parent <graphics> elements from type='network' listens.
>>
>> When this XML is passed to UpdateDevice, parsing fails:
>> XML error: graphics listen attribute 10.20.30.40 must match
>> address attribute of first listen element (found none)
>
> Note that the listen attribute of <graphics> won't be filled in if you
> request the *inactive* xml, and so there will be no error when it is fed
> back to updatedevice. I can't think of any examples right now, but have
> a very definite memory that there are several items in the config like
> this - if you feed the status xml output back to update/define "you're
> gonna have a bad time". That's why virsh edit asks for the INACTIVE xml.
>
> Did you see this when trying to do an update-device manually, or as the
> result of some management application that has forgotten to add the
> INACTIVE flag to its request for XML?
I don't know about Jan, but I experienced this failure myself with a quite
recent libvirt snapshot (around Feb 20, so with the aforementioned commit)
using oVirt/VDSM.
When we connect to display through oVirt, before to fire up we set up the
password through virDomainUpdateDeviceFlags. This fails for me on libvirt 1.2.13
from master with Commit 6992994
Let me summarize the flow:
we create and feed createXML with:
<graphics autoport="yes" keymap="en-us"
passwd="*****" passwdValidTo="1970-01-01T00:00:01" port="-1"
tlsPort="-1" type="spice">
<channel mode="secure" name="main"/>
<channel mode="secure" name="inputs"/>
<channel mode="secure" name="cursor"/>
<channel mode="secure"
name="playback"/>
<channel mode="secure" name="record"/>
<channel mode="secure"
name="display"/>
<channel mode="secure"
name="usbredir"/>
<channel mode="secure"
name="smartcard"/>
<listen network="vdsm-ovirtmgmt"
type="network"/>
</graphics>
we get back using domainGetXmlDesc:
This is where the error is caused - you need to add the inactive flag to
this call
<graphics type='spice' tlsPort='5900' autoport='yes'
listen='192.168.1.51' keymap='en-us' passwd='*****'
passwdValidTo='1970-01-01T00:00:01'>
<listen type='network' address='192.168.1.51'
network='vdsm-ovirtmgmt'/>
<channel name='main' mode='secure'/>
<channel name='display' mode='secure'/>
<channel name='inputs' mode='secure'/>
<channel name='cursor' mode='secure'/>
<channel name='playback' mode='secure'/>
<channel name='record' mode='secure'/>
<channel name='smartcard' mode='secure'/>
<channel name='usbredir' mode='secure'/>
</graphics>
we try to feed using virDomainUpdateDeviceFlags:
<graphics autoport="yes" connected="disconnect"
keymap="en-us" listen="192.168.1.51" passwd="SOMETHING"
passwdValidTo="2015-02-26T09:56:40" tlsPort="5900"
type="spice">
<listen address="192.168.1.51" network="vdsm-ovirtmgmt"
type="network"/>
<channel mode="secure" name="main"/>
<channel mode="secure" name="display"/>
<channel mode="secure" name="inputs"/>
<channel mode="secure" name="cursor"/>
<channel mode="secure" name="playback"/>
<channel mode="secure" name="record"/>
<channel mode="secure" name="smartcard"/>
<channel mode="secure" name="usbredir"/>
</graphics>
it chokes out with
File "/usr/lib64/python2.7/site-packages/libvirt.py", line 2647, in
updateDeviceFlags
if ret == -1: raise libvirtError ('virDomainUpdateDeviceFlags() failed',
dom=self)
libvirtError: XML error: graphics listen attribute 192.168.1.51 must match address
attribute of first listen element (found none)
We use flags=0 here:
self._dom.updateDeviceFlags(graphics.toxml(), 0)
(_dom is a virDomain, graphics.toxml() produces the above)
Now, I understand that here we should use a proper flag, but I'm
confused on which should we use. The docs
http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainUpdateDevice...
doesn't seem to match with I just learned from your email:
http://libvirt.org/html/libvirt-libvirt-domain.html#virDomainDeviceModify...
Which one do we need here? VIR_DOMAIN_DEVICE_MODIFY_CONFIG?
It's not the updateDevice flags that need to be changed. You need to add
VIR_DOMAIN_XML_INACTIVE to the flags for domainXmlGetDesc.
> If the latter, then I guess I can live with ignoring the error in order
> to preserve backward compatibility with the broken application.
> Reluctant ACK.
Well, I appreciate the backward compatibility effort, but I also
definitely want to fix the oVirt code! :)
Thanks,
10:10 a.m.
----- Original Message -----
From: "Laine Stump" <laine(a)laine.org>
To: libvir-list(a)redhat.com
Cc: "Francesco Romani" <fromani(a)redhat.com>, "Ján Tomko"
<jtomko(a)redhat.com>
Sent: Thursday, February 26, 2015 4:28:13 PM
Subject: Re: [libvirt] [PATCH] Ignore listen attribute of <graphics> for type
network listens
On 02/26/2015 10:11 AM, Francesco Romani wrote:
> Hi,
[...]
> When we connect to display through oVirt, before to fire up we
set up the
> password through virDomainUpdateDeviceFlags. This fails for me on libvirt
> 1.2.13
> from master with Commit 6992994
>
> Let me summarize the flow:
>
> we create and feed createXML with:
>
> <graphics autoport="yes" keymap="en-us"
passwd="*****"
> passwdValidTo="1970-01-01T00:00:01" port="-1"
tlsPort="-1"
> type="spice">
> <channel mode="secure"
name="main"/>
> <channel mode="secure"
name="inputs"/>
> <channel mode="secure"
name="cursor"/>
> <channel mode="secure"
name="playback"/>
> <channel mode="secure"
name="record"/>
> <channel mode="secure"
name="display"/>
> <channel mode="secure"
name="usbredir"/>
> <channel mode="secure"
name="smartcard"/>
> <listen network="vdsm-ovirtmgmt"
type="network"/>
> </graphics>
>
> we get back using domainGetXmlDesc:
This is where the error is caused - you need to add the inactive flag to
this call
Sorry, I forgot to point out that oVirt uses transient domains, so to use the
inactive flag is surprising to me. I'm not sure that will work for us.
I need to check the docs and play with it for a bit.
Thanks,
--
Francesco Romani
RedHat Engineering Virtualization R & D
Phone: 8261328
IRC: fromani
10:44 a.m.
On Thu, Feb 26, 2015 at 11:10:06AM -0500, Francesco Romani wrote:
----- Original Message -----
> From: "Laine Stump" <laine(a)laine.org>
> To: libvir-list(a)redhat.com
> Cc: "Francesco Romani" <fromani(a)redhat.com>, "Ján Tomko"
<jtomko(a)redhat.com>
> Sent: Thursday, February 26, 2015 4:28:13 PM
> Subject: Re: [libvirt] [PATCH] Ignore listen attribute of <graphics> for type
network listens
>
> On 02/26/2015 10:11 AM, Francesco Romani wrote:
> > Hi,
[...]
> > When we connect to display through oVirt, before to fire up we set up the
> > password through virDomainUpdateDeviceFlags. This fails for me on libvirt
> > 1.2.13
> > from master with Commit 6992994
> >
> > Let me summarize the flow:
> >
> > we create and feed createXML with:
> >
> > <graphics autoport="yes" keymap="en-us"
passwd="*****"
> > passwdValidTo="1970-01-01T00:00:01"
port="-1" tlsPort="-1"
> > type="spice">
> > <channel mode="secure"
name="main"/>
> > <channel mode="secure"
name="inputs"/>
> > <channel mode="secure"
name="cursor"/>
> > <channel mode="secure"
name="playback"/>
> > <channel mode="secure"
name="record"/>
> > <channel mode="secure"
name="display"/>
> > <channel mode="secure"
name="usbredir"/>
> > <channel mode="secure"
name="smartcard"/>
> > <listen network="vdsm-ovirtmgmt"
type="network"/>
> > </graphics>
> >
> > we get back using domainGetXmlDesc:
>
> This is where the error is caused - you need to add the inactive flag to
> this call
Sorry, I forgot to point out that oVirt uses transient domains, so to use the
inactive flag is surprising to me. I'm not sure that will work for us.
I need to check the docs and play with it for a bit.
The VIR_DOMAIN_XML_INACTIVE flag behaves differently for
persistent and transient domains.
In both cases, only the attributes that make sense in an inactive
domain definition are formatted. But for persistent domain, we format
the persistent definition, not the live definition, so hotplugged
devices will not appear there.
For transient domains, the live definition is formatted, just not with
all the attributes.
Jan
9:29 a.m.
On Thu, Feb 26, 2015 at 09:57:22AM -0500, Laine Stump wrote:
On 02/26/2015 08:53 AM, Ján Tomko wrote:
> Commit 6992994 started filling the listen attribute
> of the parent <graphics> elements from type='network' listens.
>
> When this XML is passed to UpdateDevice, parsing fails:
> XML error: graphics listen attribute 10.20.30.40 must match
> address attribute of first listen element (found none)
Note that the listen attribute of <graphics> won't be filled in if you
request the *inactive* xml, and so there will be no error when it is fed
back to updatedevice. I can't think of any examples right now, but have
a very definite memory that there are several items in the config like
this - if you feed the status xml output back to update/define "you're
gonna have a bad time". That's why virsh edit asks for the INACTIVE xml.
Did you see this when trying to do an update-device manually, or as the
result of some management application that has forgotten to add the
INACTIVE flag to its request for XML?
If the latter, then I guess I can live with ignoring the error in order
to preserve backward compatibility with the broken application.
Reluctant ACK.
I think we shouldn't fail parsing live XML when we were the ones who
generated it. Take a look at device aliases for example. We parse
them, but users aren't allowed to set them, so we don't parse them
when we are parsing inactive XML. However, if I remember correctly,
we must be parsing them when parsing e.g. status XML. So what if this
patch is modified so it behaves depending on flags?
> Ignore the address in the parent <graphics> attribute
> when no type='address' listens are found,
> the same we ignore the address for the <listen> subelements
> when parsing inactive XML.
> ---
> src/conf/domain_conf.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d95dd3e..d458195 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -9614,12 +9614,16 @@ virDomainGraphicsDefParseXML(xmlNodePtr node,
> break;
> }
> }
> - if (!matched) {
> + if (found && !matched) {
> virReportError(VIR_ERR_XML_ERROR,
> _("graphics listen attribute %s must match
address "
> "attribute of first listen element (found
%s)"),
> - listenAddr, found ? found : "none");
> + listenAddr, found);
> goto error;
> + } else if (!found && !matched) {
You are guaranteed that if found == NULL, then match == false, so the
2nd term is redundant.
(you never get into this code if listenAddr == NULL, so unless found !=
NULL the comparison that results in setting match = true will never be
true).
> + /* quietly ignore listen address if none of the listens
> + * are of type address */
> + VIR_FREE(listenAddr);
> }
> }
> }
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
10:39 a.m.
On Thu, Feb 26, 2015 at 04:29:53PM +0100, Martin Kletzander wrote:
On Thu, Feb 26, 2015 at 09:57:22AM -0500, Laine Stump wrote:
>On 02/26/2015 08:53 AM, Ján Tomko wrote:
>> Commit 6992994 started filling the listen attribute
>> of the parent <graphics> elements from type='network' listens.
>>
>> When this XML is passed to UpdateDevice, parsing fails:
>> XML error: graphics listen attribute 10.20.30.40 must match
>> address attribute of first listen element (found none)
>
>
>Note that the listen attribute of <graphics> won't be filled in if you
>request the *inactive* xml, and so there will be no error when it is fed
>back to updatedevice. I can't think of any examples right now, but have
>a very definite memory that there are several items in the config like
>this - if you feed the status xml output back to update/define "you're
>gonna have a bad time". That's why virsh edit asks for the INACTIVE xml.
>
>Did you see this when trying to do an update-device manually, or as the
>result of some management application that has forgotten to add the
>INACTIVE flag to its request for XML?
>
>If the latter, then I guess I can live with ignoring the error in order
>to preserve backward compatibility with the broken application.
>Reluctant ACK.
>
Yes, this was a workaround for vdsm.
But now I notice that since the check is only done against type='address'
listens, restarting libvirtd will lose any domain started with
listen type='network', because libvirt fails to parse its own live XML.
I'll wait with pushing the patch just in case someone else wants to
chime in.
I think we shouldn't fail parsing live XML when we were the ones who
generated it. Take a look at device aliases for example. We parse
them, but users aren't allowed to set them, so we don't parse them
when we are parsing inactive XML. However, if I remember correctly,
we must be parsing them when parsing e.g. status XML. So what if this
patch is modified so it behaves depending on flags?
With the INACTIVE flag (which is the case here in UpdateDevice), the
listen addresses for <listen type='network'> subelements are already
ignored.
The INACTIVE flag is used for almost every domain definition parsing (at
least in the QEMU driver - the only place with live XML parsing I found
is the driver initialization).
I can fix the verification to work against LISTEN_TYPE_NETWORK addresses
too for live XML, but since:
1) we only parse live XML on driver initialization, so it should be
generated by libvirt
2) even if we parsed a wrong listen address from there, it won't be used -
virDomainGraphicsDefFormat gets it from the listens array.
I chose the simpler patch with less breakage potential, since we're past
rc2.
Jan
11:40 a.m.
On 02/26/2015 11:39 AM, Ján Tomko wrote:
On Thu, Feb 26, 2015 at 04:29:53PM +0100, Martin Kletzander wrote:
> On Thu, Feb 26, 2015 at 09:57:22AM -0500, Laine Stump wrote:
>> On 02/26/2015 08:53 AM, Ján Tomko wrote:
>>> Commit 6992994 started filling the listen attribute
>>> of the parent <graphics> elements from type='network' listens.
>>>
>>> When this XML is passed to UpdateDevice, parsing fails:
>>> XML error: graphics listen attribute 10.20.30.40 must match
>>> address attribute of first listen element (found none)
>>
>> Note that the listen attribute of <graphics> won't be filled in if you
>> request the *inactive* xml, and so there will be no error when it is fed
>> back to updatedevice. I can't think of any examples right now, but have
>> a very definite memory that there are several items in the config like
>> this - if you feed the status xml output back to update/define "you're
>> gonna have a bad time". That's why virsh edit asks for the INACTIVE
xml.
>>
>> Did you see this when trying to do an update-device manually, or as the
>> result of some management application that has forgotten to add the
>> INACTIVE flag to its request for XML?
>>
>> If the latter, then I guess I can live with ignoring the error in order
>> to preserve backward compatibility with the broken application.
>> Reluctant ACK.
>>
Yes, this was a workaround for vdsm.
But now I notice that since the check is only done against type='address'
listens, restarting libvirtd will lose any domain started with
listen type='network', because libvirt fails to parse its own live XML.
Ah yes. Less reluctance (*much* less) in the ACK then. I actually ran
into something similar with networks just a week or two ago - commit
8f8e581a - and had to do the same thing.
"If you live in glass houses, don't throw stones" :-)
I'll wait with pushing the patch just in case someone else wants to
chime in.
> I think we shouldn't fail parsing live XML when we were the ones who
> generated it. Take a look at device aliases for example. We parse
> them, but users aren't allowed to set them, so we don't parse them
> when we are parsing inactive XML. However, if I remember correctly,
> we must be parsing them when parsing e.g. status XML. So what if this
> patch is modified so it behaves depending on flags?
>
With the INACTIVE flag (which is the case here in UpdateDevice), the
listen addresses for <listen type='network'> subelements are already
ignored.
... but we can't ignore the listen attribute in <graphics> because in
older XML (prior to the introduction of the <listen> subelement) that is
the only place to find the listen address.
You know, I hadn't even paid conscious attention to whether the INACTIVE
flag is set when calling parse functions before. Of course, in this case
it wouldn't make a difference, since we not only have to account for
reading our own status XML, but also for reading the XML sent by
"broken" applications.
The INACTIVE flag is used for almost every domain definition parsing (at
least in the QEMU driver - the only place with live XML parsing I found
is the driver initialization).
I can fix the verification to work against LISTEN_TYPE_NETWORK addresses
too for live XML, but since:
1) we only parse live XML on driver initialization, so it should be
generated by libvirt
2) even if we parsed a wrong listen address from there, it won't be used -
virDomainGraphicsDefFormat gets it from the listens array.
I chose the simpler patch with less breakage potential, since we're past
rc2.
Agreed.
1:55 a.m.
On Thu, Feb 26, 2015 at 12:40:43PM -0500, Laine Stump wrote:
On 02/26/2015 11:39 AM, Ján Tomko wrote:
> On Thu, Feb 26, 2015 at 04:29:53PM +0100, Martin Kletzander wrote:
>> On Thu, Feb 26, 2015 at 09:57:22AM -0500, Laine Stump wrote:
>>> On 02/26/2015 08:53 AM, Ján Tomko wrote:
>>>> Commit 6992994 started filling the listen attribute
>>>> of the parent <graphics> elements from type='network'
listens.
>>>>
>>>> When this XML is passed to UpdateDevice, parsing fails:
>>>> XML error: graphics listen attribute 10.20.30.40 must match
>>>> address attribute of first listen element (found none)
>>>
>>> Note that the listen attribute of <graphics> won't be filled in if
you
>>> request the *inactive* xml, and so there will be no error when it is fed
>>> back to updatedevice. I can't think of any examples right now, but have
>>> a very definite memory that there are several items in the config like
>>> this - if you feed the status xml output back to update/define
"you're
>>> gonna have a bad time". That's why virsh edit asks for the INACTIVE
xml.
>>>
>>> Did you see this when trying to do an update-device manually, or as the
>>> result of some management application that has forgotten to add the
>>> INACTIVE flag to its request for XML?
>>>
>>> If the latter, then I guess I can live with ignoring the error in order
>>> to preserve backward compatibility with the broken application.
>>> Reluctant ACK.
>>>
> Yes, this was a workaround for vdsm.
>
> But now I notice that since the check is only done against type='address'
> listens, restarting libvirtd will lose any domain started with
> listen type='network', because libvirt fails to parse its own live XML.
Ah yes. Less reluctance (*much* less) in the ACK then. I actually ran
into something similar with networks just a week or two ago - commit
8f8e581a - and had to do the same thing.
I have pushed the patch now.
Jan
3557
days inactive
3558
days old
9 comments
4 participants
participants (4)
-
Francesco Romani -
Ján Tomko -
Laine Stump -
Martin Kletzander