[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Tue, Mar 29, 2011 at 11:40:44AM +0900, KAMEZAWA Hiroyuki wrote:
> On Tue, 29 Mar 2011 10:41:57 +0800
> Hu Tao <hutao cn fujitsu com> wrote:
> 
> > On Fri, Mar 25, 2011 at 05:17:03PM +0900, KAMEZAWA Hiroyuki wrote:
> > > >From 638341bdf3eaac824e36d265e134608279750049 Mon Sep 17 00:00:00 2001
> > > From: KAMEZAWA Hiroyuki <kamezawa hiroyu 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 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))
> > 
> > !STREQ instead of strcmp
> > 
> 
> ok.
> 
> 
> > > +            return -1;
> > > +    if (!memcmp(&info->addr, &checked->addr, sizeof(info->addr)))
> > 
> > Is it safe to memcmp an union like this? In the cases members of an
> > union are of different sizes, and we intent to memcmp an union member
> > that has a smaller size than the other members, then data in space
> > not used by the union member to be compared is also compared. This is
> > not a desired result.
> 
> As far as I checked, it's zero cleared at allocation. Hmm, making this function
> bigger ?

Yes it is safe if zero-cleared. Not worth to make this function
complicated.

-- 
Thanks,
Hu Tao


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]