On 05/13/2013 01:23 PM, Michal Privoznik wrote:
> Currently, the function knows how to open a TAP device for a single
> time. However, in case of multiqueue network we need to open it multiple
> times. Moreover, when doing TUNSETIFF ioctl, the IFF_MULTI_QUEUE flag
> shall be requested. This commit changes a behaviour slightly as well.
> Till now it was possible to pass NULL as an @fd address by which caller
> didn't care about returned FD. While this was used only in UML driver,
> the appropriate UML code snippets has been changed as well.
> ---
> src/network/bridge_driver.c | 2 +-
> src/qemu/qemu_command.c | 2 +-
> src/uml/uml_conf.c | 5 +-
> src/util/virnetdevtap.c | 115 ++++++++++++++++++++++++--------------------
> src/util/virnetdevtap.h | 2 +
> 5 files changed, 72 insertions(+), 54 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 99c1316..f81e925 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2444,7 +2444,7 @@ networkStartNetworkVirtual(struct network_driver *driver,
> /* Keep tun fd open and interface up to allow for IPv6 DAD to happen */
> if (virNetDevTapCreateInBridgePort(network->def->bridge,
> &macTapIfName, &network->def->mac,
> - NULL, &tapfd, NULL, NULL,
> + NULL, &tapfd, 1, NULL, NULL,
> VIR_NETDEV_TAP_CREATE_USE_MAC_FOR_BRIDGE |
> VIR_NETDEV_TAP_CREATE_IFUP |
> VIR_NETDEV_TAP_CREATE_PERSIST) < 0) {
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ec061d1..e4bb2b7 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -355,7 +355,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
>
> if (cfg->privileged)
> err = virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac,
> - def->uuid, &tapfd,
> + def->uuid, &tapfd, 1,
> virDomainNetGetActualVirtPortProfile(net),
> virDomainNetGetActualVlan(net),
> tap_create_flags);
> diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
> index 52b705c..3fda7e4 100644
> --- a/src/uml/uml_conf.c
> +++ b/src/uml/uml_conf.c
> @@ -109,6 +109,7 @@ umlConnectTapDevice(virConnectPtr conn,
> const char *bridge)
> {
> bool template_ifname = false;
> + int tapfd;
>
> if (!net->ifname ||
> STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) ||
> @@ -121,7 +122,7 @@ umlConnectTapDevice(virConnectPtr conn,
> }
>
> if (virNetDevTapCreateInBridgePort(bridge, &net->ifname, &net->mac,
> - vm->uuid, NULL,
> + vm->uuid, &tapfd, 1,
> virDomainNetGetActualVirtPortProfile(net),
> virDomainNetGetActualVlan(net),
> VIR_NETDEV_TAP_CREATE_IFUP |
> @@ -139,9 +140,11 @@ umlConnectTapDevice(virConnectPtr conn,
> }
> }
>
> + VIR_FORCE_CLOSE(tapfd);
> return 0;
>
> error:
> + VIR_FORCE_CLOSE(tapfd);
> return -1;
> }
>
> diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
> index 75599db..ee839e3 100644
> --- a/src/util/virnetdevtap.c
> +++ b/src/util/virnetdevtap.c
> @@ -140,7 +140,8 @@ virNetDevProbeVnetHdr(int tapfd)
> /**
> * virNetDevTapCreate:
> * @ifname: the interface name
> - * @tapfd: file descriptor return value for the new tap device
> + * @tapfds: file descriptor return value for the new tap device
> + * @tapfdSize: size of @tapfd
> * @flags: OR of virNetDevTapCreateFlags. Only one flag is recognized:
> *
> * VIR_NETDEV_TAP_CREATE_VNET_HDR
> @@ -148,76 +149,85 @@ virNetDevProbeVnetHdr(int tapfd)
> * 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 closed. The caller must
> - * use virNetDevTapDelete to remove a persistent TAP device when it is no
> - * longer needed.
> + * Creates a tap interface. The caller must use virNetDevTapDelete to
> + * remove a persistent TAP device when it is no longer needed. In case
> + * @tapfdSize is greater than one, multiqueue extension is requested
> + * from kernel.
> *
> * Returns 0 in case of success or -1 on failure.
> */
> int virNetDevTapCreate(char **ifname,
> int *tapfd,
> + int tapfdSize,
> unsigned int flags)
> {
> - int fd;
> + int i;
> struct ifreq ifr;
> int ret = -1;
>
> - if ((fd = open("/dev/net/tun", O_RDWR)) < 0) {
> - virReportSystemError(errno, "%s",
> - _("Unable to open /dev/net/tun, is tun module loaded?"));
> - return -1;
> - }
> -
> memset(&ifr, 0, sizeof(ifr));
> + for (i = 0; i < tapfdSize; i++) {
> + int fd;
>
> - ifr.ifr_flags = IFF_TAP|IFF_NO_PI;
> + if ((fd = open("/dev/net/tun", O_RDWR)) < 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to open /dev/net/tun, is tun module loaded?"));
> + goto cleanup;
> + }
If there is a failure anywhere between here and the next time we get to
the top of the loop, we will leak fd.
> +
> + memset(&ifr, 0, sizeof(ifr));
> +
> + ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
> + /* If tapfdSize is greater than one, request multiqueue */
> + if (tapfdSize > 1)
> + ifr.ifr_flags |= IFF_MULTI_QUEUE;
>
> # ifdef IFF_VNET_HDR
> - if ((flags & VIR_NETDEV_TAP_CREATE_VNET_HDR) &&
> - virNetDevProbeVnetHdr(fd))
> - ifr.ifr_flags |= IFF_VNET_HDR;
> + if ((flags & VIR_NETDEV_TAP_CREATE_VNET_HDR) &&
> + virNetDevProbeVnetHdr(fd))
> + ifr.ifr_flags |= IFF_VNET_HDR;
> # endif
>
> - if (virStrcpyStatic(ifr.ifr_name, *ifname) == NULL) {
> - virReportSystemError(ERANGE,
> - _("Network interface name '%s' is too long"),
> - *ifname);
> - goto cleanup;
> + if (virStrcpyStatic(ifr.ifr_name, *ifname) == NULL) {
> + virReportSystemError(ERANGE,
> + _("Network interface name '%s' is too long"),
> + *ifname);
> + goto cleanup;
>
> - }
> + }
>
> - if (ioctl(fd, TUNSETIFF, &ifr) < 0) {
> - virReportSystemError(errno,
> - _("Unable to create tap device %s"),
> - NULLSTR(*ifname));
> - goto cleanup;
> - }
> + if (ioctl(fd, TUNSETIFF, &ifr) < 0) {
> + virReportSystemError(errno,
> + _("Unable to create tap device %s"),
> + NULLSTR(*ifname));
> + goto cleanup;
> + }
>
> - if ((flags & VIR_NETDEV_TAP_CREATE_PERSIST) &&
> - (errno = ioctl(fd, TUNSETPERSIST, 1))) {
> - virReportSystemError(errno,
> - _("Unable to set tap device %s to persistent"),
> - NULLSTR(*ifname));
> - goto cleanup;
> - }
> + if (i == 0) {
> + /* In case we are looping more than once, set other
> + * TAPs to have the same name */
> + VIR_FREE(*ifname);
> + if (ifr.ifr_name && VIR_STRDUP(*ifname, ifr.ifr_name) < 0)
> + goto cleanup;
> + }
>
> - VIR_FREE(*ifname);
> - if (!(*ifname = strdup(ifr.ifr_name))) {
> - virReportOOMError();
> - goto cleanup;
> + if ((flags & VIR_NETDEV_TAP_CREATE_PERSIST) &&
> + (errno = ioctl(fd, TUNSETPERSIST, 1))) {
> + virReportSystemError(errno,
> + _("Unable to set tap device %s to persistent"),
> + NULLSTR(*ifname));
> + goto cleanup;
> + }
> + tapfd[i] = fd;
> }
> - if (tapfd)
> - *tapfd = fd;
> - else
> - VIR_FORCE_CLOSE(fd);
>
> ret = 0;
>
> cleanup:
> - if (ret < 0)
> - VIR_FORCE_CLOSE(fd);
> + if (ret < 0) {
> + for (i = 0; i < tapfdSize; i++)
> + VIR_FORCE_CLOSE(tapfd[i]);
> + }
If there is a failure when only some of the items in tapfd have been
filled in, this could be very catastrophic (depending on what happens to
be in the positions that haven't been set yet). You should only close
the ones that have valid data in them.
>
> return ret;
> }
> @@ -266,6 +276,7 @@ cleanup:
> #else /* ! TUNSETIFF */
> int virNetDevTapCreate(char **ifname ATTRIBUTE_UNUSED,
> int *tapfd ATTRIBUTE_UNUSED,
> + int tapfdSize ATTRIBUTE_UNUSED,
> unsigned int flags ATTRIBUTE_UNUSED)
> {
> virReportSystemError(ENOSYS, "%s",
> @@ -286,7 +297,8 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED)
> * @brname: the bridge name
> * @ifname: the interface name (or name template)
> * @macaddr: desired MAC address
> - * @tapfd: file descriptor return value for the new tap device
> + * @tapfd: array of file descriptor return value for the new tap device
> + * @tapfdSize: size of @tapfd
> * @virtPortProfile: bridge/port specific configuration
> * @flags: OR of virNetDevTapCreateFlags:
>
> @@ -314,6 +326,7 @@ int virNetDevTapCreateInBridgePort(const char *brname,
> const virMacAddrPtr macaddr,
> const unsigned char *vmuuid,
> int *tapfd,
> + int tapfdSize,
> virNetDevVPortProfilePtr virtPortProfile,
> virNetDevVlanPtr virtVlan,
> unsigned int flags)
> @@ -321,7 +334,7 @@ int virNetDevTapCreateInBridgePort(const char *brname,
> virMacAddr tapmac;
> char macaddrstr[VIR_MAC_STRING_BUFLEN];
>
> - if (virNetDevTapCreate(ifname, tapfd, flags) < 0)
> + if (virNetDevTapCreate(ifname, tapfd, tapfdSize, flags) < 0)
> return -1;
>
> /* We need to set the interface MAC before adding it
> @@ -372,9 +385,9 @@ int virNetDevTapCreateInBridgePort(const char *brname,
>
> return 0;
>
> - error:
> - if (tapfd)
> - VIR_FORCE_CLOSE(*tapfd);
> +error:
> + while (tapfdSize)
> + VIR_FORCE_CLOSE(tapfd[--tapfdSize]);
>
> return -1;
> }
> diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h
> index 6bfc80c..cb6c284 100644
> --- a/src/util/virnetdevtap.h
> +++ b/src/util/virnetdevtap.h
> @@ -29,6 +29,7 @@
>
> int virNetDevTapCreate(char **ifname,
> int *tapfd,
> + int tapfdSize,
> unsigned int flags)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
>
> @@ -55,6 +56,7 @@ int virNetDevTapCreateInBridgePort(const char *brname,
> const virMacAddrPtr macaddr,
> const unsigned char *vmuuid,
> int *tapfd,
> + int tapfdSize,
> virNetDevVPortProfilePtr virtPortProfile,
> virNetDevVlanPtr virtVlan,
> unsigned int flags)
Otherwise looks okay.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list