> Sent: Wednesday, July 18, 2018 at 12:38 AM
> From: "John Ferlan" <jferlan@xxxxxxxxxx>
> To: "Shi Lei" <shilei.massclouds@xxxxxxx>, libvir-list@xxxxxxxxxx
> Subject: Re: [PATCH 2/3] support new forward mode of vlan for virtual network
>
>
>
> On 07/05/2018 11:36 PM, Shi Lei wrote:
> > Signed-off-by: Shi Lei <shilei.massclouds@xxxxxxx>
> > ---
> > src/conf/network_conf.c | 12 ++++---
> > src/conf/network_conf.h | 1 +
> > src/network/bridge_driver.c | 80 +++++++++++++++++++++++++++++++++++++++++----
> > 3 files changed, 82 insertions(+), 11 deletions(-)
> >
>
> This would be missing RNG changes in docs/schemas/network.rng,
> documentation changes in docs/formatnetwork.html.in in order to describe
> things, and networkxml2* tests. Usually if you look at the last person
> to add a new element to something it'll give you a good hint. In this
> case, see commit id 25e8112d7 when "open" was added. I use gitk to chase
> changes, but others use straight git - whatever works for you.
OK. Thank you! I will add these in patch v2.
>
> > diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> > index 630a87f..6e1de6c 100644
> > --- a/src/conf/network_conf.c
> > +++ b/src/conf/network_conf.c
> > @@ -50,7 +50,7 @@ VIR_ENUM_IMPL(virNetworkForward,
> > VIR_NETWORK_FORWARD_LAST,
> > "none", "nat", "route", "open",
> > "bridge", "private", "vepa", "passthrough",
> > - "hostdev")
> > + "hostdev", "vlan")
> >
> > VIR_ENUM_IMPL(virNetworkBridgeMACTableManager,
> > VIR_NETWORK_BRIDGE_MAC_TABLE_MANAGER_LAST,
> > @@ -1876,6 +1876,7 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
> > */
> > switch (def->forward.type) {
> > case VIR_NETWORK_FORWARD_NONE:
> > + case VIR_NETWORK_FORWARD_VLAN:
> > break;
> >
> > case VIR_NETWORK_FORWARD_ROUTE:
> > @@ -1963,9 +1964,10 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt)
> > (def->forward.type != VIR_NETWORK_FORWARD_NONE &&
> > def->forward.type != VIR_NETWORK_FORWARD_NAT &&
> > def->forward.type != VIR_NETWORK_FORWARD_ROUTE &&
> > - def->forward.type != VIR_NETWORK_FORWARD_OPEN)) {
> > + def->forward.type != VIR_NETWORK_FORWARD_OPEN &&
> > + def->forward.type != VIR_NETWORK_FORWARD_VLAN)) {
> > virReportError(VIR_ERR_XML_ERROR,
> > - _("mtu size only allowed in open, route, nat, "
> > + _("mtu size only allowed in open, route, nat, vlan "
> > "and isolated mode, not in %s (network '%s')"),
> > virNetworkForwardTypeToString(def->forward.type),
> > def->name);
> > @@ -2474,6 +2476,7 @@ virNetworkDefFormatBuf(virBufferPtr buf,
> > def->forward.type == VIR_NETWORK_FORWARD_NAT ||
> > def->forward.type == VIR_NETWORK_FORWARD_ROUTE ||
> > def->forward.type == VIR_NETWORK_FORWARD_OPEN ||
> > + def->forward.type == VIR_NETWORK_FORWARD_VLAN ||
> > def->bridge || def->macTableManager) {
> >
> > virBufferAddLit(buf, "<bridge");
> > @@ -2481,7 +2484,8 @@ virNetworkDefFormatBuf(virBufferPtr buf,
> > if (def->forward.type == VIR_NETWORK_FORWARD_NONE ||
> > def->forward.type == VIR_NETWORK_FORWARD_NAT ||
> > def->forward.type == VIR_NETWORK_FORWARD_ROUTE ||
> > - def->forward.type == VIR_NETWORK_FORWARD_OPEN) {
> > + def->forward.type == VIR_NETWORK_FORWARD_OPEN ||
> > + def->forward.type == VIR_NETWORK_FORWARD_VLAN) {
> > virBufferAsprintf(buf, " stp='%s' delay='%ld'",
> > def->stp ? "on" : "off", def->delay);
> > }
> > diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> > index 54c8ed1..47bb83e 100644
> > --- a/src/conf/network_conf.h
> > +++ b/src/conf/network_conf.h
> > @@ -53,6 +53,7 @@ typedef enum {
> > VIR_NETWORK_FORWARD_VEPA,
> > VIR_NETWORK_FORWARD_PASSTHROUGH,
> > VIR_NETWORK_FORWARD_HOSTDEV,
> > + VIR_NETWORK_FORWARD_VLAN,
> >
> > VIR_NETWORK_FORWARD_LAST,
> > } virNetworkForwardType;
> > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> > index da3c32e..314f78c 100644
> > --- a/src/network/bridge_driver.c
> > +++ b/src/network/bridge_driver.c
> > @@ -451,6 +451,7 @@ networkUpdateState(virNetworkObjPtr obj,
> > case VIR_NETWORK_FORWARD_NAT:
> > case VIR_NETWORK_FORWARD_ROUTE:
> > case VIR_NETWORK_FORWARD_OPEN:
> > + case VIR_NETWORK_FORWARD_VLAN:
> > /* If bridge doesn't exist, then mark it inactive */
> > if (!(def->bridge && virNetDevExists(def->bridge) == 1))
> > virNetworkObjSetActive(obj, false);
> > @@ -2092,7 +2093,8 @@ networkRefreshDaemonsHelper(virNetworkObjPtr obj,
> > ((def->forward.type == VIR_NETWORK_FORWARD_NONE) ||
> > (def->forward.type == VIR_NETWORK_FORWARD_NAT) ||
> > (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) ||
> > - (def->forward.type == VIR_NETWORK_FORWARD_OPEN))) {
> > + (def->forward.type == VIR_NETWORK_FORWARD_OPEN) ||
> > + (def->forward.type == VIR_NETWORK_FORWARD_VLAN))) {
> > /* Only the three L3 network types that are configured by
> > * libvirt will have a dnsmasq or radvd daemon associated
> > * with them. Here we send a SIGHUP to an existing
> > @@ -2131,7 +2133,8 @@ networkReloadFirewallRulesHelper(virNetworkObjPtr obj,
> > if (virNetworkObjIsActive(obj) &&
> > ((def->forward.type == VIR_NETWORK_FORWARD_NONE) ||
> > (def->forward.type == VIR_NETWORK_FORWARD_NAT) ||
> > - (def->forward.type == VIR_NETWORK_FORWARD_ROUTE))) {
> > + (def->forward.type == VIR_NETWORK_FORWARD_ROUTE) ||
> > + (def->forward.type == VIR_NETWORK_FORWARD_VLAN))) {
> > /* Only three of the L3 network types that are configured by
> > * libvirt need to have iptables rules reloaded. The 4th L3
> > * network type, forward='open', doesn't need this because it
> > @@ -2513,6 +2516,27 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver,
> > if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0)
> > goto err5;
> >
> > + if (def->forward.type == VIR_NETWORK_FORWARD_VLAN) {
> > + char vlanDevName[IFNAMSIZ];
>
> char *vlanDevName = NULL; (needs to be at the top)
OK.
>
> > + /* ifs[0].device.dev and vlan.tag[0] have been validated in networkValidate() */
> > + if (virNetDevCreateVLanDev(def->forward.ifs[0].device.dev, def->vlan.tag[0],
> > + vlanDevName, sizeof(vlanDevName)) < 0)
>
> Make sure the vlanDevName aligns under the def->forward...
OK.
>
> > + goto err5;
>
> err5 is for bandwidth failure, maybe there needs to be a err6 for VLAN
> and within that there will need to be a VIR_FREE(vlanDevName);
OK.
>
> > +
> > + if (virNetDevBridgeAddPort(def->bridge, vlanDevName) < 0) {
> > + ignore_value(virNetDevDestroyVLanDev(def->forward.ifs[0].device.dev,
> > + def->vlan.tag[0]));
> > + goto err5;
> > + }
> > +
> > + if (virNetDevSetOnline(vlanDevName, true) < 0) {
> > + ignore_value(virNetDevBridgeRemovePort(def->bridge, vlanDevName));
> > + ignore_value(virNetDevDestroyVLanDev(def->forward.ifs[0].device.dev,
> > + def->vlan.tag[0]));
>
> BTW: If all callers to virNetDevDestroyVLanDev are just going to be
> wrapped in ignore_value(), then let's just make it a void and be done
> with it.
OK. I think about it.
>
> > + goto err5;
> > + }
>
> VIR_FREE(vlanDevName);
>
> > + }
> > +
> > VIR_FREE(macTapIfName);
> > VIR_FREE(macMapFile);
> >
> > @@ -2576,6 +2600,18 @@ networkShutdownNetworkVirtual(virNetworkDriverStatePtr driver,
> > pid_t radvdPid;
> > pid_t dnsmasqPid;
> >
> > + if (def->forward.type == VIR_NETWORK_FORWARD_VLAN) {
> > + char vlanDevName[IFNAMSIZ];
>
> char *vlanDevName = NULL;
>
> > + ignore_value(virNetDevGetVLanDevName(def->forward.ifs[0].device.dev,
> > + def->vlan.tag[0],
> > + vlanDevName, sizeof(vlanDevName)));
>
> don't think this can be ignored since it's used as input for other
> functions that follow that may not expect a NULL vlanDevName
Yes. I'll fix it!
>
> > +
> > + ignore_value(virNetDevSetOnline(vlanDevName, false));
> > + ignore_value(virNetDevBridgeRemovePort(def->bridge, vlanDevName));
> > + ignore_value(virNetDevDestroyVLanDev(def->forward.ifs[0].device.dev,
>
> I'm now having more thoughts related to whether virNetDevDestroyVLanDev
> should take an ifname. Above you do the virNetDevGetVLanDevName call
> and then again when calling virNetDevDestroyVLanDev, so why not just
> have it take the vdname instead.
>
>
>
> > + def->vlan.tag[0]));
>
>
> VIR_FREE(vlanDevName);
>
> John
Yes. It's better to take the vdname. Thanks!
Shi Lei
>
> > + }
> > +
> > if (def->bandwidth)
> > virNetDevBandwidthClear(def->bridge);
> >
> > @@ -2719,6 +2755,7 @@ networkCreateInterfacePool(virNetworkDefPtr netdef)
> > case VIR_NETWORK_FORWARD_NAT:
> > case VIR_NETWORK_FORWARD_ROUTE:
> > case VIR_NETWORK_FORWARD_OPEN:
> > + case VIR_NETWORK_FORWARD_VLAN:
> > case VIR_NETWORK_FORWARD_LAST:
> > /* by definition these will never be encountered here */
> > break;
> > @@ -2817,6 +2854,7 @@ networkStartNetwork(virNetworkDriverStatePtr driver,
> > case VIR_NETWORK_FORWARD_NAT:
> > case VIR_NETWORK_FORWARD_ROUTE:
> > case VIR_NETWORK_FORWARD_OPEN:
> > + case VIR_NETWORK_FORWARD_VLAN:
> > if (networkStartNetworkVirtual(driver, obj) < 0)
> > goto cleanup;
> > break;
> > @@ -2899,6 +2937,7 @@ networkShutdownNetwork(virNetworkDriverStatePtr driver,
> > case VIR_NETWORK_FORWARD_NAT:
> > case VIR_NETWORK_FORWARD_ROUTE:
> > case VIR_NETWORK_FORWARD_OPEN:
> > + case VIR_NETWORK_FORWARD_VLAN:
> > ret = networkShutdownNetworkVirtual(driver, obj);
> > break;
> >
> > @@ -3276,7 +3315,8 @@ networkValidate(virNetworkDriverStatePtr driver,
> > if (def->forward.type == VIR_NETWORK_FORWARD_NONE ||
> > def->forward.type == VIR_NETWORK_FORWARD_NAT ||
> > def->forward.type == VIR_NETWORK_FORWARD_ROUTE ||
> > - def->forward.type == VIR_NETWORK_FORWARD_OPEN) {
> > + def->forward.type == VIR_NETWORK_FORWARD_OPEN ||
> > + def->forward.type == VIR_NETWORK_FORWARD_VLAN) {
> >
> > /* if no bridge name was given in the config, find a name
> > * unused by any other libvirt networks and assign it.
> > @@ -3447,7 +3487,8 @@ networkValidate(virNetworkDriverStatePtr driver,
> > * a pool, and those using an Open vSwitch bridge.
> > */
> >
> > - vlanAllowed = (def->forward.type == VIR_NETWORK_FORWARD_HOSTDEV ||
> > + vlanAllowed = (def->forward.type == VIR_NETWORK_FORWARD_VLAN ||
> > + def->forward.type == VIR_NETWORK_FORWARD_HOSTDEV ||
> > def->forward.type == VIR_NETWORK_FORWARD_PASSTHROUGH ||
> > (def->forward.type == VIR_NETWORK_FORWARD_BRIDGE &&
> > def->virtPortProfile &&
> > @@ -3530,6 +3571,28 @@ networkValidate(virNetworkDriverStatePtr driver,
> > }
> > }
> > }
> > +
> > + if (def->forward.type == VIR_NETWORK_FORWARD_VLAN) {
> > + if (virNetDevLoad8021Q() < 0)
> > + return -1;
> > +
> > + if (def->forward.nifs != 1 ||
> > + strlen(def->forward.ifs[0].device.dev) == 0) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("invalid dev in <forward mode='vlan' dev='xxx'> "
> > + "of network '%s' with forward mode='%s'"),
> > + def->name, virNetworkForwardTypeToString(def->forward.type));
> > + return -1;
> > + }
> > + if (def->vlan.nTags != 1 || def->vlan.tag[0] >= 4096) {
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("unsupported vlan config. check <vlan><tag id='xxx'> "
> > + "of network '%s' with forward mode='%s'"),
> > + def->name, virNetworkForwardTypeToString(def->forward.type));
> > + return -1;
> > + }
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -3757,7 +3820,8 @@ networkUpdate(virNetworkPtr net,
> > */
> > if (def->forward.type == VIR_NETWORK_FORWARD_NONE ||
> > def->forward.type == VIR_NETWORK_FORWARD_NAT ||
> > - def->forward.type == VIR_NETWORK_FORWARD_ROUTE) {
> > + def->forward.type == VIR_NETWORK_FORWARD_ROUTE ||
> > + def->forward.type == VIR_NETWORK_FORWARD_VLAN) {
> > switch (section) {
> > case VIR_NETWORK_SECTION_FORWARD:
> > case VIR_NETWORK_SECTION_FORWARD_INTERFACE:
> > @@ -4443,7 +4507,8 @@ networkAllocateActualDevice(virDomainDefPtr dom,
> > if ((netdef->forward.type == VIR_NETWORK_FORWARD_NONE) ||
> > (netdef->forward.type == VIR_NETWORK_FORWARD_NAT) ||
> > (netdef->forward.type == VIR_NETWORK_FORWARD_ROUTE) ||
> > - (netdef->forward.type == VIR_NETWORK_FORWARD_OPEN)) {
> > + (netdef->forward.type == VIR_NETWORK_FORWARD_OPEN) ||
> > + (netdef->forward.type == VIR_NETWORK_FORWARD_VLAN)) {
> > /* for these forward types, the actual net type really *is*
> > * NETWORK; we just keep the info from the portgroup in
> > * iface->data.network.actual
> > @@ -4701,7 +4766,8 @@ networkAllocateActualDevice(virDomainDefPtr dom,
> > * mode) and openvswitch bridges. Otherwise log an error and
> > * fail
> > */
> > - if (!(actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV ||
> > + if (!(netdef->forward.type == VIR_NETWORK_FORWARD_VLAN ||
> > + actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV ||
> > (actualType == VIR_DOMAIN_NET_TYPE_DIRECT &&
> > virDomainNetGetActualDirectMode(iface)
> > == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) ||
> >
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
