[libvirt] [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.

[Wen Congyang]

At 03/29/2011 01:31 PM, KAMEZAWA Hiroyuki Write:
> On Tue, 29 Mar 2011 13:30:03 +0800
> Wen Congyang <wency at cn.fujitsu.com> wrote:
> 
>> At 03/29/2011 01:13 PM, KAMEZAWA Hiroyuki Write:
>>> On Tue, 29 Mar 2011 13:09:37 +0800
>>> Wen Congyang <wency at cn.fujitsu.com> wrote:
>>>
>>>> At 03/25/2011 04:17 PM, KAMEZAWA Hiroyuki Write:
>>>>> >From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001
>>>>> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu at jp.fujitsu.com>
>>>>> Date: Fri, 25 Mar 2011 17:10:58 +0900
>>>>> Subject: [PATCHv7 3/4] libvirt/qemu - check address confliction before addition.
>>>>>
>>>>> qemuDomainAttachDevicePersistent() calls qemuDomainAssignPCIAddresses()
>>>>> and virDomainDefAddImplicitControllers() at the end of its call.
>>>>>
>>>>> But PCI/Drive address confliction checks are
>>>>>  PCI - confliction will be found but error report is not verbose.
>>>>>  Drive - never done.
>>>>>
>>>>> For example, you can add following (unusual) 2 devices without errors.
>>>>>
>>>>> <disk type='file' device='disk'>
>>>>>    <driver name='qemu' type='raw'/>
>>>>>    <source file='/var/lib/libvirt/images/test3.img'/>
>>>>>    <target dev="sdx" bus='scsi'/>
>>>>>    <address type='drive' controller='0' bus='0' unit='0'/>
>>>>> </disk>
>>>>>
>>>>> <disk type='file' device='disk'>
>>>>>    <driver name='qemu' type='raw'/>
>>>>>    <source file='/var/lib/libvirt/images/test3.img'/>
>>>>>    <target dev="sdy" bus='scsi'/>
>>>>>    <address type='drive' controller='0' bus='0' unit='0'/>
>>>>> </disk>
>>>>>
>>>>> It's better to check drive address confliction before addition.
>>>>>
>>>>> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu at jp.fujitsu.com>
>>>>> ---
>>>>>  src/conf/domain_conf.c   |   59 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  src/conf/domain_conf.h   |    2 +
>>>>>  src/libvirt_private.syms |    1 +
>>>>>  src/qemu/qemu_driver.c   |    9 +++++++
>>>>>  4 files changed, 71 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>>>> index 3e3f342..4a54f62 100644
>>>>> --- a/src/conf/domain_conf.c
>>>>> +++ b/src/conf/domain_conf.c
>>>>> @@ -1287,6 +1287,65 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def)
>>>>>      virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL);
>>>>>  }
>>>>>  
>>>>> +static int virDomainDeviceAddressMatch(virDomainDefPtr def ATTRIBUTE_UNUSED,
>>>>> +                                       virDomainDeviceInfoPtr info,
>>>>> +                                       void *opaque)
>>>>> +{
>>>>> +    virDomainDeviceInfoPtr checked = opaque;
>>>>> +    /* skip to check confliction of alias */
>>>>> +    if (info->type != checked->type)
>>>>> +            return 0;
>>>>> +    if (info->alias && checked->alias && strcmp(info->alias, checked->alias))
>>>>> +            return -1;
>>>>> +    if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr)))
>>>>> +            return -1;
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def,
>>>>> +                                          virDomainDeviceDefPtr dev)
>>>>> +{
>>>>> +    virDomainDeviceInfoPtr checked;
>>>>> +
>>>>> +    switch (dev->type) {
>>>>> +    case VIR_DOMAIN_DEVICE_DISK:
>>>>> +        checked = &dev->data.disk->info;
>>>>> +        break;
>>>>> +    case VIR_DOMAIN_DEVICE_FS:
>>>>> +        checked = &dev->data.fs->info;
>>>>> +        break;
>>>>> +    case VIR_DOMAIN_DEVICE_NET:
>>>>> +        checked = &dev->data.net->info;
>>>>> +        break;
>>>>> +    case VIR_DOMAIN_DEVICE_INPUT:
>>>>> +        checked = &dev->data.input->info;
>>>>> +        break;
>>>>> +    case VIR_DOMAIN_DEVICE_SOUND:
>>>>> +        checked = &dev->data.sound->info;
>>>>> +        break;
>>>>> +    case VIR_DOMAIN_DEVICE_VIDEO:
>>>>> +        checked = &dev->data.video->info;
>>>>> +        break;
>>>>> +    case VIR_DOMAIN_DEVICE_HOSTDEV:
>>>>> +        checked = &dev->data.hostdev->info;
>>>>> +        break;
>>>>> +    case VIR_DOMAIN_DEVICE_WATCHDOG:
>>>>> +        checked = &dev->data.watchdog->info;
>>>>> +        break;
>>>>> +    case VIR_DOMAIN_DEVICE_CONTROLLER:
>>>>> +        checked = &dev->data.controller->info;
>>>>> +        break;
>>>>> +    case VIR_DOMAIN_DEVICE_GRAPHICS: /* has no address info */
>>>>> +        return 0;
>>>>> +    default:
>>>>> +        virDomainReportError(VIR_ERR_INTERNAL_ERROR,
>>>>> +                             "%s", _("Unknown device type"));
>>>>> +        return -1;
>>>>
>>>> You report error here, but you report error in caller(qemuDomainAttachDevicePersistent())
>>>> again.
>>>
>>> "unknown device type" is an internal/"should never happen" error rathar than
>>> errors reported in the caller.
>>>
>>> I think this error should be reported. Internal error implies bug.
>>
>> Yes, this should not happen. If it happens, the error message will be overwritten by the caller.
>> We can report error here if virDomainDeviceInfoIterate() returns -1, and the caller do not
>> report error.
>>
> 
> Is that make usability of this function ? If we report error, this function cannot
> be used for simple sanity check of pci addresses.
> 
> Hmm, I'll replace this with DEBUG message. ok and never add error report here.
> Ok ?

Sounds good. But use VIR_ERR instead of VIR_DEBUG, as it implies a bug.

> 
> Thanks,
> -Kame
> 
> 
> 
>