On Tue, 29 Mar 2011 13:09:37 +0800 Wen Congyang <wency@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxx> > > --- > > 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. > > > + } > > + 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list