This looks good to me, with some minor nits below. Acked-by: Kyle Mestery <kmestery@xxxxxxxxx> On Aug 14, 2012, at 2:04 AM, Laine Stump wrote: > One of the original ideas behind allowing a <virtualport> in an > interface definition as well as in the <network> definition *and*one > or more <portgroup>s within the network, was that guest-specific > parameteres (like instanceid and interfaceid) could be given in the > interface's virtualport, and more general things (portid, managerid, > etc) could be given in the network and/or portgroup, with all the bits > brought together at guest startup time and combined into a single > virtualport to be used by the guest. This was somehow overlooked in > the implementation, though - it simply picks the "most specific" > virtualport, and uses the entire thing, with no attempt to merge in > details from the others. > > This patch uses virNetDevVPortProfileMerge3() to combine the three > possible virtualports into one, then uses > virNetDevVPortProfileCheck*() to verify that the resulting virtualport > type is appropriate for the type of network, and that all the required > attributes for that type are present. > > An example of usage is this: assuming a <network> definitions on host > ABC of: > > <network> > <name>testA</name> > ... > <virtualport type='openvswitch'/> > ... > <portgroup name='engineering'> > <virtualport> > <parameters profileid='eng'/> > </virtualport> > </portgroup> > <portgroup name='sales'> > <virtualport> > <parameters profileid='sales'/> > </virtualport> > </portgroup> > </network> > > and the same <network> on host DEF of: > > <network> > <name>testA</name> > ... > <virtualport type='802.1Qbg'> > <parameters typeid="1193047" typeidversion="2"/> > </virtualport> > ... > <portgroup name='engineering'> > <virtualport> > <parameters managerid="11"/> > </virtualport> > </portgroup> > <portgroup name='sales'> > <virtualport> > <parameters managerid="55"/> > </virtualport> > </portgroup> > </network> > > and a guest <interface> definition of: > > <interface type='network'> > <source network='testA' portgroup='sales'/> > <virtualport> > <parameters instanceid="09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f" > interfaceid="09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f"\> > </virtualport> > ... > </interface> > > If the guest was started on host ABC, the <virtualport> used would be: > > <virtualport type='openvswitch'> > <parameters interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f' > profileid='sales'/> > </virtualport> > > but if that guest was started on host DEF, the <virtualport> would be: > > <virtualport type='802.1Qbg'> > <parameters instanceid="09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f" > typeid="1193047" typeidversion="2" > managerid="55"/> > </virtualport> > > Additionally, if none of the involved <virtualport>s had a specified type > (this includes cases where no virtualport is given at all), > --- > docs/formatdomain.html.in | 72 ++++++++++++++++++++++++++++++++++++------- > docs/formatnetwork.html.in | 27 ++++++++++++----- > src/network/bridge_driver.c | 74 ++++++++++++++++++++++++++++++++------------- > 3 files changed, 134 insertions(+), 39 deletions(-) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index f97c630..f3b3fa8 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -2230,11 +2230,40 @@ > the network; one network may have multiple portgroups defined, > with each portgroup containing slightly different configuration > information for different classes of network > - connections. <span class="since">Since 0.9.4</span>). Also, > - similar to <code>direct</code> network connections (described > - below), a connection of type <code>network</code> may specify > - a <code>virtportprofile</code> element, with configuration data > - to be forwarded to a vepa or 802.1Qbh compliant switch. > + connections. <span class="since">Since 0.9.4</span>. > + </p> > + <p> > + Also, similar to <code>direct</code> network connections > + (described below), a connection of type <code>network</code> may > + specify a <code>virtualport</code> element, with configuration > + data to be forwarded to a vepa (802.1Qbg) or 802.1Qbh compliant > + switch (<span class="since">Since 0.8.2</span>), or to an > + OpenvSwitch virtual switch (<span class="since">Since If you look at http://openvswitch.org/, you can see they use "Open vSwitch" instead of "OpenvSwitch" as the name. Can you adjust the term in this patch to match that? > + 0.9.11</span>). > + </p> > + <p> > + Since the actual type of switch may vary depending on the > + configuration in the <code><network></code> on the host, > + it is acceptable to omit the virtualport <code>type</code> > + attribute, and specify attributes from multiple different > + virtualport types (and also to leave out certain attributes); at > + domain startup time, a complete <code><virtualport></code> > + element will be constructed by merging together the type and > + attributes found in the which will be filled in from the network > + or portgroup <code><virtualport></code>) > + (<span class="since">Since 0.10.0</span>). For example, in order > + to work properly with both an 802.1Qbh switch and an OpenvSwitch Same "Open vSwitch" adjustment here. > + switch, you may choose to specify no type, but both > + an <code>instanceid</code> (in case the switch is 802.1Qbh) and > + an <code>interfaceid</code> (in case the switch is OpenvSwitch) Same "Open vSwitch" adjustment here. > + (you may also omit the other attributes, such as managerid, > + typeid, or profileid, to be filled in from the > + network's <code><virtualport></code>). If you want to > + limit a guest to connecting only to certain types of switches, > + you can specify the virtualport type, but still omit some/all of > + the parameters - in this case if the host's network has a > + different type of virtualport, connection of the interface will > + fail. > </p> > > <pre> > @@ -2248,8 +2277,8 @@ > <source network='default' portgroup='engineering'/> > <target dev='vnet7'/> > <mac address="00:11:22:33:44:55"/> > - <virtualport type='802.1Qbg'> > - <parameters managerid='11' typeid='1193047' typeidversion='2' instanceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/> > + <virtualport> > + <parameters instanceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/> > </virtualport> > > </interface> > @@ -2261,7 +2290,7 @@ > <p> > <strong><em> > This is the recommended config for general guest connectivity on > - hosts with static wired networking configs > + hosts with static wired networking configs. > </em></strong> > </p> > > @@ -2276,19 +2305,40 @@ > configuration is whatever is used on the LAN. This provides the guest VM > full incoming & outgoing net access just like a physical machine. > </p> > - > + <p> > + On Linux systems, the bridge device is normally a standard Linux > + host bridge. On hosts that support OpenvSwitch, it is also Same "Open vSwitch" adjustment here. > + possible to connect to an openvswitch bridge device by adding > + a <code><virtualport type='openvswitch'/></code> to the > + interface definition. (<span class="since">Since > + 0.9.11</span>). The OpenvSwitch type virtualport accepts two > + parameters in its <code><parameters></code> element - > + an <code>interfaceid</code> which is a standard uuid used to > + uniquely identify this particular interface to OpenvSwitch (if One more "Open vSwitch". > + you do no specify one, a random interfaceid will be generated > + for you when you first define the interface), and an > + optional <code>profileid</code> which is sent to OpenvSwitch as Same "Open vSwitch" adjustment here. > + the interfaces "port-profile". > + </p> > <pre> > ... > <devices> > + ... > <interface type='bridge'> > <source bridge='br0'/> > </interface> > - ... > <interface type='bridge'> > - <source bridge='br0'/> > + <source bridge='br1'/> > <target dev='vnet7'/> > <mac address="00:11:22:33:44:55"/> > </interface> > + <interface type='bridge'> > + <source bridge='ovsbr'/> > + <virtualport type='openvswitch'/> > + <parameters profileid='menial' interfaceid='09b11c53-8b5c-4eeb-8f00-d84eaa0aaa4f'/> > + </virtualport> > + </interface> > + ... > </devices> > ...</pre> > > diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in > index 7e8e991..0ba4d2d 100644 > --- a/docs/formatnetwork.html.in > +++ b/docs/formatnetwork.html.in > @@ -147,10 +147,17 @@ > This network describes either 1) an existing host bridge > that was configured outside of libvirt (if > a <code><bridge name='xyz'/></code> element has been > - specified), or 2) an interface or group of interfaces to > - be used for a "direct" connection via macvtap using > - macvtap's "bridge" mode (if the forward element has one or > - more <code><interface></code> subelements) > + specified, <span class="since">Since 0.9.4</span>), 2) an > + existing OpenvSwitch bridge that was configured outside of "Open vSwitch" > + libvirt (if both a <code><bridge name='xyz'/></code> > + element <b>and</b> a <code><virtualport > + type='openvswitch'/></code> have been > + specified <span class="since">Since 0.10.0</span>) 3) an > + interface or group of interfaces to be used for a "direct" > + connection via macvtap using macvtap's "bridge" mode (if > + the forward element has one or > + more <code><interface></code> > + subelements, <span class="since">Since 0.9.4</span>) > (see <a href="formatdomain.html#elementsNICSDirect">Direct > attachment to physical interface</a> for descriptions of > the various macvtap modes). libvirt doesn't attempt to > @@ -337,9 +344,15 @@ > default portgroup will be used. If no portgroup is given in the > interface definition, and there is no default portgroup, then > none will be used. Any <code><bandwidth></code> > - or <code><virtualport></code> specified directly in the > - domain XML will take precedence over any setting in the chosen > - portgroup. > + > + specified directly in the domain XML will take precedence over > + any setting in the chosen portgroup. if > + a <code><virtualport></code> is specified in the portgroup > + (and/or directly in the network definition), the multiple > + virtualports will be merged, and any parameter that is specified > + in more than one virtualport, and is not identical, will be > + considered an error, and will prevent the interface from > + starting. > </p> > > <h3><a name="elementsAddress">Addressing</a></h3> > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index d9646fb..ec99e4d 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -61,6 +61,7 @@ > #include "virnetdev.h" > #include "virnetdevbridge.h" > #include "virnetdevtap.h" > +#include "virnetdevvportprofile.h" > > #define NETWORK_PID_DIR LOCALSTATEDIR "/run/libvirt/network" > #define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network" > @@ -2746,6 +2747,8 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) > virNetworkObjPtr network; > virNetworkDefPtr netdef; > virPortGroupDefPtr portgroup; > + virNetDevVPortProfilePtr virtport = NULL; > + virNetworkForwardIfDefPtr dev = NULL; > unsigned int num_virt_fns = 0; > char **vfname = NULL; > int ii; > @@ -2820,11 +2823,33 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) > goto cleanup; > } > > + /* merge virtualports from interface, network, and portgroup to > + * arrive at actual virtualport to use > + */ > + if (virNetDevVPortProfileMerge3(&iface->data.network.actual->virtPortProfile, > + iface->virtPortProfile, > + netdef->virtPortProfile, > + portgroup > + ? portgroup->virtPortProfile : NULL) < 0) { > + goto cleanup; > + } > + virtport = iface->data.network.actual->virtPortProfile; > + if (virtport) { > + /* only type='openvswitch' is allowed for bridges */ > + if (virtport->virtPortType != VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("<virtualport type='%s'> not supported for network " > + "'%s' which uses a bridge device"), > + virNetDevVPortTypeToString(virtport->virtPortType), > + netdef->name); > + goto cleanup; > + } > + } > + > } else if ((netdef->forwardType == VIR_NETWORK_FORWARD_BRIDGE) || > (netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) || > (netdef->forwardType == VIR_NETWORK_FORWARD_VEPA) || > (netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH)) { > - virNetDevVPortProfilePtr virtport = NULL; > > /* <forward type='bridge|private|vepa|passthrough'> are all > * VIR_DOMAIN_NET_TYPE_DIRECT. > @@ -2853,24 +2878,28 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) > break; > } > > - /* Find the most specific virtportprofile and copy it */ > - if (iface->virtPortProfile) { > - virtport = iface->virtPortProfile; > - } else { > - if (portgroup) > - virtport = portgroup->virtPortProfile; > - else > - virtport = netdef->virtPortProfile; > + /* merge virtualports from interface, network, and portgroup to > + * arrive at actual virtualport to use > + */ > + if (virNetDevVPortProfileMerge3(&iface->data.network.actual->virtPortProfile, > + iface->virtPortProfile, > + netdef->virtPortProfile, > + portgroup > + ? portgroup->virtPortProfile : NULL) < 0) { > + goto cleanup; > } > + virtport = iface->data.network.actual->virtPortProfile; > if (virtport) { > - if (VIR_ALLOC(iface->data.network.actual->virtPortProfile) < 0) { > - virReportOOMError(); > + /* make sure type is supported for macvtap connections */ > + if (virtport->virtPortType != VIR_NETDEV_VPORT_PROFILE_8021QBG && > + virtport->virtPortType != VIR_NETDEV_VPORT_PROFILE_8021QBH) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("<virtualport type='%s'> not supported for network " > + "'%s' which uses a macvtap device"), > + virNetDevVPortTypeToString(virtport->virtPortType), > + netdef->name); > goto cleanup; > } > - /* There are no pointers in a virtualPortProfile, so a shallow copy > - * is sufficient > - */ > - *iface->data.network.actual->virtPortProfile = *virtport; > } > > /* If there is only a single device, just return it (caller will detect > @@ -2883,8 +2912,6 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) > netdef->name); > goto cleanup; > } else { > - virNetworkForwardIfDefPtr dev = NULL; > - > /* pick an interface from the pool */ > > /* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require > @@ -2967,13 +2994,18 @@ networkAllocateActualDevice(virDomainNetDefPtr iface) > virReportOOMError(); > goto cleanup; > } > - /* we are now assured of success, so mark the allocation */ > - dev->usageCount++; > - VIR_DEBUG("Using physical device %s, usageCount %d", > - dev->dev, dev->usageCount); > } > } > > + if (virNetDevVPortProfileCheckComplete(virtport, true) < 0) > + goto cleanup; > + > + if (dev) { > + /* we are now assured of success, so mark the allocation */ > + dev->usageCount++; > + VIR_DEBUG("Using physical device %s, usageCount %d", > + dev->dev, dev->usageCount); > + } > ret = 0; > cleanup: > for (ii = 0; ii < num_virt_fns; ii++) > -- > 1.7.11.2 > > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list