[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On Tue, Jun 19, 2012 at 07:52:43PM +0200, Benjamin Cama wrote:
> Hi again,
> 
> I forgot to port my patch to the latest git master… Here is a try, not
> tested, not even compiled; but it's less intrusive regarding the API
> thanks to the new “flags” argument.
> 
> ---
>  src/network/bridge_driver.c |   19 +++++++++++++++++--
>  src/uml/uml_conf.c          |    2 +-
>  src/util/virnetdevtap.c     |   24 ++++++++++++++----------
>  src/util/virnetdevtap.h     |    2 ++
>  4 files changed, 34 insertions(+), 13 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 79d3010..8f45bd7 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"
> @@ -1744,6 +1745,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 +1767,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 +1833,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"),

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.


> 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,
> 


Looks reasonable apart from the cleanup bug.

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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]