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
> ---
> 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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
