On Tue, May 11, 2010 at 06:54:18AM -0400, Stefan Berger wrote:
> "Daniel P. Berrange" <berrange@xxxxxxxxxx> 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list