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

On Tue, 29 Mar 2011 13:30:03 +0800 Wen Congyang <wency(a)cn.fujitsu.com&gt; wrote: > At 03/29/2011 01:13 PM, KAMEZAWA Hiroyuki Write: >> On Tue, 29 Mar 2011 13:09:37 +0800 >> Wen Congyang <wency(a)cn.fujitsu.com&gt; 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(a)jp.fujitsu.com&gt; >>>> 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(a)jp.fujitsu.com&gt; >>>> --- >>>> 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 ?

Thanks, -Kame