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

On Wed, Jun 20, 2012 at 01:49:30PM +0200, Benjamin Cama wrote: > 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. > > --- > src/network/bridge_driver.c | 22 ++++++++++++++++++++-- > src/uml/uml_conf.c | 2 +- > src/util/virnetdevtap.c | 24 ++++++++++++++---------- > src/util/virnetdevtap.h | 2 ++ > 4 files changed, 37 insertions(+), 13 deletions(-) > > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index 79d3010..e9f3c90 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -62,6 +62,7 @@ > #include "virnetdev.h" > #include "virnetdevbridge.h" > #include "virnetdevtap.h" > +#include "virfile.h" > > #define NETWORK_PID_DIR LOCALSTATEDIR "/run/libvirt/network" > #define NETWORK_STATE_DIR LOCALSTATEDIR "/lib/libvirt/network" > @@ -840,6 +841,7 @@ networkStartRadvd(virNetworkObjPtr network) > " AdvSendAdvert on;\n" > " AdvManagedFlag off;\n" > " AdvOtherConfigFlag off;\n" > + " IgnoreIfMissing on;\n" > "\n", > network->def->bridge); > for (ii = 0; > @@ -1744,6 +1746,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) > @@ -1765,10 +1768,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, > - VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE) < 0) { > + NULL, &tapfd, 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; > } > @@ -1828,6 +1834,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) { > networkReportError(VIR_ERR_INTERNAL_ERROR, > _("cannot set bandwidth limits on %s"), > @@ -1866,6 +1882,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 79b249d..c9b5044 100644 > --- a/src/uml/uml_conf.c > +++ b/src/uml/uml_conf.c > @@ -141,7 +141,7 @@ umlConnectTapDevice(virConnectPtr conn, > if (virNetDevTapCreateInBridgePort(bridge, &net->ifname, net->mac, > vm->uuid, NULL, > virDomainNetGetActualVirtPortProfile(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 5d21164..45ff20f 100644 > --- a/src/util/virnetdevtap.c > +++ b/src/util/virnetdevtap.c > @@ -118,18 +118,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 an errno code in case of failure. > */ > int virNetDevTapCreate(char **ifname, > int *tapfd, > - unsigned int flags ATTRIBUTE_UNUSED) > + unsigned int flags) > { > int fd; > struct ifreq ifr; > @@ -166,7 +168,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"), > @@ -267,14 +269,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 d9a3593..f1355ba 100644 > --- a/src/util/virnetdevtap.h > +++ b/src/util/virnetdevtap.h > @@ -42,6 +42,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, > ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|