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