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

[Wen Congyang]

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.

> 
>>
>>> +    }
>>> +    if (checked->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
>>> +        return 0;
>>> +    return virDomainDeviceInfoIterate(def, virDomainDeviceAddressMatch, checked);
>>> +}
>>>  
>>>  /* Generate a string representation of a device address
>>>   * @param address Device address to stringify
>>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>>> index b791714..53ccf32 100644
>>> --- a/src/conf/domain_conf.h
>>> +++ b/src/conf/domain_conf.h
>>> @@ -1194,6 +1194,8 @@ int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info);
>>>  void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info);
>>>  void virDomainDefClearPCIAddresses(virDomainDefPtr def);
>>>  void virDomainDefClearDeviceAliases(virDomainDefPtr def);
>>> +int virDomainDefFindDeviceAddressConflict(virDomainDefPtr def,
>>> +                                           virDomainDeviceDefPtr dev);
>>>  
>>>  typedef int (*virDomainDeviceInfoCallback)(virDomainDefPtr def,
>>>                                             virDomainDeviceInfoPtr dev,
>>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>>> index 7459c40..ffdf9cf 100644
>>> --- a/src/libvirt_private.syms
>>> +++ b/src/libvirt_private.syms
>>> @@ -220,6 +220,7 @@ virDomainCpuSetParse;
>>>  virDomainDefAddImplicitControllers;
>>>  virDomainDefClearDeviceAliases;
>>>  virDomainDefClearPCIAddresses;
>>> +virDomainDefFindDeviceAddressConflict;
>>>  virDomainDefFormat;
>>>  virDomainDefFree;
>>>  virDomainDefParseFile;
>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>> index 4d3b416..e746576 100644
>>> --- a/src/qemu/qemu_driver.c
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -4144,6 +4144,9 @@ cleanup:
>>>      return ret;
>>>  }
>>>  
>>> +/*
>>> + * Checking device's definition meets qemu's (and qemu drivre's) limitation.
>>> + */
>>
>> Is this comment in correct place?
>> You do not modify this function. If this comment is for this function, it should be
>> merged into patch 2.
>> According to the comment's content, it is not for this function.
>>
> 
> will check again. I'm not a good git user ;(
> 
> Thanks,
> -Kame
> 
>