[libvirt] [PATCH 4/5] conf: add global check for duplicate drive addresses

[Michal Privoznik]

On 30.11.2016 12:47, Marc Hartmayer wrote:
> Add a global check for duplicate drive addresses. This will fix the
> problem of duplicate disk and hostdev drive addresses.
> 
> Example for duplicate drive addresses:
> <disk>
>   ...
>   <target name='sda'/>
> </disk>
> <disk>
>   ...
>   <target name='sdb'/>
>   <address type='drive' controller=0 bus=0 target=0 unit=0/>
> </disk>
> 
> Another example:
> <hostdev mode='subsystem' type='scsi' managed='no'>
>   <source>
>   ...
>   </source>
>   <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> </hostdev>
> <hostdev mode='subsystem' type='scsi' managed='no'>
>   <source>
>   ...
>   </source>
>   <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> </hostdev>
> 
> Unfortunately the fixes (1b08cc170a84077afd4d15f4639a9a2cf398e9a2,
> 8d46386bfe01b84982e25e915ad9cfbae5cf4cb1) weren't enough to catch these
> cases and it isn't possible to add additional checks in
> virDomainDeviceDefPostParseInternal() for SCSI hostdevs or
> virDomainDiskDefAssignAddress() for SCSI/IDE/FDC/SATA disks without
> adding another parse flag (virDomainDefParseFlags) to disable this
> validation while updating or detaching a disk or hostdev.
> 
> Signed-off-by: Marc Hartmayer <mhartmay at linux.vnet.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
> ---
>  src/conf/domain_conf.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 104 insertions(+)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index cd30728..cb47980 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -4841,6 +4841,107 @@ virDomainDefCheckDuplicateDiskInfo(const virDomainDef *def)
>      return 0;
>  }
>  
> +/**
> + * virDomainDefCheckDuplicateDriveAddresses:
> + * @def: domain definition to check against
> + *
> + * This function checks @def for duplicate drive addresses. Drive
> + * addresses are only in use for disks and hostdevs at the moment.
> + *
> + * Returns 0 in case of there are no duplicate drive addresses, -1
> + * otherwise.
> + */
> +static int
> +virDomainDefCheckDuplicateDriveAddresses(const virDomainDef *def)
> +{
> +    size_t i;
> +    size_t j;
> +
> +    for (i = 0; i < def->ndisks; i++) {
> +        virDomainDiskDefPtr disk_i = def->disks[i];
> +        virDomainDeviceInfoPtr disk_info_i = &disk_i->info;
> +        if (disk_info_i->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)
> +            continue;

We like to have an empty line between variable declarations and the
actual code. Just like you did at the beginning of this function. But
that's something I can fix before pushing.

> +
> +        for (j = i + 1; j < def->ndisks; j++) {
> +            virDomainDiskDefPtr disk_j = def->disks[j];
> +            virDomainDeviceInfoPtr disk_info_j = &disk_j->info;
> +            if (disk_i->bus != disk_j->bus)
> +                continue;
> +
> +            if (disk_info_j->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE)
> +                continue;
> +
> +            if (virDomainDeviceInfoAddressIsEqual(disk_info_i, disk_info_j)) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("Found duplicate drive address for disk with "
> +                                 "target name '%s' controller='%u' bus='%u' "
> +                                 "target='%u' unit='%u'"),
> +                               disk_i->dst,
> +                               disk_info_i->addr.drive.controller,
> +                               disk_info_i->addr.drive.bus,
> +                               disk_info_i->addr.drive.target,
> +                               disk_info_i->addr.drive.unit);
> +                return -1;
> +            }
> +        }
> +
> +        /* Note: There is no need to check for conflicts with SCSI
> +         * hostdevs above, because conflicts with hostdevs are checked
> +         * in the next loop.
> +         */
> +    }

Michal