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

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



On Tue, May 11, 2010 at 06:54:18AM -0400, Stefan Berger wrote:
> "Daniel P. Berrange" <berrange redhat com> wrote on 05/11/2010 06:25:05 
> AM:
> 
> 
> > 
> > On Mon, May 10, 2010 at 07:57:37PM -0400, Stefan Berger wrote:
> > > Below is David Alan's original patch with lots of changes. 
> > > 
> > > In particular, it now parses the following XML and stored the data
> > > internally. No sending of netlink messages has been implemented here.
> > > 
> > >    <interface type='direct'>
> > >       <source dev='static' mode='vepa'/>
> > >       <model type='virtio'/>
> > >       <vsi managerid='12' typeid='0x123456' typeidversion='1'
> > >            instanceid='fa9b7fff-b0a0-4893-8e0e-beef4ff18f8f' />
> > >       <filterref filter='clean-traffic'/>
> > >     </interface>
> > > 
> > >     <interface type='direct'>
> > >       <source dev='static' mode='vepa'/>
> > >       <model type='virtio'/>
> > >       <vsi profileid='my_profile'/>
> > >     </interface>
> > 
> > Could we have an explanation of what these attributes all mean in
> > the commit message. 
> 
> 
> To summarize here for now:
> The 1st provides parameters necessary to run a protocol between the 
> host and the switch to setup switch parameters for a VM's
> traffic (for example). The protocol will be run by LLDPAD (daemon) 
> getting the parameters passed via netlink messages where libvirt 
> will (likely) send the message in form of a (netlink) multicast to
> be ignored by the kernel and handled by LLDPAD. The 1st is to
> support the (pre-)standard 802.1Qbg.
> 
> The 2nd one provides a similar parameter necessary also for
> running a protocol between the host and the switch. In this case
> there will be support by the Ethernet adapter's firmware to run the
> protocol and libvirt will trigger it via a netlink message digested
> by the kernel + driver. This is to support the (pre-)standard 802.1Qbh.
> 
> > 
> > Also, when we have 2 different sets of attributes, we normally use
> > a type attribute on the element to tell the parser what set of
> > data to expect. So I think this should gain a 'type' attribute here.
> > 
> > > @@ -1873,6 +1879,20 @@ virDomainNetDefParseXML(virCapsPtr caps,
> > >                         xmlStrEqual(cur->name, BAD_CAST "source")) {
> > >                  dev  = virXMLPropString(cur, "dev");
> > >                  mode = virXMLPropString(cur, "mode");
> > > +            } else if ((vsiManagerID == NULL) &&
> > > +                       (vsiTypeID == NULL) &&
> > > +                       (vsiTypeIDVersion == NULL) &&
> > > +                       (vsiInstanceID == NULL) &&
> > > +                       (vsiProfileID == NULL) &&
> > > +                       (def->type == VIR_DOMAIN_NET_TYPE_DIRECT) &&
> > > +                       xmlStrEqual(cur->name, BAD_CAST "vsi")) {
> > > +                vsiManagerID = virXMLPropString(cur, "managerid");
> > > +                vsiTypeID = virXMLPropString(cur, "typeid");
> > > +                vsiTypeIDVersion = virXMLPropString(cur,
> > > "typeidversion");
> > > +                vsiInstanceID = virXMLPropString(cur, "instanceid");
> > > +#ifdef IFLA_VF_PORT_PROFILE_MAX
> > > +                vsiProfileID = virXMLPropString(cur, "profileid");
> > > +#endif
> > 
> > XML parsing routines should not be #ifdefd. The XML format is formally
> > defined by the schema and must never change based on compile time 
> options.
> > 
> 
> Ok. I'll do away with this then.
> 
> > >              } else if ((network == NULL) &&
> > >                         ((def->type == VIR_DOMAIN_NET_TYPE_SERVER) ||
> > >                          (def->type == VIR_DOMAIN_NET_TYPE_CLIENT) ||
> > > @@ -2049,6 +2069,51 @@ virDomainNetDefParseXML(virCapsPtr caps,
> > >          } else
> > >              def->data.direct.mode =
> > > VIR_DOMAIN_NETDEV_MACVTAP_MODE_VEPA;
> > > 
> > > +        vsi = &def->data.direct.vsiProfile;
> > > +
> > > +#ifdef IFLA_VF_PORT_PROFILE_MAX
> > > +        if (vsiProfileID != NULL) {
> > > +            if (virStrcpyStatic(vsi->u.vsi8021Qbh.profileID,
> > > +                                vsiProfileID) != NULL) {
> > > +                vsi->vsiType = VIR_VSI_8021QBH;
> > > +                break;
> > > +            }
> > > +        }
> > > +#endif
> > 
> > Likewise here
> 
> Ok.
> 
> > > 
> > > 
> > > +#define IFLA_VF_PORT_PROFILE_MAX 40
> > > +enum virVSIType {
> > > +    VIR_VSI_INVALID,
> > 
> > This isn't really 'INVALID' - this is better named 'NONE'
> > since its simply an indication that this interface does not
> > have any VSI info defined
> 
> 
> Will change it.
> 
> > 
> > > +    VIR_VSI_8021QBG,
> > > +#ifdef IFLA_VF_PORT_PROFILE_MAX
> > > +    VIR_VSI_8021QBH,
> > > +#endif
> > 
> > And here, etc
> 
> Ok.
> 
> > 
> > > +};
> > > +
> > > +/* profile data for macvtap (VEPA) */
> > > +typedef struct _virVSIProfileDef virVSIProfileDef;
> > > +typedef virVSIProfileDef *virVSIProfileDefPtr;
> > > +struct _virVSIProfileDef {
> > > +    enum virVSIType   vsiType;
> > > +    union {
> > > +        struct {
> > > +            uint8_t       managerID;
> > > +            uint32_t      typeID; // 24 bit valid
> > > +            uint8_t       typeIDVersion;
> > > +            unsigned char instanceID[VIR_UUID_BUFLEN];
> > > +        } vsi8021Qbg;
> > > +#ifdef IFLA_VF_PORT_PROFILE_MAX
> > > +        struct {
> > > +            char          profileID[IFLA_VF_PORT_PROFILE_MAX];
> > > +        } vsi8021Qbh;
> > > +#endif
> 
> The size of this character array is supposed to be 40 chars as per a 
> kernel #define that will become available
> through some future kernel include and #define. I'd like to restrict the 
> size of the profile id somewhere
> when parsing it... What's the best way to do that? Introduce a constant 
> that also has 40 as value ?

The XML parsers shouldn't rely on platform #defines, so add a constant 
in the XML parser itself.

Regardless of this, the actual implementation in qemu_driver.c should also
check length boundary if the kernel API its using has a fixed buffer size

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]