[libvirt] [PATCH] Do not search xenstore for disk device IDs
Disk devices can be referenced by name in Xen, e.g. when modifying their configuration or remvoving them. As such, don't search xenstore for a device ID corresponding to the disk device. Instead, search the disks contained in the domain definition and use the disk's target name if found. This approach allows removing a disk when domain is inactive. We obviously can't search xenstore when the domain is inactive. --- src/xen/xend_internal.c | 38 +++++++++++++++++++++++--------------- 1 files changed, 23 insertions(+), 15 deletions(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 9d95291..d0d32e2 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -93,6 +93,7 @@ xenDaemonFormatSxprOnePCI(virConnectPtr conn, static int virDomainXMLDevID(virDomainPtr domain, + virDomainDefPtr domDef, virDomainDeviceDefPtr dev, char *class, char *ref, @@ -4212,7 +4213,7 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, const char *xml, sexpr = virBufferContentAndReset(&buf); - if (virDomainXMLDevID(domain, dev, class, ref, sizeof(ref))) { + if (virDomainXMLDevID(domain, def, dev, class, ref, sizeof(ref))) { /* device doesn't exist, define it */ ret = xend_op(domain->conn, domain->name, "op", "device_create", "config", sexpr, NULL); @@ -4306,7 +4307,7 @@ xenDaemonDetachDeviceFlags(virDomainPtr domain, const char *xml, def, xml, VIR_DOMAIN_XML_INACTIVE))) goto cleanup; - if (virDomainXMLDevID(domain, dev, class, ref, sizeof(ref))) + if (virDomainXMLDevID(domain, def, dev, class, ref, sizeof(ref))) goto cleanup; if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { @@ -6048,6 +6049,7 @@ error: */ static int virDomainXMLDevID(virDomainPtr domain, + virDomainDefPtr domDef, virDomainDeviceDefPtr dev, char *class, char *ref, @@ -6056,27 +6058,33 @@ virDomainXMLDevID(virDomainPtr domain, xenUnifiedPrivatePtr priv = domain->conn->privateData; char *xref; char *tmp; + unsigned int i; + virDomainDiskDefPtr disk; if (dev->type == VIR_DOMAIN_DEVICE_DISK) { + if (dev->data.disk->dst == NULL) + return -1; if (dev->data.disk->driverName && STREQ(dev->data.disk->driverName, "tap")) strcpy(class, "tap"); else strcpy(class, "vbd"); - if (dev->data.disk->dst == NULL) - return -1; - xenUnifiedLock(priv); - xref = xenStoreDomainGetDiskID(domain->conn, domain->id, - dev->data.disk->dst); - xenUnifiedUnlock(priv); - if (xref == NULL) - return -1; - - tmp = virStrcpy(ref, xref, ref_len); - VIR_FREE(xref); - if (tmp == NULL) - return -1; + /* For disks, the device name can be used directly. + * If disk device exists in domain definintion, + * copy it to ref and return success. + */ + for (i = 0; i < domDef->ndisks; i++) { + disk = domDef->disks[i]; + if (STREQ(dev->data.disk->dst, disk->dst)) { + tmp = virStrcpy(ref, disk->dst, ref_len); + if (tmp == NULL) + return -1; + else + return 0; + } + } + return -1; } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { char mac[30]; virDomainNetDefPtr def = dev->data.net; -- 1.6.0.2
On Mon, Feb 22, 2010 at 02:50:07PM -0700, Jim Fehlig wrote:
Disk devices can be referenced by name in Xen, e.g. when modifying their configuration or remvoving them. As such, don't search xenstore for a device ID corresponding to the disk device. Instead, search the disks contained in the domain definition and use the disk's target name if found.
This approach allows removing a disk when domain is inactive. We obviously can't search xenstore when the domain is inactive.
This sounds reasonable, but I'm wondering if old XenD support the lookup-based on name ? Daniel
--- src/xen/xend_internal.c | 38 +++++++++++++++++++++++--------------- 1 files changed, 23 insertions(+), 15 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 9d95291..d0d32e2 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -93,6 +93,7 @@ xenDaemonFormatSxprOnePCI(virConnectPtr conn,
static int virDomainXMLDevID(virDomainPtr domain, + virDomainDefPtr domDef, virDomainDeviceDefPtr dev, char *class, char *ref, @@ -4212,7 +4213,7 @@ xenDaemonAttachDeviceFlags(virDomainPtr domain, const char *xml,
sexpr = virBufferContentAndReset(&buf);
- if (virDomainXMLDevID(domain, dev, class, ref, sizeof(ref))) { + if (virDomainXMLDevID(domain, def, dev, class, ref, sizeof(ref))) { /* device doesn't exist, define it */ ret = xend_op(domain->conn, domain->name, "op", "device_create", "config", sexpr, NULL); @@ -4306,7 +4307,7 @@ xenDaemonDetachDeviceFlags(virDomainPtr domain, const char *xml, def, xml, VIR_DOMAIN_XML_INACTIVE))) goto cleanup;
- if (virDomainXMLDevID(domain, dev, class, ref, sizeof(ref))) + if (virDomainXMLDevID(domain, def, dev, class, ref, sizeof(ref))) goto cleanup;
if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { @@ -6048,6 +6049,7 @@ error: */ static int virDomainXMLDevID(virDomainPtr domain, + virDomainDefPtr domDef, virDomainDeviceDefPtr dev, char *class, char *ref, @@ -6056,27 +6058,33 @@ virDomainXMLDevID(virDomainPtr domain, xenUnifiedPrivatePtr priv = domain->conn->privateData; char *xref; char *tmp; + unsigned int i; + virDomainDiskDefPtr disk;
if (dev->type == VIR_DOMAIN_DEVICE_DISK) { + if (dev->data.disk->dst == NULL) + return -1; if (dev->data.disk->driverName && STREQ(dev->data.disk->driverName, "tap")) strcpy(class, "tap"); else strcpy(class, "vbd");
- if (dev->data.disk->dst == NULL) - return -1; - xenUnifiedLock(priv); - xref = xenStoreDomainGetDiskID(domain->conn, domain->id, - dev->data.disk->dst); - xenUnifiedUnlock(priv); - if (xref == NULL) - return -1; - - tmp = virStrcpy(ref, xref, ref_len); - VIR_FREE(xref); - if (tmp == NULL) - return -1; + /* For disks, the device name can be used directly. + * If disk device exists in domain definintion, + * copy it to ref and return success. + */ + for (i = 0; i < domDef->ndisks; i++) { + disk = domDef->disks[i]; + if (STREQ(dev->data.disk->dst, disk->dst)) { + tmp = virStrcpy(ref, disk->dst, ref_len); + if (tmp == NULL) + return -1; + else + return 0; + } + } + return -1; } else if (dev->type == VIR_DOMAIN_DEVICE_NET) { char mac[30]; virDomainNetDefPtr def = dev->data.net; -- 1.6.0.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
On Mon, Feb 22, 2010 at 02:50:07PM -0700, Jim Fehlig wrote:
Disk devices can be referenced by name in Xen, e.g. when modifying their configuration or remvoving them. As such, don't search xenstore for a device ID corresponding to the disk device. Instead, search the disks contained in the domain definition and use the disk's target name if found.
This approach allows removing a disk when domain is inactive. We obviously can't search xenstore when the domain is inactive.
This sounds reasonable, but I'm wondering if old XenD support the lookup-based on name ?
Yes, I had thought about this as well. Guess it depends on how old we are talking about. It exists in Xen 3.1.x and newer. I wonder if we should even care about Xen 3.0.x. The upstream tree hasn't been touched in 3 years. Jim
On Tue, Feb 23, 2010 at 03:35:04PM -0700, Jim Fehlig wrote:
Daniel P. Berrange wrote:
On Mon, Feb 22, 2010 at 02:50:07PM -0700, Jim Fehlig wrote:
Disk devices can be referenced by name in Xen, e.g. when modifying their configuration or remvoving them. As such, don't search xenstore for a device ID corresponding to the disk device. Instead, search the disks contained in the domain definition and use the disk's target name if found.
This approach allows removing a disk when domain is inactive. We obviously can't search xenstore when the domain is inactive.
This sounds reasonable, but I'm wondering if old XenD support the lookup-based on name ?
Yes, I had thought about this as well. Guess it depends on how old we are talking about. It exists in Xen 3.1.x and newer. I wonder if we should even care about Xen 3.0.x. The upstream tree hasn't been touched in 3 years.
Well we still ship this, so it would be appreciated if it didn't broke if libvirt got updated for some reasons ;-) 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 Mon, Mar 01, 2010 at 02:45:41PM +0100, Daniel Veillard wrote:
On Tue, Feb 23, 2010 at 03:35:04PM -0700, Jim Fehlig wrote:
Daniel P. Berrange wrote:
On Mon, Feb 22, 2010 at 02:50:07PM -0700, Jim Fehlig wrote:
Disk devices can be referenced by name in Xen, e.g. when modifying their configuration or remvoving them. As such, don't search xenstore for a device ID corresponding to the disk device. Instead, search the disks contained in the domain definition and use the disk's target name if found.
This approach allows removing a disk when domain is inactive. We obviously can't search xenstore when the domain is inactive.
This sounds reasonable, but I'm wondering if old XenD support the lookup-based on name ?
Yes, I had thought about this as well. Guess it depends on how old we are talking about. It exists in Xen 3.1.x and newer. I wonder if we should even care about Xen 3.0.x. The upstream tree hasn't been touched in 3 years.
Well we still ship this, so it would be appreciated if it didn't broke if libvirt got updated for some reasons ;-)
Yep, RHEL-5 still ships with Xen 3.0.x series & we aim to support that in libvirt. I think we can probably just make your new approach be conditional on the xendConfigVersion variable we already have. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.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)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Fehlig