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

On 09/21/2012 02:45 PM, Eric Blake wrote: > 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?

Whatever we push should have the commit log message from the first version of the patch. That gave a very good description of the problem, and why the patch is IFUPing a tap device that's connected to the bridge, but has no IP address (v6 or v4), no backend connection, and no traffic flowing over it. It seems very counterintuitive until you read the explanation in the original 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" This was added to the patch after V2, and I don't see any explanation why. Does this just silence the complaint log messages when the dummy tap is down?

> "\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), Hmm. dnsmasq waits for it because it wants to listen for DNS packets on the bridge's IPv6 address, right? Okay, so the timing problem mentioned in the earlier messages is a thing of the past.

> 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, This has my ACK as long as: 1) the original commit message (with some modifications) is used, 2) the reason for adding the "IgnoreIfMissing" line is added to that commit message (and maybe as a comment in the code).