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

Re: [libvirt] [PATCHv3 08/11] qemu: Rework qemuNetworkIfaceConnect



On 05/16/2013 08:49 AM, Michal Privoznik wrote:
> For future work it's better, if tapfd is passed as pointer.
> Moreover, we need to be able to return multiple values now.
> ---
>  src/qemu/qemu_command.c | 89 ++++++++++++++++++++++++++-----------------------
>  src/qemu/qemu_command.h |  4 ++-
>  src/qemu/qemu_hotplug.c |  4 +--
>  3 files changed, 53 insertions(+), 44 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ec66a33..f2c6d47 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -281,11 +281,12 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
>                          virConnectPtr conn,
>                          virQEMUDriverPtr driver,
>                          virDomainNetDefPtr net,
> -                        virQEMUCapsPtr qemuCaps)
> +                        virQEMUCapsPtr qemuCaps,
> +                        int *tapfd,
> +                        int tapfdSize)
>  {
>      char *brname = NULL;
> -    int err;
> -    int tapfd = -1;
> +    int ret = -1;
>      unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
>      bool template_ifname = false;
>      int actualType = virDomainNetGetActualType(net);
> @@ -297,7 +298,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
>          virNetworkPtr network = virNetworkLookupByName(conn,
>                                                         net->data.network.name);
>          if (!network)
> -            return -1;
> +            return ret;
>  
>          active = virNetworkIsActive(network);
>          if (active != 1) {
> @@ -322,18 +323,18 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
>          virFreeError(errobj);
>  
>          if (fail)
> -            return -1;
> +            return ret;
>  
>      } else if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
>          if (!(brname = strdup(virDomainNetGetActualBridgeName(net)))) {
>              virReportOOMError();
> -            return -1;
> +            return ret;
>          }
>      } else {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Network type %d is not supported"),
>                         virDomainNetGetActualType(net));
> -        return -1;
> +        return ret;
>      }
>  
>      if (!net->ifname ||
> @@ -353,55 +354,61 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
>          tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
>      }
>  
> -    if (cfg->privileged)
> -        err = virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac,
> -                                             def->uuid, &tapfd, 1,
> -                                             virDomainNetGetActualVirtPortProfile(net),
> -                                             virDomainNetGetActualVlan(net),
> -                                             tap_create_flags);
> -    else
> -        err = qemuCreateInBridgePortWithHelper(cfg, brname,
> -                                               &net->ifname,
> -                                               &tapfd, tap_create_flags);
> -
> -    virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0);
> -    if (err < 0) {
> -        if (template_ifname)
> -            VIR_FREE(net->ifname);
> -        tapfd = -1;
> +    if (cfg->privileged) {
> +        if (virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac,
> +                                           def->uuid, tapfd, tapfdSize,
> +                                           virDomainNetGetActualVirtPortProfile(net),
> +                                           virDomainNetGetActualVlan(net),
> +                                           tap_create_flags) < 0) {
> +            virDomainAuditNetDevice(def, net, "/dev/net/tun", false);
> +            goto cleanup;
> +        }
> +    } else {
> +        if (qemuCreateInBridgePortWithHelper(cfg, brname,
> +                                             &net->ifname,
> +                                             tapfd, tap_create_flags) < 0) {
> +            virDomainAuditNetDevice(def, net, "/dev/net/tun", false);
> +            goto cleanup;
> +        }


since qemuCreateInBridgePortWithHelper() can't possibly produce more
than a single fd, and the callers to qemuNetworkIfaceConnect have no way
of knowing that, we need to make tapfdSize an int* and change it to 1 here.


>      }
>  
> -    if (cfg->macFilter) {
> -        if ((err = networkAllowMacOnPort(driver, net->ifname, &net->mac))) {
> -            virReportSystemError(err,
> -                 _("failed to add ebtables rule to allow MAC address on '%s'"),
> -                                 net->ifname);
> -        }
> +    virDomainAuditNetDevice(def, net, "/dev/net/tun", true);


When creating the vhost-net file descriptors, you emit one audit message
per fd, but in this case you're only emitting a single audit message, no
matter how many fd's are created. Which is correct?

(My guess is that it's better to just emit a single audit message, since
there is no info specific to each fd anyway, and no extra access gained
by the multiple fds.)


> +
> +    if (cfg->macFilter &&
> +        (ret = networkAllowMacOnPort(driver, net->ifname, &net->mac)) < 0) {
> +        virReportSystemError(ret,
> +                             _("failed to add ebtables rule "
> +                               "to allow MAC address on '%s'"),
> +                             net->ifname);
>      }
>  
> -    if (tapfd >= 0 &&
> -        virNetDevBandwidthSet(net->ifname,
> +    if (virNetDevBandwidthSet(net->ifname,
>                                virDomainNetGetActualBandwidth(net),
>                                false) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("cannot set bandwidth limits on %s"),
>                         net->ifname);
> -        VIR_FORCE_CLOSE(tapfd);
>          goto cleanup;
>      }
>  
> -    if (tapfd >= 0) {
> -        if ((net->filter) && (net->ifname)) {
> -            if (virDomainConfNWFilterInstantiate(conn, def->uuid, net) < 0)
> -                VIR_FORCE_CLOSE(tapfd);
> -        }
> -    }
> +    if (net->filter && net->ifname &&
> +        virDomainConfNWFilterInstantiate(conn, def->uuid, net) < 0)
> +        goto cleanup;
> +
> +
> +    ret = 0;
>  
>  cleanup:
> +    if (ret < 0) {
> +        while (tapfdSize--)
> +            VIR_FORCE_CLOSE(tapfd[tapfdSize]);

(notice that this bit here would fail horribly if we were running
non-privileged, multiple queues had been requested, and there was a
failure somewhere in this function.)


> +        if (template_ifname)
> +            VIR_FREE(net->ifname);
> +    }
>      VIR_FREE(brname);
>      virObjectUnref(cfg);
>  
> -    return tapfd;
> +    return ret;
>  }
>  
>  
> @@ -6488,8 +6495,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>  
>      if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
>          actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> -        tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, qemuCaps);
> -        if (tapfd < 0)
> +        if (qemuNetworkIfaceConnect(def, conn, driver, net,
> +                                    qemuCaps, &tapfd, 1) < 0)
>              goto cleanup;
>      } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
>          tapfd = qemuPhysIfaceConnect(def, driver, net, qemuCaps, vmop);
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index c87b754..adb8d98 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -158,7 +158,9 @@ int qemuNetworkIfaceConnect(virDomainDefPtr def,
>                              virConnectPtr conn,
>                              virQEMUDriverPtr driver,
>                              virDomainNetDefPtr net,
> -                            virQEMUCapsPtr qemuCaps)
> +                            virQEMUCapsPtr qemuCaps,
> +                            int *tapfd,
> +                            int tapfdSize)
>      ATTRIBUTE_NONNULL(2);
>  
>  int qemuPhysIfaceConnect(virDomainDefPtr def,
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index fdc4b24..953339b 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -735,8 +735,8 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>  
>      if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
>          actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
> -        if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net,
> -                                             priv->qemuCaps)) < 0)
> +        if (qemuNetworkIfaceConnect(vm->def, conn, driver, net,
> +                                    priv->qemuCaps, &tapfd, 1) < 0)
>              goto cleanup;
>          iface_connected = true;
>          vhostfdSize = 1;


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