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

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.