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

[KAMEZAWA Hiroyuki]

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.

> 
> > +    }
> > +    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