These functions all set/get items in the sysfs for a bridge device. --- John suggested splitting the bridge and bridge port functions into two separate commits, but I was lazy :-) (and also I think they are both simple and related enough in form and function, that it's okay to add them in a single patch. Oh, and I seriously doubt anyone would want to backport one without the others). src/libvirt_private.syms | 6 ++ src/util/virnetdevbridge.c | 235 ++++++++++++++++++++++++++++++++++++++++++++- src/util/virnetdevbridge.h | 28 +++++- 3 files changed, 266 insertions(+), 3 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2647d36..6453826 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1662,9 +1662,15 @@ virNetDevBridgeCreate; virNetDevBridgeDelete; virNetDevBridgeGetSTP; virNetDevBridgeGetSTPDelay; +virNetDevBridgeGetVlanFiltering; +virNetDevBridgePortGetLearning; +virNetDevBridgePortGetUnicastFlood; +virNetDevBridgePortSetLearning; +virNetDevBridgePortSetUnicastFlood; virNetDevBridgeRemovePort; virNetDevBridgeSetSTP; virNetDevBridgeSetSTPDelay; +virNetDevBridgeSetVlanFiltering; # util/virnetdevmacvlan.h diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index 15434de..c6a3e8e 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2007-2013 Red Hat, Inc. + * Copyright (C) 2007-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -219,6 +219,170 @@ static int virNetDevBridgeGet(const char *brname, } #endif /* __linux__ */ +#if defined(__linux__) +static int +virNetDevBridgePortSet(const char *brname, + const char *ifname, + const char *paramname, + unsigned long value) +{ + char *path = NULL; + char valuestr[INT_BUFSIZE_BOUND(value)]; + int ret = -1; + + snprintf(valuestr, sizeof(valuestr), "%lu", value); + + if (virAsprintf(&path, "%s/%s/brif/%s/%s", + SYSFS_NET_DIR, brname, ifname, paramname) < 0) + return -1; + + if (!virFileExists(path)) + errno = EINVAL; + else + ret = virFileWriteStr(path, valuestr, 0); + + if (ret < 0) { + virReportSystemError(errno, + _("Unable to set bridge %s port %s %s to %s"), + brname, ifname, paramname, valuestr); + } + + VIR_FREE(path); + return ret; +} + + +static int +virNetDevBridgePortGet(const char *brname, + const char *ifname, + const char *paramname, + unsigned long *value) +{ + char *path = NULL; + char *valuestr = NULL; + int ret = -1; + + if (virAsprintf(&path, "%s/%s/brif/%s/%s", + SYSFS_NET_DIR, brname, ifname, paramname) < 0) + return -1; + + if (virFileReadAll(path, INT_BUFSIZE_BOUND(unsigned long), &valuestr) < 0) + goto cleanup; + + if (virStrToLong_ul(valuestr, NULL, 10, value) < 0) { + virReportSystemError(EINVAL, + _("Unable to get bridge %s port %s %s"), + brname, ifname, paramname); + goto cleanup; + } + + ret = 0; + cleanup: + VIR_FREE(path); + VIR_FREE(valuestr); + return ret; +} + + +int +virNetDevBridgePortGetLearning(const char *brname, + const char *ifname, + bool *enable) +{ + int ret = -1; + unsigned long value; + + if (virNetDevBridgePortGet(brname, ifname, "learning", &value) < 0) + goto cleanup; + + *enable = !!value; + ret = 0; + cleanup: + return ret; +} + + +int +virNetDevBridgePortSetLearning(const char *brname, + const char *ifname, + bool enable) +{ + return virNetDevBridgePortSet(brname, ifname, "learning", enable ? 1 : 0); +} + + +int +virNetDevBridgePortGetUnicastFlood(const char *brname, + const char *ifname, + bool *enable) +{ + int ret = -1; + unsigned long value; + + if (virNetDevBridgePortGet(brname, ifname, "unicast_flood", &value) < 0) + goto cleanup; + + *enable = !!value; + ret = 0; + cleanup: + return ret; +} + + +int +virNetDevBridgePortSetUnicastFlood(const char *brname, + const char *ifname, + bool enable) +{ + return virNetDevBridgePortSet(brname, ifname, "unicast_flood", enable ? 1 : 0); +} + + +#else +int +virNetDevBridgePortGetLearning(const char *brname ATTRIBUTE_UNUSED, + const char *ifname ATTRIBUTE_UNUSED, + bool *enable ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to get bridge port learning on this platform")); + return -1; +} + + +int +virNetDevBridgePortSetLearning(const char *brname ATTRIBUTE_UNUSED, + const char *ifname ATTRIBUTE_UNUSED, + bool enable) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to set bridge port learning on this platform")); + return -1; +} + + +int +virNetDevBridgePortGetUnicastFlood(const char *brname ATTRIBUTE_UNUSED, + const char *ifname ATTRIBUTE_UNUSED, + bool *enable ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to get bridge port unicast_flood on this platform")); + return -1; +} + + +int +virNetDevBridgePortSetUnicastFlood(const char *brname ATTRIBUTE_UNUSED, + const char *ifname ATTRIBUTE_UNUSED, + bool enable ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to set bridge port unicast_flood on this platform")); + return -1; +} +#endif + /** * virNetDevBridgeCreate: @@ -517,7 +681,7 @@ int virNetDevBridgeSetSTPDelay(const char *brname, * @brname: the bridge device name * @delayms: the forward delay in milliseconds * - * Retrives the forward delay for the bridge device @brname + * Retrieves the forward delay for the bridge device @brname * storing it in @delayms. The forward delay is only meaningful * if STP is enabled * @@ -682,3 +846,70 @@ int virNetDevBridgeGetSTP(const char *brname, return -1; } #endif + +#if defined(HAVE_STRUCT_IFREQ) && defined(__linux__) +/** + * virNetDevBridgeGetVlanFiltering: + * @brname: the bridge device name + * @enable: true or false + * + * Retrieves the vlan_filtering setting for the bridge device @brname + * storing it in @enable. + * + * Returns 0 on success, -1 on error + */ +int +virNetDevBridgeGetVlanFiltering(const char *brname, + bool *enable) +{ + int ret = -1; + unsigned long value; + + if (virNetDevBridgeGet(brname, "vlan_filtering", &value, -1, NULL) < 0) + goto cleanup; + + *enable = !!value; + ret = 0; + cleanup: + return ret; +} + + +/** + * virNetDevBridgeSetVlanFiltering: + * @brname: the bridge name + * @enable: true or false + * + * Set the bridge vlan_filtering mode + * + * Returns 0 in case of success or -1 on failure + */ + +int +virNetDevBridgeSetVlanFiltering(const char *brname, + bool enable) +{ + return virNetDevBridgeSet(brname, "vlan_filtering", enable ? 1 : 0, -1, NULL); +} + + +#else +int +virNetDevBridgeGetVlanFiltering(const char *brname ATTRIBUTE_UNUSED, + bool *enable ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to get bridge vlan_filtering on this platform")); + return -1; +} + + +int +virNetDevBridgeSetVlanFiltering(const char *brname ATTRIBUTE_UNUSED, + bool enable ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to set bridge vlan_filtering on this platform")); + return -1; +} +#endif diff --git a/src/util/virnetdevbridge.h b/src/util/virnetdevbridge.h index 13776d2..8188a2f 100644 --- a/src/util/virnetdevbridge.h +++ b/src/util/virnetdevbridge.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2007-2011 Red Hat, Inc. + * Copyright (C) 2007-2012, 2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -51,4 +51,30 @@ int virNetDevBridgeGetSTP(const char *brname, bool *enable) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +int virNetDevBridgeSetVlanFiltering(const char *brname, + bool enable) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int virNetDevBridgeGetVlanFiltering(const char *brname, + bool *enable) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + +int virNetDevBridgePortGetLearning(const char *brname, + const char *ifname, + bool *enable) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_RETURN_CHECK; +int virNetDevBridgePortSetLearning(const char *brname, + const char *ifname, + bool enable) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +int virNetDevBridgePortGetUnicastFlood(const char *brname, + const char *ifname, + bool *enable) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_RETURN_CHECK; +int virNetDevBridgePortSetUnicastFlood(const char *brname, + const char *ifname, + bool enable) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + #endif /* __VIR_NETDEV_BRIDGE_H__ */ -- 1.9.3

These two functions use netlink RTM_NEWNEIGH and RTM_DELNEIGH messages to add and delete entries from a bridge's fdb. The bridge itself is not referenced in the arguments to the functions, only the name of the device that is attached to the bridge (since a device can only be attached to one bridge at a time, and must be attached for this function to make sense, the kernel easily infers which bridge's fdb is being modified by looking at the device name/index). --- Change from V1: turn the anonymous enum for flags into a typedefed enum so the name can be used in the function args, thus providing self-documentation. Also took John's suggestion of documenting the code directly with the information from the commit log message (and added the statement that adding an existing entry, or deleting a non-existing entry, is an error.) src/libvirt_private.syms | 2 + src/util/virnetdevbridge.c | 147 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetdevbridge.h | 16 +++++ 3 files changed, 165 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6453826..6b6c51b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1660,6 +1660,8 @@ virNetDevBandwidthUpdateRate; virNetDevBridgeAddPort; virNetDevBridgeCreate; virNetDevBridgeDelete; +virNetDevBridgeFDBAdd; +virNetDevBridgeFDBDel; virNetDevBridgeGetSTP; virNetDevBridgeGetSTPDelay; virNetDevBridgeGetVlanFiltering; diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index c6a3e8e..78c4a25 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -37,6 +37,9 @@ #include <netinet/in.h> #ifdef __linux__ +# if defined(HAVE_LIBNL) +# include "virnetlink.h" +# endif # include <linux/sockios.h> # include <linux/param.h> /* HZ */ # if NETINET_LINUX_WORKAROUND @@ -913,3 +916,147 @@ virNetDevBridgeSetVlanFiltering(const char *brname ATTRIBUTE_UNUSED, return -1; } #endif + + +#if defined(__linux__) && defined(HAVE_LIBNL) +/* virNetDevBridgeFDBAddDel: + * @mac: the MAC address being added to the table + * @ifname: name of the port (interface) of the bridge that wants this MAC + * @flags: any of virNetDevBridgeFDBFlags ORed together. + * @isAdd: true if adding the entry, fals if deleting + * + * Use netlink RTM_NEWNEIGH and RTM_DELNEIGH messages to add and + * delete entries from a bridge's fdb. The bridge itself is not + * referenced in the arguments to the function, only the name of the + * device that is attached to the bridge (since a device can only be + * attached to one bridge at a time, and must be attached for this + * function to make sense, the kernel easily infers which bridge's fdb + * is being modified by looking at the device name/index). + * + * Attempting to add an existing entry, or delete a non-existing entry + * *is* an error. + * + * returns 0 on success, -1 on failure. + */ +static int +virNetDevBridgeFDBAddDel(const virMacAddr *mac, const char *ifname, + unsigned int flags, bool isAdd) +{ + int ret = -1; + struct nlmsghdr *resp = NULL; + struct nlmsgerr *err; + unsigned int recvbuflen; + struct nl_msg *nl_msg; + struct ndmsg ndm = { .ndm_family = PF_BRIDGE, .ndm_state = NUD_NOARP }; + + if (virNetDevGetIndex(ifname, &ndm.ndm_ifindex) < 0) + return -1; + + if (flags & VIR_NETDEVBRIDGE_FDB_FLAG_ROUTER) + ndm.ndm_flags |= NTF_ROUTER; + if (flags & VIR_NETDEVBRIDGE_FDB_FLAG_SELF) + ndm.ndm_flags |= NTF_SELF; + if (flags & VIR_NETDEVBRIDGE_FDB_FLAG_MASTER) + ndm.ndm_flags |= NTF_MASTER; + /* default self (same as iproute2's bridge command */ + if (!(ndm.ndm_flags & (NTF_MASTER | NTF_SELF))) + ndm.ndm_flags |= NTF_SELF; + + if (flags & VIR_NETDEVBRIDGE_FDB_FLAG_PERMANENT) + ndm.ndm_state |= NUD_PERMANENT; + if (flags & VIR_NETDEVBRIDGE_FDB_FLAG_TEMP) + ndm.ndm_state |= NUD_REACHABLE; + /* default permanent, same as iproute2's bridge command */ + if (!(ndm.ndm_state & (NUD_PERMANENT | NUD_REACHABLE))) + ndm.ndm_state |= NUD_PERMANENT; + + nl_msg = nlmsg_alloc_simple(isAdd ? RTM_NEWNEIGH : RTM_DELNEIGH, + NLM_F_REQUEST | + (isAdd ? (NLM_F_CREATE | NLM_F_EXCL) : 0)); + if (!nl_msg) { + virReportOOMError(); + return -1; + } + + if (nlmsg_append(nl_msg, &ndm, sizeof(ndm), NLMSG_ALIGNTO) < 0) + goto buffer_too_small; + if (nla_put(nl_msg, NDA_LLADDR, VIR_MAC_BUFLEN, mac) < 0) + goto buffer_too_small; + + /* NB: this message can also accept a Destination IP, a port, a + * vlan tag, and a via (see iproute2/bridge/fdb.c:fdb_modify()), + * but those aren't required for our application + */ + + if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0, + NETLINK_ROUTE, 0) < 0) { + goto cleanup; + } + if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL) + goto malformed_resp; + + switch (resp->nlmsg_type) { + case NLMSG_ERROR: + err = (struct nlmsgerr *)NLMSG_DATA(resp); + if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err))) + goto malformed_resp; + if (err->error) { + virReportSystemError(-err->error, + _("error adding fdb entry for %s"), ifname); + goto cleanup; + } + break; + case NLMSG_DONE: + break; + + default: + goto malformed_resp; + } + + ret = 0; + cleanup: + nlmsg_free(nl_msg); + VIR_FREE(resp); + return ret; + + malformed_resp: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed netlink response message")); + goto cleanup; + + buffer_too_small: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("allocated netlink buffer is too small")); + goto cleanup; +} + + +#else +static int +virNetDevBridgeFDBAddDel(const virMacAddr *mac ATTRIBUTE_UNUSED, + const char *ifname ATTRIBUTE_UNUSED, + unsigned int fdbFlags ATTRIBUTE_UNUSED, + bool isAdd ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Unable to add/delete fdb entries on this platform")); + return -1; +} + + +#endif + +int +virNetDevBridgeFDBAdd(const virMacAddr *mac, const char *ifname, + unsigned int flags) +{ + return virNetDevBridgeFDBAddDel(mac, ifname, flags, true); +} + + +int +virNetDevBridgeFDBDel(const virMacAddr *mac, const char *ifname, + unsigned int flags) +{ + return virNetDevBridgeFDBAddDel(mac, ifname, flags, false); +} diff --git a/src/util/virnetdevbridge.h b/src/util/virnetdevbridge.h index 8188a2f..141f231 100644 --- a/src/util/virnetdevbridge.h +++ b/src/util/virnetdevbridge.h @@ -24,6 +24,7 @@ # define __VIR_NETDEV_BRIDGE_H__ # include "internal.h" +# include "virmacaddr.h" int virNetDevBridgeCreate(const char *brname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; @@ -77,4 +78,19 @@ int virNetDevBridgePortSetUnicastFlood(const char *brname, bool enable) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +typedef enum { + VIR_NETDEVBRIDGE_FDB_FLAG_ROUTER = (1 << 0), + VIR_NETDEVBRIDGE_FDB_FLAG_SELF = (1 << 1), + VIR_NETDEVBRIDGE_FDB_FLAG_MASTER = (1 << 2), + + VIR_NETDEVBRIDGE_FDB_FLAG_PERMANENT = (1 << 3), + VIR_NETDEVBRIDGE_FDB_FLAG_TEMP = (1 << 4), +} virNetDevBridgeFDBFlags; + +int virNetDevBridgeFDBAdd(const virMacAddr *mac, const char *ifname, + unsigned int flags) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +int virNetDevBridgeFDBDel(const virMacAddr *mac, const char *ifname, + unsigned int flags) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; #endif /* __VIR_NETDEV_BRIDGE_H__ */ -- 1.9.3

