From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56197) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsV25-0004nq-Am for qemu-devel@nongnu.org; Tue, 08 Jan 2013 03:59:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TsV23-0003C2-OZ for qemu-devel@nongnu.org; Tue, 08 Jan 2013 03:59:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:5548) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TsV23-00039w-GI for qemu-devel@nongnu.org; Tue, 08 Jan 2013 03:59:39 -0500 Date: Tue, 8 Jan 2013 08:59:30 +0000 From: "Daniel P. Berrange" Message-ID: <20130108085930.GF4038@redhat.com> References: <1357610330-10835-1-git-send-email-lig.fnst@cn.fujitsu.com> <1357610330-10835-2-git-send-email-lig.fnst@cn.fujitsu.com> <20130108080403.GA4038@redhat.com> <1357634235.27154.12.camel@liguang.fnst.cn.fujitsu.com> <1357634860.27526.3.camel@liguang.fnst.cn.fujitsu.com> <20130108085110.GE4038@redhat.com> <1357635328.27526.9.camel@liguang.fnst.cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1357635328.27526.9.camel@liguang.fnst.cn.fujitsu.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [libvirt] [PATCH v2 1/2] add pci-bridge controller type Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: li guang Cc: libvir-list@redhat.com, qemu-devel@nongnu.org, mst@redhat.com On Tue, Jan 08, 2013 at 04:55:28PM +0800, li guang wrote: > =E5=9C=A8 2013-01-08=E4=BA=8C=E7=9A=84 08:51 +0000=EF=BC=8CDaniel P. Be= rrange=E5=86=99=E9=81=93=EF=BC=9A > > On Tue, Jan 08, 2013 at 04:47:40PM +0800, li guang wrote: > > > =E5=9C=A8 2013-01-08=E4=BA=8C=E7=9A=84 16:37 +0800=EF=BC=8Cli guang= =E5=86=99=E9=81=93=EF=BC=9A > > > > =E5=9C=A8 2013-01-08=E4=BA=8C=E7=9A=84 08:04 +0000=EF=BC=8CDaniel= P. Berrange=E5=86=99=E9=81=93=EF=BC=9A > > > > > On Tue, Jan 08, 2013 at 09:58:49AM +0800, liguang wrote: > > > > > > Signed-off-by: liguang > > > > > > --- > > > > > > src/conf/device_conf.c | 12 +++++++++++- > > > > > > src/conf/device_conf.h | 1 + > > > > > > src/conf/domain_conf.c | 5 ++++- > > > > > > src/conf/domain_conf.h | 1 + > > > > > > 4 files changed, 17 insertions(+), 2 deletions(-) > > > > > >=20 > > > > > > diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c > > > > > > index 7b97f45..1c06ed0 100644 > > > > > > --- a/src/conf/device_conf.c > > > > > > +++ b/src/conf/device_conf.c > > > > > > @@ -51,16 +51,18 @@ int > > > > > > virDevicePCIAddressParseXML(xmlNodePtr node, > > > > > > virDevicePCIAddressPtr addr) > > > > > > { > > > > > > - char *domain, *slot, *bus, *function, *multi; > > > > > > + char *domain, *slot, *bus, *function, *multi, *bridge; > > > > > > int ret =3D -1; > > > > > > =20 > > > > > > memset(addr, 0, sizeof(*addr)); > > > > > > + addr->bridge =3D -1; > > > > > > =20 > > > > > > domain =3D virXMLPropString(node, "domain"); > > > > > > bus =3D virXMLPropString(node, "bus"); > > > > > > slot =3D virXMLPropString(node, "slot"); > > > > > > function =3D virXMLPropString(node, "function"); > > > > > > multi =3D virXMLPropString(node, "multifunction"); > > > > > > + bridge =3D virXMLPropString(node, "bridge"); > > > > > > =20 > > > > > > if (domain && > > > > > > virStrToLong_ui(domain, NULL, 0, &addr->domain) < 0)= { > > > > > > @@ -98,6 +100,14 @@ virDevicePCIAddressParseXML(xmlNodePtr no= de, > > > > > > goto cleanup; > > > > > > =20 > > > > > > } > > > > > > + > > > > > > + if (bridge && > > > > > > + virStrToLong_i(bridge, NULL, 0, &addr->bridge) < 0) = { > > > > > > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > > > > > > + _("pci-bridge number must be >=3D 0 "= )); > > > > > > + goto cleanup; > > > > > > + } > > > > >=20 > > > > > This is bogus - there's no need for a new 'bridge' attribute - = we > > > > > have 'bus' which is sufficient. > > > >=20 > > > > Oh, yes, this version 'bridge' is unnecessary. > > > >=20 > > > > In former version, I want to discriminate if a pci device want to > > > > sitting on default bus pci.0 or pci-bridge0, so bring in 'bridge'. > > > >=20 > > > > Thanks! > > >=20 > > > but, without 'bridge', can't know if user want or don't want pci-br= idge, > > > and the check for 'bus !=3D 0' will be removed, then if user happen= ed to=20 > > > define a pci device greater than 0, qemu will complain about this, > > > so it's inconvenient for this case. > >=20 > > The check for 'bus !=3D 0' was only added because we didn't have any > > support for bridges. Once we have bridge support, then that check > > can be changed. If bus !=3D 0, then check to see if there's a matchin= g > > bridge device, otherwise raise an error. >=20 > OK for me, though it seems much more changes will be involved > than with 'bridge' condition. The point is that the 'bridge' attribute is redundant information in the XML that you're forcing the admin to specify to avoid doing more coding work in libvirt. That's not optimizing for the right person. Daniel --=20 |: http://berrange.com -o- http://www.flickr.com/photos/dberrange= / :| |: http://libvirt.org -o- http://virt-manager.or= g :| |: http://autobuild.org -o- http://search.cpan.org/~danberr= / :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vn= c :|