[libvirt] [PATCH] Fix libvirtd restart for domains with PCI passthrough devices
When libvirtd shuts down, it places a <state/> tag in the XML state file it writes out for guests with PCI passthrough devices. For devices that are attached at bootup time, the state tag is empty. However, at libvirtd startup time, it ignores anything with a <state/> tag in the XML, effectively hiding the guest. I can think of at least 3 ways to fix this: 1) Don't throw an error on "unknown" tags in virDomainHostdevSubsysPciDefParseXML(). 2) Have virDomainLoadAllConfigs() pass the VIR_DOMAIN_XML_INTERNAL_STATUS flag when parsing the domain XML. 3) Remove the check for VIR_DOMAIN_XML_INTERNAL_STATUS when parsing the XML. I chose approach 3). My reasoning for this is that the <state> tag is a legitimate part of the XML, so we should always offer to parse it. This fixes the problem with reconnecting to domains that have PCI passthrough devices. Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/conf/domain_conf.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 74c2337..595c46c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2876,7 +2876,7 @@ static int virDomainHostdevSubsysPciDefParseXML(virConnectPtr conn, const xmlNodePtr node, virDomainHostdevDefPtr def, - int flags) { + int flags ATTRIBUTE_UNUSED) { int ret = -1; xmlNodePtr cur; @@ -2890,8 +2890,7 @@ virDomainHostdevSubsysPciDefParseXML(virConnectPtr conn, if (virDomainDevicePCIAddressParseXML(conn, cur, addr) < 0) goto out; - } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && - xmlStrEqual(cur->name, BAD_CAST "state")) { + } else if (xmlStrEqual(cur->name, BAD_CAST "state")) { /* Legacy back-compat. Don't add any more attributes here */ char *devaddr = virXMLPropString(cur, "devaddr"); if (devaddr && -- 1.6.6
On Fri, Jan 22, 2010 at 11:40:39AM -0500, Chris Lalancette wrote:
When libvirtd shuts down, it places a <state/> tag in the XML state file it writes out for guests with PCI passthrough devices. For devices that are attached at bootup time, the state tag is empty. However, at libvirtd startup time, it ignores anything with a <state/> tag in the XML, effectively hiding the guest.
I can think of at least 3 ways to fix this:
1) Don't throw an error on "unknown" tags in virDomainHostdevSubsysPciDefParseXML(). 2) Have virDomainLoadAllConfigs() pass the VIR_DOMAIN_XML_INTERNAL_STATUS flag when parsing the domain XML. 3) Remove the check for VIR_DOMAIN_XML_INTERNAL_STATUS when parsing the XML.
I chose approach 3). My reasoning for this is that the <state> tag is a legitimate part of the XML, so we should always offer to parse it. This fixes the problem with reconnecting to domains that have PCI passthrough devices.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/conf/domain_conf.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 74c2337..595c46c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2876,7 +2876,7 @@ static int virDomainHostdevSubsysPciDefParseXML(virConnectPtr conn, const xmlNodePtr node, virDomainHostdevDefPtr def, - int flags) { + int flags ATTRIBUTE_UNUSED) {
int ret = -1; xmlNodePtr cur; @@ -2890,8 +2890,7 @@ virDomainHostdevSubsysPciDefParseXML(virConnectPtr conn,
if (virDomainDevicePCIAddressParseXML(conn, cur, addr) < 0) goto out; - } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && - xmlStrEqual(cur->name, BAD_CAST "state")) { + } else if (xmlStrEqual(cur->name, BAD_CAST "state")) { /* Legacy back-compat. Don't add any more attributes here */ char *devaddr = virXMLPropString(cur, "devaddr"); if (devaddr &&
I'm not sure, it feels like 2/ is the most specific change possible to this problem, but 3/ makes sense too, I just don't know if there is any side effect. So ACK to this patch but let's keep an eye on testing in the next week, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Fri, Jan 22, 2010 at 06:19:14PM +0100, Daniel Veillard wrote:
On Fri, Jan 22, 2010 at 11:40:39AM -0500, Chris Lalancette wrote:
When libvirtd shuts down, it places a <state/> tag in the XML state file it writes out for guests with PCI passthrough devices. For devices that are attached at bootup time, the state tag is empty. However, at libvirtd startup time, it ignores anything with a <state/> tag in the XML, effectively hiding the guest.
I can think of at least 3 ways to fix this:
1) Don't throw an error on "unknown" tags in virDomainHostdevSubsysPciDefParseXML(). 2) Have virDomainLoadAllConfigs() pass the VIR_DOMAIN_XML_INTERNAL_STATUS flag when parsing the domain XML. 3) Remove the check for VIR_DOMAIN_XML_INTERNAL_STATUS when parsing the XML.
I chose approach 3). My reasoning for this is that the <state> tag is a legitimate part of the XML, so we should always offer to parse it. This fixes the problem with reconnecting to domains that have PCI passthrough devices.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/conf/domain_conf.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 74c2337..595c46c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2876,7 +2876,7 @@ static int virDomainHostdevSubsysPciDefParseXML(virConnectPtr conn, const xmlNodePtr node, virDomainHostdevDefPtr def, - int flags) { + int flags ATTRIBUTE_UNUSED) {
int ret = -1; xmlNodePtr cur; @@ -2890,8 +2890,7 @@ virDomainHostdevSubsysPciDefParseXML(virConnectPtr conn,
if (virDomainDevicePCIAddressParseXML(conn, cur, addr) < 0) goto out; - } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && - xmlStrEqual(cur->name, BAD_CAST "state")) { + } else if (xmlStrEqual(cur->name, BAD_CAST "state")) { /* Legacy back-compat. Don't add any more attributes here */ char *devaddr = virXMLPropString(cur, "devaddr"); if (devaddr &&
I'm not sure, it feels like 2/ is the most specific change possible to this problem, but 3/ makes sense too, I just don't know if there is any side effect.
So ACK to this patch but let's keep an eye on testing in the next week,
I pushed it, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Fri, Jan 22, 2010 at 11:40:39AM -0500, Chris Lalancette wrote:
When libvirtd shuts down, it places a <state/> tag in the XML state file it writes out for guests with PCI passthrough devices. For devices that are attached at bootup time, the state tag is empty. However, at libvirtd startup time, it ignores anything with a <state/> tag in the XML, effectively hiding the guest.
This description doesn't make any sense to me. At libvirtd startup - Running guest state is loaded from /var/lib/libvirt/qemu/$NAME.xml using flags=VIR_DOMAIN_XML_INTERNAL_STATUS - Persistent configs are loaded from /etc/libvirt/qemu/$NAME.xml using flags=0 The <state/> tags are only ever written into the configs that are in /var/lib/libvirt/qemu/, so flags=VIR_DOMAIN_XML_INTERNAL_STATUS means the <state/> element is read at startup. Regardless, simply ignoring the <state/> tag won't cause the guest to be hidden.
I can think of at least 3 ways to fix this:
1) Don't throw an error on "unknown" tags in virDomainHostdevSubsysPciDefParseXML(). 2) Have virDomainLoadAllConfigs() pass the VIR_DOMAIN_XML_INTERNAL_STATUS flag when parsing the domain XML. 3) Remove the check for VIR_DOMAIN_XML_INTERNAL_STATUS when parsing the XML.
I chose approach 3). My reasoning for this is that the <state> tag is a legitimate part of the XML, so we should always offer to parse it. This fixes the problem with reconnecting to domains that have PCI passthrough devices.
I don't think there's a clear enough description of the original problem to come up with a fix here.
Signed-off-by: Chris Lalancette <clalance@redhat.com> --- src/conf/domain_conf.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 74c2337..595c46c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2876,7 +2876,7 @@ static int virDomainHostdevSubsysPciDefParseXML(virConnectPtr conn, const xmlNodePtr node, virDomainHostdevDefPtr def, - int flags) { + int flags ATTRIBUTE_UNUSED) {
int ret = -1; xmlNodePtr cur; @@ -2890,8 +2890,7 @@ virDomainHostdevSubsysPciDefParseXML(virConnectPtr conn,
if (virDomainDevicePCIAddressParseXML(conn, cur, addr) < 0) goto out; - } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && - xmlStrEqual(cur->name, BAD_CAST "state")) { + } else if (xmlStrEqual(cur->name, BAD_CAST "state")) { /* Legacy back-compat. Don't add any more attributes here */ char *devaddr = virXMLPropString(cur, "devaddr"); if (devaddr &&
NACK to this change. This lets end users inject bogus state into libvirtd. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (3)
-
Chris Lalancette -
Daniel P. Berrange -
Daniel Veillard