[libvirt] [PATCH] Reject duplicate disk targets
We never check for disks with duplicate targets connected to the same controller/bus/etc. That means we go ahead, create the same IDs for them and pass them to qemu, which subsequently fails to start. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=968899 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3398d8b..01720e1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2629,6 +2629,45 @@ virDomainDeviceInfoIterate(virDomainDefPtr def, static int +virDomainDefRejectDuplicateDiskTargets(virDomainDefPtr def) +{ + char *disk_id = NULL; + int ret = -1; + size_t i = 0; + virHashTablePtr targets = NULL; + + if (!(targets = virHashCreate(def->ndisks, NULL))) + goto cleanup; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + if (virAsprintf(&disk_id, "%d%s%d%d%d%d", + disk->bus, + NULLSTR(disk->dst), + disk->info.addr.drive.controller, + disk->info.addr.drive.bus, + disk->info.addr.drive.target, + disk->info.addr.drive.unit) < 0) + goto cleanup; + + if (virHashAddEntry(targets, disk_id, NULL) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Multiple disks with same address and target ('%s')"), + def->disks[i]->dst); + VIR_FREE(disk_id); + goto cleanup; + } + VIR_FREE(disk_id); + } + + ret = 0; + cleanup: + virHashFree(targets); + return ret; +} + +static int virDomainDefRejectDuplicateControllers(virDomainDefPtr def) { int max_idx[VIR_DOMAIN_CONTROLLER_TYPE_LAST]; @@ -2762,7 +2801,8 @@ virDomainDefPostParseInternal(virDomainDefPtr def, } } - if (virDomainDefRejectDuplicateControllers(def) < 0) + if (virDomainDefRejectDuplicateControllers(def) < 0 || + virDomainDefRejectDuplicateDiskTargets(def) < 0) return -1; return 0; } -- 1.8.3.2
On Wed, Jul 10, 2013 at 02:34:30PM +0200, Martin Kletzander wrote:
We never check for disks with duplicate targets connected to the same controller/bus/etc. That means we go ahead, create the same IDs for them and pass them to qemu, which subsequently fails to start.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=968899 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3398d8b..01720e1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2629,6 +2629,45 @@ virDomainDeviceInfoIterate(virDomainDefPtr def,
static int +virDomainDefRejectDuplicateDiskTargets(virDomainDefPtr def) +{ + char *disk_id = NULL; + int ret = -1; + size_t i = 0; + virHashTablePtr targets = NULL; + + if (!(targets = virHashCreate(def->ndisks, NULL))) + goto cleanup; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + if (virAsprintf(&disk_id, "%d%s%d%d%d%d", + disk->bus, + NULLSTR(disk->dst), + disk->info.addr.drive.controller, + disk->info.addr.drive.bus, + disk->info.addr.drive.target, + disk->info.addr.drive.unit) < 0)
Err, disk->info.addr is a union of many different types of address. You can't just arbitrarily reference info.addr.drive without first validating the type of the union. Also, it seems like this problem is more general than just disks. Any type of device can have an <address> element set and cause a clash, not merely disks attached to controllers. So I'd say we want something that iterates over all devices in the domaindef and validates every address element. Yes, we might catch PCI address clashes later in QEMU code, but there's no harm in detecting them up front, if we can do so in a way that is generally applicable to all address types Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/10/2013 02:39 PM, Daniel P. Berrange wrote:
On Wed, Jul 10, 2013 at 02:34:30PM +0200, Martin Kletzander wrote:
We never check for disks with duplicate targets connected to the same controller/bus/etc. That means we go ahead, create the same IDs for them and pass them to qemu, which subsequently fails to start.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=968899 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3398d8b..01720e1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2629,6 +2629,45 @@ virDomainDeviceInfoIterate(virDomainDefPtr def,
static int +virDomainDefRejectDuplicateDiskTargets(virDomainDefPtr def) +{ + char *disk_id = NULL; + int ret = -1; + size_t i = 0; + virHashTablePtr targets = NULL; + + if (!(targets = virHashCreate(def->ndisks, NULL))) + goto cleanup; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + if (virAsprintf(&disk_id, "%d%s%d%d%d%d", + disk->bus, + NULLSTR(disk->dst), + disk->info.addr.drive.controller, + disk->info.addr.drive.bus, + disk->info.addr.drive.target, + disk->info.addr.drive.unit) < 0)
Err, disk->info.addr is a union of many different types of address. You can't just arbitrarily reference info.addr.drive without first validating the type of the union.
I jumped straight into that because we know it is disk and we do it the same way when assigning aliases. I'd be glad to see what option I missed, thanks for any pointers.
Also, it seems like this problem is more general than just disks. Any type of device can have an <address> element set and cause a clash, not merely disks attached to controllers.
The address duplication problem is already dealt with, but the id we are creating for disks are dependent on disk->dst. I added the other address types because if two same disks are connected to different controllers/addresses, there is no problem.
So I'd say we want something that iterates over all devices in the domaindef and validates every address element.
Yes, we might catch PCI address clashes later in QEMU code, but there's no harm in detecting them up front, if we can do so in a way that is generally applicable to all address types
Daniel
On Wed, Jul 10, 2013 at 03:51:45PM +0200, Martin Kletzander wrote:
On 07/10/2013 02:39 PM, Daniel P. Berrange wrote:
On Wed, Jul 10, 2013 at 02:34:30PM +0200, Martin Kletzander wrote:
We never check for disks with duplicate targets connected to the same controller/bus/etc. That means we go ahead, create the same IDs for them and pass them to qemu, which subsequently fails to start.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=968899 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3398d8b..01720e1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2629,6 +2629,45 @@ virDomainDeviceInfoIterate(virDomainDefPtr def,
static int +virDomainDefRejectDuplicateDiskTargets(virDomainDefPtr def) +{ + char *disk_id = NULL; + int ret = -1; + size_t i = 0; + virHashTablePtr targets = NULL; + + if (!(targets = virHashCreate(def->ndisks, NULL))) + goto cleanup; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + if (virAsprintf(&disk_id, "%d%s%d%d%d%d", + disk->bus, + NULLSTR(disk->dst), + disk->info.addr.drive.controller, + disk->info.addr.drive.bus, + disk->info.addr.drive.target, + disk->info.addr.drive.unit) < 0)
Err, disk->info.addr is a union of many different types of address. You can't just arbitrarily reference info.addr.drive without first validating the type of the union.
I jumped straight into that because we know it is disk and we do it the same way when assigning aliases. I'd be glad to see what option I missed, thanks for any pointers.
Assigning aliases is not the same, because the alias is stored in the same part of the struct, regardless of address type. Any disk with bus=virtio uses PCI addressing. Any disk with bus=usb uses USB addresing. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/10/2013 04:03 PM, Daniel P. Berrange wrote:
On Wed, Jul 10, 2013 at 03:51:45PM +0200, Martin Kletzander wrote:
On 07/10/2013 02:39 PM, Daniel P. Berrange wrote:
On Wed, Jul 10, 2013 at 02:34:30PM +0200, Martin Kletzander wrote:
We never check for disks with duplicate targets connected to the same controller/bus/etc. That means we go ahead, create the same IDs for them and pass them to qemu, which subsequently fails to start.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=968899 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3398d8b..01720e1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2629,6 +2629,45 @@ virDomainDeviceInfoIterate(virDomainDefPtr def,
static int +virDomainDefRejectDuplicateDiskTargets(virDomainDefPtr def) +{ + char *disk_id = NULL; + int ret = -1; + size_t i = 0; + virHashTablePtr targets = NULL; + + if (!(targets = virHashCreate(def->ndisks, NULL))) + goto cleanup; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + if (virAsprintf(&disk_id, "%d%s%d%d%d%d", + disk->bus, + NULLSTR(disk->dst), + disk->info.addr.drive.controller, + disk->info.addr.drive.bus, + disk->info.addr.drive.target, + disk->info.addr.drive.unit) < 0)
Err, disk->info.addr is a union of many different types of address. You can't just arbitrarily reference info.addr.drive without first validating the type of the union.
I jumped straight into that because we know it is disk and we do it the same way when assigning aliases. I'd be glad to see what option I missed, thanks for any pointers.
Assigning aliases is not the same, because the alias is stored in the same part of the struct, regardless of address type.
Any disk with bus=virtio uses PCI addressing. Any disk with bus=usb uses USB addresing.
In order to make this as unified as possible, I suggest we check it when the domain is starting, right after these aliases are assigned. Reasoning -- there is no problem with using same addresses, this is already being checked for. The problem is only with our internal naming which is externally seen only when we use it for describing disks to qemu. Because the way we choose how to describe is based upon qemu capabilities, it would make sense to do it when starting the domain then. Looking back at this patch, it might break some non-qemu machines. The patch would then look like the following, would you be OK with that? Martin diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 325ef38..f19c83f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -864,6 +864,32 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) } +static int +qemuRejectDuplicateDiskIDs(virDomainDefPtr def) +{ + int ret = -1; + size_t i = 0; + virHashTablePtr targets = NULL; + + if (!(targets = virHashCreate(def->ndisks, NULL))) + goto cleanup; + + for (i = 0; i < def->ndisks; i++) { + if (virHashAddEntry(targets, def->disks[i]->info.alias, NULL) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Multiple disks with the same alias ('%s')"), + def->disks[i]->info.alias); + goto cleanup; + } + } + + ret = 0; + cleanup: + virHashFree(targets); + return ret; +} + + int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) { @@ -886,6 +912,9 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) } } + if (qemuRejectDuplicateDiskIDs(def) < 0) + return -1; + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) return 0; --
On Wed, Jul 10, 2013 at 07:22:11PM +0200, Martin Kletzander wrote:
On 07/10/2013 04:03 PM, Daniel P. Berrange wrote:
On Wed, Jul 10, 2013 at 03:51:45PM +0200, Martin Kletzander wrote:
On 07/10/2013 02:39 PM, Daniel P. Berrange wrote:
On Wed, Jul 10, 2013 at 02:34:30PM +0200, Martin Kletzander wrote:
We never check for disks with duplicate targets connected to the same controller/bus/etc. That means we go ahead, create the same IDs for them and pass them to qemu, which subsequently fails to start.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=968899 Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3398d8b..01720e1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2629,6 +2629,45 @@ virDomainDeviceInfoIterate(virDomainDefPtr def,
static int +virDomainDefRejectDuplicateDiskTargets(virDomainDefPtr def) +{ + char *disk_id = NULL; + int ret = -1; + size_t i = 0; + virHashTablePtr targets = NULL; + + if (!(targets = virHashCreate(def->ndisks, NULL))) + goto cleanup; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = def->disks[i]; + + if (virAsprintf(&disk_id, "%d%s%d%d%d%d", + disk->bus, + NULLSTR(disk->dst), + disk->info.addr.drive.controller, + disk->info.addr.drive.bus, + disk->info.addr.drive.target, + disk->info.addr.drive.unit) < 0)
Err, disk->info.addr is a union of many different types of address. You can't just arbitrarily reference info.addr.drive without first validating the type of the union.
I jumped straight into that because we know it is disk and we do it the same way when assigning aliases. I'd be glad to see what option I missed, thanks for any pointers.
Assigning aliases is not the same, because the alias is stored in the same part of the struct, regardless of address type.
Any disk with bus=virtio uses PCI addressing. Any disk with bus=usb uses USB addresing.
In order to make this as unified as possible, I suggest we check it when the domain is starting, right after these aliases are assigned.
Reasoning -- there is no problem with using same addresses, this is already being checked for. The problem is only with our internal naming which is externally seen only when we use it for describing disks to qemu. Because the way we choose how to describe is based upon qemu capabilities, it would make sense to do it when starting the domain then. Looking back at this patch, it might break some non-qemu machines. The patch would then look like the following, would you be OK with that?
Martin
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 325ef38..f19c83f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -864,6 +864,32 @@ qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller) }
+static int +qemuRejectDuplicateDiskIDs(virDomainDefPtr def) +{ + int ret = -1; + size_t i = 0; + virHashTablePtr targets = NULL; + + if (!(targets = virHashCreate(def->ndisks, NULL))) + goto cleanup; + + for (i = 0; i < def->ndisks; i++) { + if (virHashAddEntry(targets, def->disks[i]->info.alias, NULL) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Multiple disks with the same alias ('%s')"), + def->disks[i]->info.alias); + goto cleanup; + } + } + + ret = 0; + cleanup: + virHashFree(targets); + return ret; +} + + int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) { @@ -886,6 +912,9 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) } }
+ if (qemuRejectDuplicateDiskIDs(def) < 0) + return -1; + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) return 0;
Huh, that doesn't make any sense. The 'alias' data is not user specified. If they attempt to set it in the XML, it'll be dropped by the parser. The alias names are assigned by the QEMU driver unconditionally and unless that code is broken in some way, you'll never get a duplicate alias Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/11/2013 11:47 AM, Daniel P. Berrange wrote: [...]
Huh, that doesn't make any sense. The 'alias' data is not user specified. If they attempt to set it in the XML, it'll be dropped by the parser. The alias names are assigned by the QEMU driver unconditionally and unless that code is broken in some way, you'll never get a duplicate alias
It is. And that's what i was trying to fix. But when trying to explain myself properly in this email, I went through the code few times and found out this is deeper than I thought. I thought the addresses were checked but it looks like it doesn't apply for other than PCI and CCW addresses. We should definitely check "drive" and "virtio" addresses as well and keep them for hot(un)plug and other purposes (probably the same way as we already do with PCI addresses). I'll rewrite it from scratch, but just want to point out that for example for virtio disks, the alias is based just on the 'target' attribute, which can be duplicated in the XML and that's why I was trying to add check for the alias as well. Have a nice day, Martin
participants (2)
-
Daniel P. Berrange -
Martin Kletzander