[libvirt] [PATCH] qemu: Handle huge number of queues correctly

[Martin Kletzander]

On 09/02/2013 04:25 PM, Michal Privoznik wrote:
> Currently, kernel supports up to 8 queues for a multiqueue tap device.
> However, if user tries to enter a huge number (e.g. one million) the tap
> allocation fails, as expected. But what is not expected is the log full
> of warnings:
> 
>     warning : virFileClose:83 : Tried to close invalid fd 0
> 
> The problem is, upon error we iterate over an array of FDs (handlers to
> queues) and VIR_FORCE_CLOSE() over each item. However, the array is
> pre-filled with zeros. Hence, we repeatedly close stdin. Ouch.
> But there's more. The queues allocation is done in virNetDevTapCreate()
> which cleans up the FDs in case of error. Then, its caller, the
> virNetDevTapCreateInBridgePort() iterates over the FD array and tries to
> close them too. And so does qemuNetworkIfaceConnect() and
> qemuBuildInterfaceCommandLine().
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_command.c | 10 +++++++---
>  src/util/virnetdevtap.c |  4 ++--
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index f8fccea..73a13b3 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -405,7 +405,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
>  cleanup:
>      if (ret < 0) {
>          size_t i;
> -        for (i = 0; i < *tapfdSize; i++)
> +        for (i = 0; i < *tapfdSize && tapfd[i] >= 0; i++)
>              VIR_FORCE_CLOSE(tapfd[i]);
>          if (template_ifname)
>              VIR_FREE(net->ifname);
> @@ -7241,6 +7241,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>              VIR_ALLOC_N(tapfdName, tapfdSize) < 0)
>              goto cleanup;
>  
> +        memset(tapfd, -1, tapfdSize * sizeof(tapfd[0]));
> +
>          if (qemuNetworkIfaceConnect(def, conn, driver, net,
>                                      qemuCaps, tapfd, &tapfdSize) < 0)
>              goto cleanup;
> @@ -7268,6 +7270,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>              VIR_ALLOC_N(vhostfdName, vhostfdSize))
>              goto cleanup;
>  
> +        memset(vhostfd, -1, vhostfdSize * sizeof(vhostfd[0]));
> +

Just a question; are these non-standard settings necessary when we close
only non-zero fds?

>          if (qemuOpenVhostNet(def, net, qemuCaps, vhostfd, &vhostfdSize) < 0)
>              goto cleanup;
>      }
> @@ -7329,13 +7333,13 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>  cleanup:
>      if (ret < 0)
>          virDomainConfNWFilterTeardown(net);
> -    for (i = 0; tapfd && i < tapfdSize; i++) {
> +    for (i = 0; tapfd && i < tapfdSize && tapfd[i] >= 0; i++) {
>          if (ret < 0)
>              VIR_FORCE_CLOSE(tapfd[i]);
>          if (tapfdName)
>              VIR_FREE(tapfdName[i]);
>      }
> -    for (i = 0; vhostfd && i < vhostfdSize; i++) {
> +    for (i = 0; vhostfd && i < vhostfdSize && vhostfd[i] >= 0; i++) {
>          if (ret < 0)
>              VIR_FORCE_CLOSE(vhostfd[i]);
>          if (vhostfdName)
> diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
> index 42e8dfe..dc2c224 100644
> --- a/src/util/virnetdevtap.c
> +++ b/src/util/virnetdevtap.c
> @@ -498,8 +498,8 @@ int virNetDevTapCreateInBridgePort(const char *brname,
>      return 0;
>  
>  error:
> -    while (tapfdSize)
> -        VIR_FORCE_CLOSE(tapfd[--tapfdSize]);
> +    while (tapfdSize && tapfd[--tapfdSize] >= 0)
> +        VIR_FORCE_CLOSE(tapfd[tapfdSize]);

This will not close anything in case the array looks like this:
[ 4, 5, 6, 7, 0, 0, 0, 0 ]

I suggest modifying it to the same for as in those other cases.

ACK with that fixed,
Martin