"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 ?

   Stefan