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

Re: [libvirt] [PATCH v8] vepa: parsing for 802.1Qb{g|h} XML



On Mon, May 24, 2010 at 01:12:12PM -0700, Chris Wright wrote:
> * Hugh O. Brock (hbrock redhat com) wrote:
> > On Mon, May 24, 2010 at 11:47:18AM -0700, Chris Wright wrote:
> > > * Daniel P. Berrange (berrange redhat com) wrote:
> > > > On Sun, May 23, 2010 at 12:51:50PM -0400, Stefan Berger wrote:
> > > > > Index: libvirt-acl/src/util/macvtap.h
> > > > > ===================================================================
> > > > > --- libvirt-acl.orig/src/util/macvtap.h
> > > > > +++ libvirt-acl/src/util/macvtap.h
> > > > > @@ -27,15 +27,14 @@
> > > > >  # if defined(WITH_MACVTAP)
> > > > >  
> > > > >  #  include "internal.h"
> > > > > +#  include "conf/domain_conf.h"
> > > > 
> > > > This isn't allowed. It is introducing a dependancy cycle
> > > > between the util & conf directories. Code in util/ is not
> > > > allowed to depend on any other code in the libvirt tree.
> > > 
> > > IOW, you mean using virDomainNetDefPtr in openMacvtapTap is a libvirt
> > > layering violation, and you'd prefer openMacvtapTap() w/ large number
> > > of parameters?  I think it's impractical to not invent some structure to
> > > pass the data...otherwise, I believe the worst case would be:
> > > 
> > > int openMacvtapTap(const char *tgifname,
> > > 		   const unsigned char *macaddress,
> > > 		   const char *linkdev,
> > > 		   int mode,
> > > 		   char **res_ifname,
> > > 		   int vnet_hdr,
> > > 		   int vf,
> > > 		   int port_type,
> > > 		   unsigned char mgrid,
> > > 		   unsigned typeid,
> > > 		   const unsigned char *instanceid,
> > > 		   const unsigned char *profileid,
> > > 		   const unsigned char *vmuuid)
> > > 
> > > But, any such structure will create some dependency.
> > > 
> > > What do you think?
> > > 
> > > thanks,
> > > -chris
> > 
> > Earlier today on IRC Dan said:
> > 
> > <danpb> if that's really neccessary, a macvtap.h should have its own
> > struct definition, separate from the XML structs.
> > 
> > Hopefully that makes sense in this context?
> 
> It does.  The only question is how to share the structure definition.
> For example, we have:
> 
> typedef struct _virVirtualPortProfileDef virVirtualPortProfileDef;
> struct _virVirtualPortProfileDef {
>     enum virVirtualPortType   virtPortType;
>     union {
>         struct {
>             uint8_t       managerID;
>             uint32_t      typeID; // 24 bit valid
>             uint8_t       typeIDVersion;
>             unsigned char instanceID[VIR_UUID_BUFLEN];
>         } virtPort8021Qbg;
>         struct {
>             char          profileID[LIBVIRT_IFLA_VF_PORT_PROFILE_MAX];
>         } virtPort8021Qbh;
>     } u;
> };
> 
> So we either promote the union or structures within virVirtualPortProfileDef
> to a common def and pass those, or create some new def to share.

Yep, just moving this struct into macvtap.h would be sufficient
for now. Also rename it to virVirtualPortProfileParams while doing
this, since names ending in 'Def' are specifically related to the
XML parser routines.


> $ grep conf/ src/util/*.[ch]
> src/util/hooks.c:#include "conf/domain_conf.h"
> src/util/macvtap.c:# include "conf/domain_conf.h"

Both bugs that must be fixed. The include in hooks.c doesn't appear to be
needed at all - it compiles fine without it. Likewise macvtap.c compiles
fine without it for me too.


Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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