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@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