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@xxxxxxxxxx> >> --- >> 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 > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list