On Tue, 29 Mar 2011 13:30:03 +0800
Wen Congyang <wency@xxxxxxxxxxxxxx> wrote:
> At 03/29/2011 01:13 PM, KAMEZAWA Hiroyuki Write:
> > 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.
>
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list