[libvirt] [PATCH] Reject duplicate disk targets

[Daniel P. Berrange]

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 at 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 :|