5:54 a.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote on 05/11/2010 06:25:05
AM:
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
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.