The fdb attribute of a network's bridge subelement tells libvirt how the bridge's forwarding database (fdb) is managed. In the default mode, "learningWithFlood", management of fdb entries is left to the kernel, which determines entries in part by turning on promiscuous mode on all ports of the bridge, flooding packets to all ports when the correct destination is unknown, and adding/removing entries to the fdb as it sees incoming traffic from particular MAC addresses. In "managed" mode, libvirt turns off learning and floowing on all the bridge ports connected to guest domain interfaces, and adds/removes fdb entries according to the MAC addresses in the domain interface configurations. A side effect of turning off learning and unicast_flood on the ports of a bridge is that (with Linux kernel 3.17 and newer), the kernel can automatically turn off promiscuous mode one or more of the bridge's ports (usually only the one interface that is used to connect the bridge to the physical network). The result is better performance (because packets aren't being flooded to all ports, and can be dropped earlier when they are of no interest) and better security (since a guest will only receive traffic intended for the guest interface's configured MAC address). The attribute looks like this in the configuration: <network> <name>test</name> <bridge name='br0' fdb='managed'/> ... This patch only adds the config knob, documentation, and test cases. The functionality behind this knob is added in later patches. --- Changes from V1: Changed name and values of attribute, as outlined in the patchset cover letter. docs/formatnetwork.html.in | 42 +++++++++++++++--- docs/schemas/network.rng | 9 ++++ src/conf/network_conf.c | 50 +++++++++++++++++----- src/conf/network_conf.h | 11 +++++ src/libvirt_private.syms | 2 + tests/networkxml2xmlin/host-bridge-no-flood.xml | 6 +++ .../nat-network-explicit-flood.xml | 21 +++++++++ tests/networkxml2xmlout/host-bridge-no-flood.xml | 6 +++ .../nat-network-explicit-flood.xml | 23 ++++++++++ tests/networkxml2xmltest.c | 2 + 10 files changed, 155 insertions(+), 17 deletions(-) create mode 100644 tests/networkxml2xmlin/host-bridge-no-flood.xml create mode 100644 tests/networkxml2xmlin/nat-network-explicit-flood.xml create mode 100644 tests/networkxml2xmlout/host-bridge-no-flood.xml create mode 100644 tests/networkxml2xmlout/nat-network-explicit-flood.xml diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index dc438ae..e624e1d 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -81,7 +81,7 @@ <pre> ... - &lt;bridge name="virbr0" stp="on" delay="5"/&gt; + &lt;bridge name="virbr0" stp="on" delay="5" fdb="managed"/&gt; &lt;domain name="example.com"/&gt; &lt;forward mode="nat" dev="eth0"/&gt; ...</pre> @@ -92,18 +92,48 @@ defines the name of a bridge device which will be used to construct the virtual network. The virtual machines will be connected to this bridge device allowing them to talk to each other. The bridge device - may also be connected to the LAN. It is recommended that bridge - device names started with the prefix <code>vir</code>, but the name - <code>virbr0</code> is reserved for the "default" virtual - network. This element should always be provided when defining + may also be connected to the LAN. When defining a new network with a <code>&lt;forward&gt;</code> mode of + "nat" or "route" (or an isolated network with - no <code>&lt;forward&gt;</code> element). + no <code>&lt;forward&gt;</code> element), libvirt will + automatically generate a unique name for the bridge device if + none is given, and this name will be permanently stored in the + network configuration so that that the same name will be used + every time the network is started. For these types of networks + (nat, routed, and isolated), a bridge name beginning with the + prefix "virbr" is recommended (and that is what is + auto-generated), but not enforced. Attribute <code>stp</code> specifies if Spanning Tree Protocol is 'on' or 'off' (default is 'on'). Attribute <code>delay</code> sets the bridge's forward delay value in seconds (default is 0). <span class="since">Since 0.3.0</span> + + <p> + The <code>fdb</code> attribute of the bridge element is used + to tell libvirt how the bridge's forwarding database (fdb) + will be updated. In the default + mode <code>learningWithFlood</code>, the kernel + automatically adds and removes entries, using bridge + learning, flooding, and promiscuous mode on the bridge's + ports in order to determine the proper destination for + packets. In <code>managed</code> mode, libvirt disables + learning and unicast_flooding on all bridge ports connected + to guest domain interfaces, and explicitly adds entries to + the fdb based on the MAC addresses in the domain interface + configurations. Turning off learning and unicast_flood can + also permit the kernel to turn off promiscuous mode on some + ports of the bridge, and the combination of turning off + these three settings can improve performance and provide + better security, but can also cause some networking setups + to stop working (e.g. vlan tagging, multicast) and is not + supported by older kernels. + <span class="since">Since 1.2.11, requires kernel 3.17 or + newer</span> + </p> + + </dd> <dt><code>domain</code></dt> <dd> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 4546f80..cc0095b 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -65,6 +65,15 @@ </attribute> </optional> + <optional> + <attribute name="fdb"> + <choice> + <value>learningWithFlood</value> + <value>managed</value> + </choice> + </attribute> + </optional> + </element> </optional> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index a249e32..4c5809d 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -55,6 +55,10 @@ VIR_ENUM_IMPL(virNetworkForward, VIR_NETWORK_FORWARD_LAST, "none", "nat", "route", "bridge", "private", "vepa", "passthrough", "hostdev") +VIR_ENUM_IMPL(virNetworkBridgeFDB, + VIR_NETWORK_BRIDGE_FDB_LAST, + "absent", "learningWithFlood", "managed") + VIR_ENUM_DECL(virNetworkForwardHostdevDevice) VIR_ENUM_IMPL(virNetworkForwardHostdevDevice, VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_LAST, @@ -2108,6 +2112,17 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) } VIR_FREE(tmp); + tmp = virXPathString("string(./bridge[1]/@fdb)", ctxt); + if (tmp) { + if ((def->fdb = virNetworkBridgeFDBTypeFromString(tmp)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid fdb setting '%s' " + "in network '%s'"), tmp, def->name); + goto error; + } + VIR_FREE(tmp); + } + tmp = virXPathString("string(./mac[1]/@address)", ctxt); if (tmp) { if (virMacAddrParse(tmp, &def->mac) < 0) { @@ -2290,6 +2305,14 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) def->name); goto error; } + if (def->fdb) { + virReportError(VIR_ERR_XML_ERROR, + _("bridge fdb setting not allowed " + "in %s mode (network '%s')"), + virNetworkForwardTypeToString(def->forward.type), + def->name); + goto error; + } /* fall through to next case */ case VIR_NETWORK_FORWARD_BRIDGE: if (def->delay || stp) { @@ -2783,22 +2806,27 @@ virNetworkDefFormatBuf(virBufferPtr buf, virBufferAddLit(buf, "</forward>\n"); } + 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_NAT || + def->forward.type == VIR_NETWORK_FORWARD_ROUTE || + def->bridge || def->fdb) { virBufferAddLit(buf, "<bridge"); - if (def->bridge) - virBufferEscapeString(buf, " name='%s'", def->bridge); - virBufferAsprintf(buf, " stp='%s' delay='%ld'/>\n", - def->stp ? "on" : "off", - def->delay); - } else if (def->forward.type == VIR_NETWORK_FORWARD_BRIDGE && - def->bridge) { - virBufferEscapeString(buf, "<bridge name='%s'/>\n", def->bridge); + virBufferEscapeString(buf, " name='%s'", def->bridge); + if (def->forward.type == VIR_NETWORK_FORWARD_NONE || + def->forward.type == VIR_NETWORK_FORWARD_NAT || + def->forward.type == VIR_NETWORK_FORWARD_ROUTE) { + virBufferAsprintf(buf, " stp='%s' delay='%ld'", + def->stp ? "on" : "off", def->delay); + } + if (def->fdb) { + virBufferAsprintf(buf, " fdb='%s'", + virNetworkBridgeFDBTypeToString(def->fdb)); + } + virBufferAddLit(buf, "/>\n"); } - if (def->mac_specified) { char macaddr[VIR_MAC_STRING_BUFLEN]; virMacAddrFormat(&def->mac, macaddr); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 660cd2d..cf9b743 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -54,6 +54,16 @@ typedef enum { } virNetworkForwardType; typedef enum { + VIR_NETWORK_BRIDGE_FDB_ABSENT = 0, + VIR_NETWORK_BRIDGE_FDB_AUTO, + VIR_NETWORK_BRIDGE_FDB_MANAGED, + + VIR_NETWORK_BRIDGE_FDB_LAST, +} virNetworkBridgeFDBType; + +VIR_ENUM_DECL(virNetworkBridgeFDB) + +typedef enum { VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NONE = 0, VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_PCI, VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_NETDEV, @@ -231,6 +241,7 @@ struct _virNetworkDef { int connections; /* # of guest interfaces connected to this network */ char *bridge; /* Name of bridge device */ + int fdb; /* enum virNetworkBridgeFDBType - default is learningWithFlood */ char *domain; unsigned long delay; /* Bridge forward delay (ms) */ bool stp; /* Spanning tree protocol */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6b6c51b..703cba8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -529,6 +529,8 @@ virNetDevVPortTypeToString; # conf/network_conf.h virNetworkAssignDef; +virNetworkBridgeFDBTypeFromString; +virNetworkBridgeFDBTypeToString; virNetworkConfigChangeSetup; virNetworkConfigFile; virNetworkDefCopy; diff --git a/tests/networkxml2xmlin/host-bridge-no-flood.xml b/tests/networkxml2xmlin/host-bridge-no-flood.xml new file mode 100644 index 0000000..d5fced0 --- /dev/null +++ b/tests/networkxml2xmlin/host-bridge-no-flood.xml @@ -0,0 +1,6 @@ +<network> + <name>host-bridge-net</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a8e</uuid> + <forward mode="bridge"/> + <bridge name="br0" fdb='managed'/> +</network> diff --git a/tests/networkxml2xmlin/nat-network-explicit-flood.xml b/tests/networkxml2xmlin/nat-network-explicit-flood.xml new file mode 100644 index 0000000..d6fa1b7 --- /dev/null +++ b/tests/networkxml2xmlin/nat-network-explicit-flood.xml @@ -0,0 +1,21 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <bridge name="virbr0" fdb="learningWithFlood"/> + <forward mode="nat" dev="eth1"/> + <ip address="192.168.122.1" netmask="255.255.255.0"> + <dhcp> + <range start="192.168.122.2" end="192.168.122.254"/> + <host mac="00:16:3e:77:e2:ed" name="a.example.com" ip="192.168.122.10"/> + <host mac="00:16:3e:3e:a9:1a" name="b.example.com" ip="192.168.122.11"/> + </dhcp> + </ip> + <ip family="ipv4" address="192.168.123.1" netmask="255.255.255.0"> + </ip> + <ip family="ipv6" address="2001:db8:ac10:fe01::1" prefix="64"> + </ip> + <ip family="ipv6" address="2001:db8:ac10:fd01::1" prefix="64"> + </ip> + <ip family="ipv4" address="10.24.10.1"> + </ip> +</network> diff --git a/tests/networkxml2xmlout/host-bridge-no-flood.xml b/tests/networkxml2xmlout/host-bridge-no-flood.xml new file mode 100644 index 0000000..ff07943 --- /dev/null +++ b/tests/networkxml2xmlout/host-bridge-no-flood.xml @@ -0,0 +1,6 @@ +<network> + <name>host-bridge-net</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a8e</uuid> + <forward mode='bridge'/> + <bridge name='br0' fdb='managed'/> +</network> diff --git a/tests/networkxml2xmlout/nat-network-explicit-flood.xml b/tests/networkxml2xmlout/nat-network-explicit-flood.xml new file mode 100644 index 0000000..340f6bb --- /dev/null +++ b/tests/networkxml2xmlout/nat-network-explicit-flood.xml @@ -0,0 +1,23 @@ +<network> + <name>default</name> + <uuid>81ff0d90-c91e-6742-64da-4a736edb9a9b</uuid> + <forward dev='eth1' mode='nat'> + <interface dev='eth1'/> + </forward> + <bridge name='virbr0' stp='on' delay='0' fdb='learningWithFlood'/> + <ip address='192.168.122.1' netmask='255.255.255.0'> + <dhcp> + <range start='192.168.122.2' end='192.168.122.254'/> + <host mac='00:16:3e:77:e2:ed' name='a.example.com' ip='192.168.122.10'/> + <host mac='00:16:3e:3e:a9:1a' name='b.example.com' ip='192.168.122.11'/> + </dhcp> + </ip> + <ip family='ipv4' address='192.168.123.1' netmask='255.255.255.0'> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fe01::1' prefix='64'> + </ip> + <ip family='ipv6' address='2001:db8:ac10:fd01::1' prefix='64'> + </ip> + <ip family='ipv4' address='10.24.10.1'> + </ip> +</network> diff --git a/tests/networkxml2xmltest.c b/tests/networkxml2xmltest.c index 65ac591..34a5211 100644 --- a/tests/networkxml2xmltest.c +++ b/tests/networkxml2xmltest.c @@ -120,6 +120,8 @@ mymain(void) DO_TEST("hostdev"); DO_TEST_FULL("hostdev-pf", VIR_NETWORK_XML_INACTIVE); DO_TEST("passthrough-address-crash"); + DO_TEST("nat-network-explicit-flood"); + DO_TEST("host-bridge-no-flood"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.9.3

When the actualType of a virDomainNetDef is "network", it means that we are connecting to a libvirt-managed network (routed, natted, or isolated) which does use a bridge device (created by libvirt). In the past we have required drivers such as qemu to call the public API to retrieve the bridge name in this case (even though it is available in the NetDef's ActualNetDef if the actualType is "bridge" (i.e., an externally-created bridge that isn't managed by libvirt). There is no real reason for this difference, and as a matter of fact it complicates things for qemu. Also, there is another bridge-related attribute (fdb) that will need to be available in both cases, so this makes things consistent. In order to avoid problems when restarting libvirtd after an update from an older version that *doesn't* store the network's bridgename in the ActualNetDef, we also need to put it in place during networkNotifyActualDevice() (this function is run for each interface of each domain whenever libvirtd is restarted). Along with making the bridge name available in the internal object, it is also now reported in the <source> element of the <interface> state XML (or the <actual> subelement in the internally-stored format). The one oddity about this change is that usually there is a separate union for every different "type" in a higher level object (e.g. in the case of a virDomainNetDef there are separate "network" and "bridge" members of the union that pivots on the type), but in this case network and bridge types both have exactly the same attributes, so the "bridge" member is used for both type==network and type==bridge. --- Changes from V1: * populate actual.brname of an active interface when libvirtd is restarted, to avoid problems during upgrade. (John had asked questions about what happens during a save/restore or a migration; the answer is that everything in the <actual> is discarded as the interface is re-allocated/re-created, so there are no upgrade issues in that case.) src/conf/domain_conf.c | 102 +++++++++++++++++++++++--------------------- src/network/bridge_driver.c | 20 +++++++++ 2 files changed, 73 insertions(+), 49 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b0caa76..c4fb902 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1354,6 +1354,7 @@ virDomainActualNetDefFree(virDomainActualNetDefPtr def) switch (def->type) { case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_NETWORK: VIR_FREE(def->data.bridge.brname); break; case VIR_DOMAIN_NET_TYPE_DIRECT: @@ -7106,9 +7107,12 @@ virDomainActualNetDefParseXML(xmlNodePtr node, goto error; } VIR_FREE(class_id); - } else if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { + } + if (actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE || + actual->type == VIR_DOMAIN_NET_TYPE_NETWORK) { char *brname = virXPathString("string(./source/@bridge)", ctxt); - if (!brname) { + + if (!brname && actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Missing <source> element with bridge name in " "interface's <actual> element")); @@ -17126,60 +17130,59 @@ virDomainHostdevDefFormatCaps(virBufferPtr buf, static int virDomainActualNetDefContentsFormat(virBufferPtr buf, virDomainNetDefPtr def, - const char *typeStr, bool inSubelement, unsigned int flags) { - const char *mode; - - switch (virDomainNetGetActualType(def)) { - case VIR_DOMAIN_NET_TYPE_BRIDGE: - virBufferEscapeString(buf, "<source bridge='%s'/>\n", - virDomainNetGetActualBridgeName(def)); - break; - - case VIR_DOMAIN_NET_TYPE_DIRECT: - virBufferAddLit(buf, "<source"); - virBufferEscapeString(buf, " dev='%s'", - virDomainNetGetActualDirectDev(def)); + int actualType = virDomainNetGetActualType(def); - mode = virNetDevMacVLanModeTypeToString(virDomainNetGetActualDirectMode(def)); - if (!mode) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected source mode %d"), - virDomainNetGetActualDirectMode(def)); - return -1; - } - virBufferAsprintf(buf, " mode='%s'/>\n", mode); - break; - - case VIR_DOMAIN_NET_TYPE_HOSTDEV: + if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) { if (virDomainHostdevDefFormatSubsys(buf, virDomainNetGetActualHostdev(def), flags, true) < 0) { return -1; } - break; - - case VIR_DOMAIN_NET_TYPE_NETWORK: - if (!inSubelement) { - /* the <source> element isn't included in <actual> - * (i.e. when we're putting our output into a subelement - * rather than inline) because the main element has the - * same info already. If we're outputting inline, though, - * we *do* need to output <source>, because the caller - * won't have done it. + } else { + virBufferAddLit(buf, "<source"); + if (def->type == VIR_DOMAIN_NET_TYPE_NETWORK && !inSubelement) { + /* When we're putting our output into the <actual> + * subelement rather than the main <interface>, the + * network name isn't included in the <source> because the + * main interface element's <source> has the same info + * already. If we've been called to output directly into + * the main element's <source> though (the case here - + * "!inSubElement"), we *do* need to output the network + * name, because the caller won't have done it). */ - virBufferEscapeString(buf, "<source network='%s'/>\n", - def->data.network.name); + virBufferEscapeString(buf, " network='%s'", def->data.network.name); } - if (def->data.network.actual && def->data.network.actual->class_id) - virBufferAsprintf(buf, "<class id='%u'/>\n", - def->data.network.actual->class_id); - break; - default: - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected actual net type %s"), typeStr); - return -1; + if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || + actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { + /* actualType == NETWORK includes the name of the bridge + * that is used by the network, whether we are + * "inSubElement" or not. + */ + virBufferEscapeString(buf, " bridge='%s'", + virDomainNetGetActualBridgeName(def)); + } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { + const char *mode; + + virBufferEscapeString(buf, " dev='%s'", + virDomainNetGetActualDirectDev(def)); + mode = virNetDevMacVLanModeTypeToString(virDomainNetGetActualDirectMode(def)); + if (!mode) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected source mode %d"), + virDomainNetGetActualDirectMode(def)); + return -1; + } + virBufferAsprintf(buf, " mode='%s'", mode); + } + + virBufferAddLit(buf, "/>\n"); + } + if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT && + def->data.network.actual && def->data.network.actual->class_id) { + virBufferAsprintf(buf, "<class id='%u'/>\n", + def->data.network.actual->class_id); } if (virNetDevVlanFormat(virDomainNetGetActualVlan(def), buf) < 0) @@ -17227,7 +17230,7 @@ virDomainActualNetDefFormat(virBufferPtr buf, virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); - if (virDomainActualNetDefContentsFormat(buf, def, typeStr, true, flags) < 0) + if (virDomainActualNetDefContentsFormat(buf, def, true, flags) < 0) return -1; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</actual>\n"); @@ -17398,7 +17401,7 @@ virDomainNetDefFormat(virBufferPtr buf, * the standard place... (this is for public reporting of * interface status) */ - if (virDomainActualNetDefContentsFormat(buf, def, typeStr, false, flags) < 0) + if (virDomainActualNetDefContentsFormat(buf, def, false, flags) < 0) return -1; } else { /* ...but if we've asked for the inactive XML (rather than @@ -20751,7 +20754,8 @@ virDomainNetGetActualBridgeName(virDomainNetDefPtr iface) return iface->data.bridge.brname; if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK && iface->data.network.actual && - iface->data.network.actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE) + (iface->data.network.actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE || + iface->data.network.actual->type == VIR_DOMAIN_NET_TYPE_NETWORK)) return iface->data.network.actual->data.bridge.brname; return NULL; } diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 52d2b98..82b301a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3790,6 +3790,15 @@ networkAllocateActualDevice(virDomainDefPtr dom, */ iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_NETWORK; + /* we also store the bridge device + * in iface->data.network.actual->data.bridge for later use + * after the domain's tap device is created (to attach to the + * bridge and set flood/learning mode on the tap device) + */ + if (VIR_STRDUP(iface->data.network.actual->data.bridge.brname, + netdef->bridge) < 0) + goto error; + if (networkPlugBandwidth(network, iface) < 0) goto error; @@ -4126,6 +4135,17 @@ networkNotifyActualDevice(virDomainDefPtr dom, } netdef = network->def; + /* if we're restarting libvirtd after an upgrade from a version + * that didn't save bridge name in actualNetDef for + * actualType==network, we need to copy it in so that it will be + * available in all cases + */ + if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK && + !iface->data.network.actual->data.bridge.brname && + (VIR_STRDUP(iface->data.network.actual->data.bridge.brname, + netdef->bridge) < 0)) + goto error; + if (!iface->data.network.actual || (actualType != VIR_DOMAIN_NET_TYPE_DIRECT && actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV)) { -- 1.9.3

At the time that the network driver allocates a connection to a network, the tap device that will be used hasn't yet been created - that will be done later by qemu (or lxc or whoever) - but if the network has fdb='managed', then when we do get around to creating the tap device, we will need to add an entry for it to the network bridge's fdb (forwarding database) *and* turn off learning and unicast_flood for that tap device in the bridge's sysfs settings. This means that qemu needs to know both the bridge name as well as the setting of fdb, so we either need to create a new API to retrieve that info, or just pass it back in the ActualNetDef that is created during networkAllocateActualDevice. We choose the latter method, since it's already done for the bridge device, and it has the side effect of making the information available in domain status. (NB: in the future, I think that the tap device should actually be created by networkAllocateActualDevice(), as that will solve several other problems, but that is a battle for another day, and this information will still be useful outside the network driver) --- Changes from V1: Only names of attribute and its values src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 ++ src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 4 +++- 4 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c4fb902..3a9f7a5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -51,6 +51,7 @@ #include "netdev_bandwidth_conf.h" #include "netdev_vlan_conf.h" #include "device_conf.h" +#include "network_conf.h" #include "virtpm.h" #include "virstring.h" @@ -7003,6 +7004,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node, char *mode = NULL; char *addrtype = NULL; char *trustGuestRxFilters = NULL; + char *fdb = NULL; if (VIR_ALLOC(actual) < 0) return -1; @@ -7119,6 +7121,15 @@ virDomainActualNetDefParseXML(xmlNodePtr node, goto error; } actual->data.bridge.brname = brname; + fdb = virXPathString("string(./source/@fdb)", ctxt); + if (fdb && + (actual->data.bridge.fdb + = virNetworkBridgeFDBTypeFromString(fdb)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Invalid fdb setting '%s' " + "in domain interface's <actual> element"), fdb); + goto error; + } } bandwidth_node = virXPathNode("./bandwidth", ctxt); @@ -7139,6 +7150,7 @@ virDomainActualNetDefParseXML(xmlNodePtr node, VIR_FREE(mode); VIR_FREE(addrtype); VIR_FREE(trustGuestRxFilters); + VIR_FREE(fdb); virDomainActualNetDefFree(actual); ctxt->node = save_ctxt; @@ -17156,12 +17168,18 @@ virDomainActualNetDefContentsFormat(virBufferPtr buf, } if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || actualType == VIR_DOMAIN_NET_TYPE_NETWORK) { + int fdb = virDomainNetGetActualFDB(def); + /* actualType == NETWORK includes the name of the bridge * that is used by the network, whether we are * "inSubElement" or not. */ virBufferEscapeString(buf, " bridge='%s'", virDomainNetGetActualBridgeName(def)); + if (fdb) { + virBufferAsprintf(buf, " fdb='%s'", + virNetworkBridgeFDBTypeToString(fdb)); + } } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { const char *mode; @@ -20760,6 +20778,17 @@ virDomainNetGetActualBridgeName(virDomainNetDefPtr iface) return NULL; } +int +virDomainNetGetActualFDB(virDomainNetDefPtr iface) +{ + if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK && + iface->data.network.actual && + (iface->data.network.actual->type == VIR_DOMAIN_NET_TYPE_BRIDGE || + iface->data.network.actual->type == VIR_DOMAIN_NET_TYPE_NETWORK)) + return iface->data.network.actual->data.bridge.fdb; + return 0; +} + const char * virDomainNetGetActualDirectDev(virDomainNetDefPtr iface) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 439f3c0..1194f20 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -888,6 +888,7 @@ struct _virDomainActualNetDef { union { struct { char *brname; + int fdb; /* enum virNetworkBridgeFDBType */ } bridge; struct { char *linkdev; @@ -2540,6 +2541,7 @@ int virDomainGraphicsListenSetNetwork(virDomainGraphicsDefPtr def, int virDomainNetGetActualType(virDomainNetDefPtr iface); const char *virDomainNetGetActualBridgeName(virDomainNetDefPtr iface); +int virDomainNetGetActualFDB(virDomainNetDefPtr iface); const char *virDomainNetGetActualDirectDev(virDomainNetDefPtr iface); int virDomainNetGetActualDirectMode(virDomainNetDefPtr iface); virDomainHostdevDefPtr virDomainNetGetActualHostdev(virDomainNetDefPtr iface); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 703cba8..9fc01f3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -331,6 +331,7 @@ virDomainNetGetActualBandwidth; virDomainNetGetActualBridgeName; virDomainNetGetActualDirectDev; virDomainNetGetActualDirectMode; +virDomainNetGetActualFDB; virDomainNetGetActualHostdev; virDomainNetGetActualTrustGuestRxFilters; virDomainNetGetActualType; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 82b301a..3299cd6 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -3790,7 +3790,7 @@ networkAllocateActualDevice(virDomainDefPtr dom, */ iface->data.network.actual->type = VIR_DOMAIN_NET_TYPE_NETWORK; - /* we also store the bridge device + /* we also store the bridge device and fdb settings * in iface->data.network.actual->data.bridge for later use * after the domain's tap device is created (to attach to the * bridge and set flood/learning mode on the tap device) @@ -3798,6 +3798,7 @@ networkAllocateActualDevice(virDomainDefPtr dom, if (VIR_STRDUP(iface->data.network.actual->data.bridge.brname, netdef->bridge) < 0) goto error; + iface->data.network.actual->data.bridge.fdb = netdef->fdb; if (networkPlugBandwidth(network, iface) < 0) goto error; @@ -3813,6 +3814,7 @@ networkAllocateActualDevice(virDomainDefPtr dom, if (VIR_STRDUP(iface->data.network.actual->data.bridge.brname, netdef->bridge) < 0) goto error; + iface->data.network.actual->data.bridge.fdb = netdef->fdb; /* merge virtualports from interface, network, and portgroup to * arrive at actual virtualport to use -- 1.9.3

When the bridge device for a network has fdb='managed' the intent is to configure the bridge to opportunistically turn off promiscuous mode and/or flood mode for as many ports/links on the bridge as possible, thus improving performance and security. The setup required for the bridge itself is: 1) set the "vlan_filtering" property of the bridge device to 1. 2) If the bridge has a "Dummy" tap device used to set a fixed MAC address on the bridge, turn off learning and unicast_flood on this tap (this is needed even though this tap is never IFF_UP, because the kernel ignores the IFF_UP flag of other devices when using their settings to automatically decide whether or not to turn off promiscuous mode for any attached device). This is done both for libvirt-created/managed bridges, and for bridges that are created by the host system config. There is no attempt to turn vlan_filtering off when destroying the network because in the case of a libvirt-created bridge, the bridge is about to be destroyed anyway, and in the case of a system bridge, if the other devices attached to the bridge could operate properly before destroying libvirt's network object, they will continue to operate properly (this is similar to the way that libvirt will enable ip_forwarding whenever a routed/natted network is started, but will never attempt to disable it if they are stopped). --- Changes from V1: only in attribute/value names src/network/bridge_driver.c | 52 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 3299cd6..9b3dc58 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1912,6 +1912,27 @@ networkAddAddrToBridge(virNetworkObjPtr network, return 0; } + +static int +networkStartHandleFDBMode(virNetworkObjPtr network, const char *macTapIfName) +{ + const char *brname = network->def->bridge; + + if (brname && + network->def->fdb == VIR_NETWORK_BRIDGE_FDB_MANAGED) { + if (virNetDevBridgeSetVlanFiltering(brname, true) < 0) + return -1; + if (macTapIfName) { + if (virNetDevBridgePortSetLearning(brname, macTapIfName, false) < 0) + return -1; + if (virNetDevBridgePortSetUnicastFlood(brname, macTapIfName, false) < 0) + return -1; + } + } + return 0; +} + + /* add an IP (static) route to a bridge */ static int networkAddRouteToBridge(virNetworkObjPtr network, @@ -2038,6 +2059,9 @@ networkStartNetworkVirtual(virNetworkObjPtr network) goto err2; } + if (networkStartHandleFDBMode(network, macTapIfName) < 0) + goto err2; + /* Bring up the bridge interface */ if (virNetDevSetOnline(network->def->bridge, 1) < 0) goto err2; @@ -2182,6 +2206,27 @@ static int networkShutdownNetworkVirtual(virNetworkObjPtr network) } +static int +networkStartNetworkBridge(virNetworkObjPtr network) +{ + /* put anything here that needs to be done each time a network of + * type BRIDGE, is started. On failure, undo anything you've done, + * and return -1. On success return 0. + */ + return networkStartHandleFDBMode(network, NULL); +} + +static int +networkShutdownNetworkBridge(virNetworkObjPtr network ATTRIBUTE_UNUSED) +{ + /* put anything here that needs to be done each time a network of + * type BRIDGE is shutdown. On failure, undo anything you've done, + * and return -1. On success return 0. + */ + return 0; +} + + /* networkCreateInterfacePool: * @netdef: the original NetDef from the network * @@ -2345,6 +2390,10 @@ networkStartNetwork(virNetworkObjPtr network) break; case VIR_NETWORK_FORWARD_BRIDGE: + if (networkStartNetworkBridge(network) < 0) + goto cleanup; + break; + case VIR_NETWORK_FORWARD_PRIVATE: case VIR_NETWORK_FORWARD_VEPA: case VIR_NETWORK_FORWARD_PASSTHROUGH: @@ -2411,6 +2460,9 @@ static int networkShutdownNetwork(virNetworkObjPtr network) break; case VIR_NETWORK_FORWARD_BRIDGE: + ret = networkShutdownNetworkBridge(network); + break; + case VIR_NETWORK_FORWARD_PRIVATE: case VIR_NETWORK_FORWARD_VEPA: case VIR_NETWORK_FORWARD_PASSTHROUGH: -- 1.9.3

On 12/02/2014 04:48 PM, John Ferlan wrote: It may seem that way on casual perusal, but no. This function is called from two different places: 1) for networks using a bridge created by libvirt (aka "virtual/managed network"), which also creates a special "dummy tap device" that is created purely for the purpose of setting a stable MAC address for the bridge[*], and 2) for networks that are just referencing a bridge already created by the host system config, usually attached to a physical ethernet, but having no dummy tap device (because the bridge will always get the MAC address of the physical ethernet). In order to preserve the necessary condition of "all ports except the physdev have learning and flood disabled", when a dummy tap device exists (macTapIfname), we must also set learning and flood for that device (just as we do for every other tap device when it is created just prior to passing it to a guest). Even in the case where macTapIfname == NULL (because there *isn't* any "macTap") all the guest tap devices will still have their learning/flood disabled, so there isn't any condition where the bridge's vlan_filtering is set but the ports don't get learning/flood disabled. Some time in the future someone may come up with a valid reason to do that, but I haven't seen it yet, and I don't want to add any unnecessary knobs. *If* there is ever a valid use case for setting vlan_filtering on the bridge while leaving learning/flood on, then at that time we can consider which setting trumps which. In the meantime I don't think the current setup will preclude making a later decision in either direction. [*] there is a long history behind the necessity for the dummy tap device, including at least one BZ filed and resolved a long time ago: https://bugzilla.redhat.com/show_bug.cgi?id=609463 (fixed with commit 5754dbd5). A short description is that each host bridge has one "builtin" port that is automatically presented to the host as a netdev, but doesn't have any MAC address; instead, the bridge's netdev will always acquire the numerically lowest MAC address of all other netdevs attached to the bridge; if the currently lowest numbered netdev is deleted, the MAC address of the bridge (which will also be the MAC address of the default route for all the guests) will change; this in turn will trigger systems like MS Windows to believe that the network has changed, and bring up a "new network detected" wizard. To prevent this, we create a tap device (that is never IFF_UP) with a MAC address guaranteed to be lower than the MAC address of any guest tap device.