Re: [libvirt] [PATCH] Reject duplicate disk targets
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(a)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