[libvirt] [PATCH] qemuOpenVhostNet: Decrease vhostfdSize on open failure
Currently, if there's an error opening /dev/vhost-net (e.g. because it doesn't exist) but it's not required we proceed with vhostfd array filled with -1 and vhostfdSize unchanged. Later, when constructing the qemu command line only non-negative items within vhostfd array are taken into account. This means, vhostfdSize may be greater than the actual count of non-negative items in vhostfd array. This results in improper command line arguments being generated, e.g.: -netdev tap,fd=21,id=hostnet0,vhost=on,vhostfd=(null) --- src/qemu/qemu_command.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 434f5a7..d969f88 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -486,6 +486,8 @@ qemuOpenVhostNet(virDomainDefPtr def, "but is unavailable")); goto error; } + i--; + (*vhostfdSize)--; } } virDomainAuditNetDevice(def, net, "/dev/vhost-net", *vhostfdSize); @@ -6560,12 +6562,10 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, } for (i = 0; i < vhostfdSize; i++) { - if (vhostfd[i] >= 0) { - virCommandTransferFD(cmd, vhostfd[i]); - if (virAsprintf(&vhostfdName[i], "%d", vhostfd[i]) < 0) { - virReportOOMError(); - goto cleanup; - } + virCommandTransferFD(cmd, vhostfd[i]); + if (virAsprintf(&vhostfdName[i], "%d", vhostfd[i]) < 0) { + virReportOOMError(); + goto cleanup; } } -- 1.8.2.1
On 05/24/2013 08:50 AM, Michal Privoznik wrote:
Currently, if there's an error opening /dev/vhost-net (e.g. because it doesn't exist) but it's not required we proceed with vhostfd array filled with -1 and vhostfdSize unchanged. Later, when constructing the qemu command line only non-negative items within vhostfd array are taken into account. This means, vhostfdSize may be greater than the actual count of non-negative items in vhostfd array. This results in improper command line arguments being generated, e.g.:
-netdev tap,fd=21,id=hostnet0,vhost=on,vhostfd=(null) --- src/qemu/qemu_command.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 434f5a7..d969f88 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -486,6 +486,8 @@ qemuOpenVhostNet(virDomainDefPtr def, "but is unavailable")); goto error; } + i--; + (*vhostfdSize)--;
This will still go back through the loop again and try to open another. I would instead just set vhostfdSize = i (in case there were any successful opens) and break out of the loop. And again you'll need to decide what is an error and what gets just a warning - if someone asks for 3 queues and gets none, that should lead to continuing without any error or even warning. But if they request 2 and only get one, is that an error, or do we warn and continue? or just silently continue? (I don't have the answer, I'm just asking the question :-)
} } virDomainAuditNetDevice(def, net, "/dev/vhost-net", *vhostfdSize); @@ -6560,12 +6562,10 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, }
for (i = 0; i < vhostfdSize; i++) { - if (vhostfd[i] >= 0) { - virCommandTransferFD(cmd, vhostfd[i]); - if (virAsprintf(&vhostfdName[i], "%d", vhostfd[i]) < 0) { - virReportOOMError(); - goto cleanup; - } + virCommandTransferFD(cmd, vhostfd[i]); + if (virAsprintf(&vhostfdName[i], "%d", vhostfd[i]) < 0) { + virReportOOMError(); + goto cleanup; } }
On 24.05.2013 16:42, Laine Stump wrote:
On 05/24/2013 08:50 AM, Michal Privoznik wrote:
Currently, if there's an error opening /dev/vhost-net (e.g. because it doesn't exist) but it's not required we proceed with vhostfd array filled with -1 and vhostfdSize unchanged. Later, when constructing the qemu command line only non-negative items within vhostfd array are taken into account. This means, vhostfdSize may be greater than the actual count of non-negative items in vhostfd array. This results in improper command line arguments being generated, e.g.:
-netdev tap,fd=21,id=hostnet0,vhost=on,vhostfd=(null) --- src/qemu/qemu_command.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 434f5a7..d969f88 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -486,6 +486,8 @@ qemuOpenVhostNet(virDomainDefPtr def, "but is unavailable")); goto error; } + i--; + (*vhostfdSize)--;
This will still go back through the loop again and try to open another. I would instead just set vhostfdSize = i (in case there were any successful opens) and break out of the loop.
And again you'll need to decide what is an error and what gets just a warning - if someone asks for 3 queues and gets none, that should lead to continuing without any error or even warning. But if they request 2 and only get one, is that an error, or do we warn and continue? or just silently continue? (I don't have the answer, I'm just asking the question :-)
Well, MQ is designed to be M:N (where M is the number of TAP devices = queues, N is the number of vhost-nets). So I think we can continue with any number of successful openings.
} } virDomainAuditNetDevice(def, net, "/dev/vhost-net", *vhostfdSize); @@ -6560,12 +6562,10 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, }
for (i = 0; i < vhostfdSize; i++) { - if (vhostfd[i] >= 0) { - virCommandTransferFD(cmd, vhostfd[i]); - if (virAsprintf(&vhostfdName[i], "%d", vhostfd[i]) < 0) { - virReportOOMError(); - goto cleanup; - } + virCommandTransferFD(cmd, vhostfd[i]); + if (virAsprintf(&vhostfdName[i], "%d", vhostfd[i]) < 0) { + virReportOOMError(); + goto cleanup; } }
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Laine Stump -
Michal Privoznik