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

Re: [libvirt] [PATCH] macvtap teardown rework



On Wed, Feb 17, 2010 at 03:05:53PM -0500, Stefan Berger wrote:
> I have reworked and simplified the teardown of the macvtap device.
> Basically all devices with the same MAC address and link device are kept
> alive and not attempted to be torn down. If a macvtap device linked to a
> physical interface with a certain MAC address 'M' is to be created it
> will automatically fail if the interface is 'up'ed and another macvtap
> with the same properties (MAC addr 'M', link dev) happens to be 'up'.
> This will prevent the VM from starting or the device from being attached
> to a running VM. Stale interfaces are assumed to be there for some
> reason and not stem from libvirt.
> 
> In the VM shutdown path I am assuming that an interface name is always
> available so that if the device type is DIRECT it can be torn down using
> its name.
> 
> Signed-off-by: Stefan Berger <stefanb us ibm com>
> 

> Index: libvirt-macvtap/src/util/macvtap.h
> ===================================================================
> --- libvirt-macvtap.orig/src/util/macvtap.h
> +++ libvirt-macvtap/src/util/macvtap.h
> @@ -35,8 +35,7 @@ int openMacvtapTap(virConnectPtr conn,
>                     int mode,
>                     char **res_ifname);
>  
> -int delMacvtapByMACAddress(const unsigned char *macaddress,
> -                           int *hasBusyDev);
> +void delMacvtap(const char *name);
>  
>  #endif /* WITH_MACVTAP */
>  
> Index: libvirt-macvtap/src/util/macvtap.c
> ===================================================================
> --- libvirt-macvtap.orig/src/util/macvtap.c
> +++ libvirt-macvtap/src/util/macvtap.c
> @@ -447,15 +447,14 @@ buffer_too_small:
>  
>  
>  static int
> -link_del(const char *type,
> -         const char *name)
> +link_del(const char *name)
>  {
>      int rc = 0;
>      char nlmsgbuf[256];
>      struct nlmsghdr *nlm = (struct nlmsghdr *)nlmsgbuf, *resp;
>      struct nlmsgerr *err;
>      char rtattbuf[64];
> -    struct rtattr *rta, *rta1;
> +    struct rtattr *rta;
>      struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC };
>      char *recvbuf = NULL;
>      int recvbuflen;
> @@ -467,23 +466,6 @@ link_del(const char *type,
>      if (!nlAppend(nlm, sizeof(nlmsgbuf), &ifinfo, sizeof(ifinfo)))
>          goto buffer_too_small;
>  
> -    rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_LINKINFO, NULL, 0);
> -    if (!rta)
> -        goto buffer_too_small;
> -
> -    if (!(rta1 = nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len)))
> -        goto buffer_too_small;
> -
> -    rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_INFO_KIND,
> -                       type, strlen(type));
> -    if (!rta)
> -        goto buffer_too_small;
> -
> -    if (!nlAppend(nlm, sizeof(nlmsgbuf), rtattbuf, rta->rta_len))
> -        goto buffer_too_small;
> -
> -    rta1->rta_len = (char *)nlm + nlm->nlmsg_len - (char *)rta1;
> -
>      rta = rtattrCreate(rtattbuf, sizeof(rtattbuf), IFLA_IFNAME,
>                         name, strlen(name)+1);
>      if (!rta)
> @@ -633,7 +615,8 @@ macvtapModeFromInt(enum virDomainNetdevM
>  }
>  
>  
> -/* openMacvtapTap:
> +/**
> + * openMacvtapTap:
>   * Create an instance of a macvtap device and open its tap character
>   * device.
>   * @conn: Pointer to virConnect object
> @@ -707,14 +690,17 @@ create_name:
>      rc = ifUp(cr_ifname, 1);
>      if (rc != 0) {
>          virReportSystemError(errno,
> -                             _("cannot 'up' interface %s"), cr_ifname);
> +                             _("cannot 'up' interface %s -- another "
> +                             "macvtap device may be 'up' and have the same "
> +                             "MAC address"),
> +                             cr_ifname);
>          rc = -1;
>          goto link_del_exit;
>      }
>  
>      rc = openTap(cr_ifname, 10);
>  
> -    if (rc > 0)
> +    if (rc >= 0)
>          *res_ifname = strdup(cr_ifname);
>      else
>          goto link_del_exit;
> @@ -722,79 +708,22 @@ create_name:
>      return rc;
>  
>  link_del_exit:
> -    link_del(type, ifname);
> +    link_del(cr_ifname);
>  
>      return rc;
>  }
>  
>  
> -/* Delete a macvtap type of interface given the MAC address. This
> - * function will delete all macvtap type of interfaces that have the
> - * given MAC address.
> - * @macaddress : Pointer to 6 bytes holding the MAC address of the
> - *    macvtap device(s) to destroy
> +/**
> + * delMacvtapByName:
> + * @ifname : The name of the macvtap interface
>   *
> - * Returns 0 in case of success, negative value in case of error.
> + * Delete an interface given its name.
>   */
> -int
> -delMacvtapByMACAddress(const unsigned char *macaddress,
> -                       int *hasBusyDev)
> +void
> +delMacvtap(const char *ifname)
>  {
> -    struct ifreq ifr;
> -    FILE *file;
> -    char *ifname, *pos;
> -    char buffer[1024];
> -    off_t oldpos = 0;
> -    int tapfd;
> -
> -    *hasBusyDev = 0;
> -
> -    file = fopen("/proc/net/dev", "r");
> -
> -    if (!file) {
> -        virReportSystemError(errno, "%s",
> -                             _("cannot open file to read network interfaces "
> -                             "from"));
> -        return -1;
> -    }
> -
> -    int sock = socket(AF_INET, SOCK_DGRAM, 0);
> -    if (sock < 0) {
> -        virReportSystemError(errno, "%s",
> -                             _("cannot open socket"));
> -        goto sock_err;
> -    }
> -
> -    while (NULL != (ifname = fgets(buffer, sizeof(buffer), file))) {
> -        if (c_isspace(ifname[0]))
> -            ifname++;
> -        if ((pos = strchr(ifname, ':')) != NULL) {
> -            pos[0] = 0;
> -            if (virStrncpy(ifr.ifr_name, ifname, strlen(ifname),
> -                           sizeof(ifr.ifr_name)) == NULL)
> -                continue;
> -            if (ioctl(sock, SIOCGIFHWADDR, (char *)&ifr) >= 0) {
> -                if (memcmp(&ifr.ifr_hwaddr.sa_data[0], macaddress, 6) == 0) {
> -                    tapfd = openTap(ifname, 0);
> -                    if (tapfd > 0) {
> -                        close(tapfd);
> -                        ifUp(ifname, 0);
> -                        if (link_del("macvtap", ifname) == 0)
> -                            fseeko(file, oldpos, SEEK_SET);
> -                    } else {
> -                        *hasBusyDev = 1;
> -                    }
> -                }
> -            }
> -        }
> -        oldpos = ftello(file);
> -    }
> -
> -    close(sock);
> -sock_err:
> -    fclose(file);
> -
> -    return 0;
> +    link_del(ifname);
>  }
>  
>  #endif
> Index: libvirt-macvtap/src/qemu/qemu_driver.c
> ===================================================================
> --- libvirt-macvtap.orig/src/qemu/qemu_driver.c
> +++ libvirt-macvtap/src/qemu/qemu_driver.c
> @@ -2942,17 +2942,6 @@ static void qemudShutdownVMDaemon(struct
>          }
>      }
>  
> -#if WITH_MACVTAP
> -    def = vm->def;
> -    for (i = 0; i < def->nnets; i++) {
> -        virDomainNetDefPtr net = def->nets[i];
> -        if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
> -            int dummy;
> -            delMacvtapByMACAddress(net->mac, &dummy);
> -        }
> -    }
> -#endif
> -
>      if (virKillProcess(vm->pid, 0) == 0 &&
>          virKillProcess(vm->pid, SIGTERM) < 0)
>          virReportSystemError(errno,
> @@ -2999,6 +2988,17 @@ static void qemudShutdownVMDaemon(struct
>  
>      qemuDomainReAttachHostDevices(driver, vm->def);
>  
> +#if WITH_MACVTAP
> +    def = vm->def;
> +    for (i = 0; i < def->nnets; i++) {
> +        virDomainNetDefPtr net = def->nets[i];
> +        if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
> +            if (net->ifname)
> +                delMacvtap(net->ifname);
> +        }
> +    }
> +#endif
> +
>  retry:
>      if ((ret = qemuRemoveCgroup(driver, vm, 0)) < 0) {
>          if (ret == -EBUSY && (retries++ < 5)) {
> @@ -6347,8 +6347,8 @@ qemudDomainDetachNetDevice(struct qemud_
>  
>  #if WITH_MACVTAP
>      if (detach->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
> -        int dummy;
> -        delMacvtapByMACAddress(detach->mac, &dummy);
> +        if (detach->ifname)
> +            delMacvtap(detach->ifname);
>      }
>  #endif
>  
> Index: libvirt-macvtap/src/qemu/qemu_conf.c
> ===================================================================
> --- libvirt-macvtap.orig/src/qemu/qemu_conf.c
> +++ libvirt-macvtap/src/qemu/qemu_conf.c
> @@ -1439,15 +1439,6 @@ qemudPhysIfaceConnect(virConnectPtr conn
>      int rc;
>  #if WITH_MACVTAP
>      char *res_ifname = NULL;
> -    int hasBusyDev = 0;
> -
> -    delMacvtapByMACAddress(net->mac, &hasBusyDev);
> -
> -    if (hasBusyDev) {
> -        virReportSystemError(errno, "%s",
> -                             _("A macvtap with the same MAC address is in use"));
> -        return -1;
> -    }
>  
>      rc = openMacvtapTap(conn, net->ifname, net->mac, linkdev, brmode,
>                          &res_ifname);
> @@ -1460,7 +1451,6 @@ qemudPhysIfaceConnect(virConnectPtr conn
>      (void)net;
>      (void)linkdev;
>      (void)brmode;
> -    (void)conn;
>      qemuReportError(VIR_ERR_INTERNAL_ERROR,
>                      "%s", _("No support for macvtap device"));
>      rc = -1;
> Index: libvirt-macvtap/src/libvirt_macvtap.syms
> ===================================================================
> --- libvirt-macvtap.orig/src/libvirt_macvtap.syms
> +++ libvirt-macvtap/src/libvirt_macvtap.syms
> @@ -2,4 +2,4 @@
>  
>  # macvtap.h
>  openMacvtapTap;
> -delMacvtapByMACAddress;
> +delMacvtap;


ACK, this looks better


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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