Re: [libvirt] [PATCH] Fix dnsmasq/radvd on bridges not being able to be bound to IPv6 address on "recent" kernels

From: Benjamin Cama <benjamin.cama(a)telecom-bretagne.eu&gt; Le mercredi 20 juin 2012 à 11:05 +0100, Daniel P. Berrange a écrit : > I think you also need to have a VIR_FOFRCE_CLOSE(tapfd) in the > very last of the cleanup jump labels. Since between the time > we open the tapfd & close it, we might have done a 'goto err1' > or similar. Yes, I forgot that. New patch following. > Looks reasonable apart from the cleanup bug. Thanks. BTW, if this patch is commited, I'm already in AUTHORS under another email address, which is OK. --- This is my attempt at resolving the merge conflict, but I'm not confident enough with the original patch, nor with my test environment, to see if I actually solved anything (not to mention the commit message used by 'git am' is horrible). Is it worth using this version?

src/network/bridge_driver.c | 22 ++++++++++++++++++++-- src/uml/uml_conf.c | 3 ++- src/util/virnetdevtap.c | 24 ++++++++++++++---------- src/util/virnetdevtap.h | 2 ++ 4 files changed, 38 insertions(+), 13 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index fce1739..33aba74 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -65,6 +65,7 @@ #include "virnetdevtap.h" #include "virnetdevvportprofile.h" #include "virdbus.h" +#include "virfile.h" #define NETWORK_PID_DIR LOCALSTATEDIR "/run/libvirt/network" #define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network" @@ -993,6 +994,7 @@ networkRadvdConfContents(virNetworkObjPtr network, char **configstr) " AdvSendAdvert on;\n" " AdvManagedFlag off;\n" " AdvOtherConfigFlag off;\n" + " IgnoreIfMissing on;\n"

"\n", network->def->bridge); @@ -2061,6 +2063,7 @@ networkStartNetworkVirtual(struct network_driver *driver, virErrorPtr save_err = NULL; virNetworkIpDefPtr ipdef; char *macTapIfName = NULL; + int tapfd = -1; /* Check to see if any network IP collides with an existing route */ if (networkCheckRouteCollision(network) < 0) @@ -2082,10 +2085,13 @@ networkStartNetworkVirtual(struct network_driver *driver, virReportOOMError(); goto err0; } + /* Keep tun fd open and interface up to allow for IPv6 DAD to happen */ if (virNetDevTapCreateInBridgePort(network->def->bridge, &macTapIfName, &network->def->mac, - NULL, NULL, NULL, NULL, - VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE) < 0) { + NULL, &tapfd, NULL, NULL, + VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE | + VIR_NETDEV_TAP_CREATE_IFUP | + VIR_NETDEV_TAP_CREATE_PERSIST) < 0) { VIR_FREE(macTapIfName); goto err0; } @@ -2149,6 +2155,16 @@ networkStartNetworkVirtual(struct network_driver *driver, if (v6present && networkStartRadvd(network) < 0) goto err4; + /* DAD has happened (dnsmasq waits for it),

dnsmasq is now bound to the + * bridge's IPv6 address, and radvd to the interface, so we can now set the + * dummy tun down. + */ + if (tapfd >= 0) { + if (virNetDevSetOnline(macTapIfName, false) < 0) + goto err4; + VIR_FORCE_CLOSE(tapfd); + } + if (virNetDevBandwidthSet(network->def->bridge, network->def->bandwidth) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot set bandwidth limits on %s"), @@ -2187,6 +2203,8 @@ networkStartNetworkVirtual(struct network_driver *driver, save_err = virSaveLastError(); if (macTapIfName) { + if (tapfd >= 0) + VIR_FORCE_CLOSE(tapfd); ignore_value(virNetDevTapDelete(macTapIfName)); VIR_FREE(macTapIfName); } diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index a317bcc..e8b8779 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -142,7 +142,8 @@ umlConnectTapDevice(virConnectPtr conn, vm->uuid, NULL, virDomainNetGetActualVirtPortProfile(net), virDomainNetGetActualVlan(net), - VIR_NETDEV_TAP_CREATE_IFUP) < 0) { + VIR_NETDEV_TAP_CREATE_IFUP | + VIR_NETDEV_TAP_CREATE_PERSIST) < 0) { if (template_ifname) VIR_FREE(net->ifname); goto error; diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 9f2dca1..0eadd6c 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -112,18 +112,20 @@ virNetDevProbeVnetHdr(int tapfd) * * VIR_NETDEV_TAP_CREATE_VNET_HDR * - Enable IFF_VNET_HDR on the tap device + * VIR_NETDEV_TAP_CREATE_PERSIST + * - The device will persist after the file descriptor is closed * * Creates a tap interface. - * If the @tapfd parameter is supplied, the open tap device file - * descriptor will be returned, otherwise the TAP device will be made - * persistent and closed. The caller must use virNetDevTapDelete to - * remove a persistent TAP devices when it is no longer needed. + * If the @tapfd parameter is supplied, the open tap device file descriptor + * will be returned, otherwise the TAP device will be closed. The caller must + * use virNetDevTapDelete to remove a persistent TAP device when it is no + * longer needed. * * Returns 0 in case of success or -1 on failure. */ int virNetDevTapCreate(char **ifname, int *tapfd, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { int fd; struct ifreq ifr; @@ -160,7 +162,7 @@ int virNetDevTapCreate(char **ifname, goto cleanup; } - if (!tapfd && + if ((flags & VIR_NETDEV_TAP_CREATE_PERSIST) && (errno = ioctl(fd, TUNSETPERSIST, 1))) { virReportSystemError(errno, _("Unable to set tap device %s to persistent"), @@ -261,14 +263,16 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED) * - Enable IFF_VNET_HDR on the tap device * VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE * - Set this interface's MAC as the bridge's MAC address + * VIR_NETDEV_TAP_CREATE_PERSIST + * - The device will persist after the file descriptor is closed * * This function creates a new tap device on a bridge. @ifname can be either * a fixed name or a name template with '%d' for dynamic name allocation. * in either case the final name for the bridge will be stored in @ifname. - * If the @tapfd parameter is supplied, the open tap device file - * descriptor will be returned, otherwise the TAP device will be made - * persistent and closed. The caller must use virNetDevTapDelete to remove - * a persistent TAP devices when it is no longer needed. + * If the @tapfd parameter is supplied, the open tap device file descriptor + * will be returned, otherwise the TAP device will be closed. The caller must + * use virNetDevTapDelete to remove a persistent TAP device when it is no + * longer needed. * * Returns 0 in case of success or -1 on failure */ diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h index fa6a647..980db61 100644 --- a/src/util/virnetdevtap.h +++ b/src/util/virnetdevtap.h @@ -43,6 +43,8 @@ typedef enum { VIR_NETDEV_TAP_CREATE_VNET_HDR = 1 << 1, /* Set this interface's MAC as the bridge's MAC address */ VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE = 1 << 2, + /* The device will persist after the file descriptor is closed */ + VIR_NETDEV_TAP_CREATE_PERSIST = 1 << 3, } virNetDevTapCreateFlags; int virNetDevTapCreateInBridgePort(const char *brname,