Re: [libvirt] [PATCHv3 10/11] qemu: Adapt qemuBuildInterfaceCommandLine to to multiqueue net

On 05/16/2013 08:49 AM, Michal Privoznik wrote: > --- > src/qemu/qemu_command.c | 82 ++++++++++++++++++++++++++++++++++--------------- > 1 file changed, 58 insertions(+), 24 deletions(-) >

> goto cleanup; > } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { > - tapfd = qemuPhysIfaceConnect(def, driver, net, qemuCaps, vmop); > - if (tapfd < 0) > + if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(tapfdName) < 0) { > + virReportOOMError(); > + goto cleanup; > + } > + tapfdSize = 1; > + tapfd[0] = qemuPhysIfaceConnect(def, driver, net, > + qemuCaps, vmop); macvtap doesn't support multiple queues? But macvtap is supposed to be the panacea of high performance...

> + if (tapfd[0] < 0) > goto cleanup; > } > > @@ -6510,28 +6526,34 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, > actualType == VIR_DOMAIN_NET_TYPE_DIRECT) { > /* Attempt to use vhost-net mode for these types of > network device */ > - int vhostfd; > - int vhostfdSize = 1; > + if (VIR_ALLOC(vhostfd) < 0 || VIR_ALLOC(vhostfdName)) { > + virReportOOMError(); > + goto cleanup; > + } > + vhostfdSize = 1; I am *really* losing track of which layer of which set of fds is getting the "multi" treatment in each patch. I think there are just too many steps in this patch series. It would be easier to follow if there was a patch to add the config parser/formatter/rng/docs, then just another single patch to wire that data all the way up and down the stack. I find myself spending time at each stage ttrying to decide if something is a bug, or just an interim thing that will be fixed in a later step.

> + > + if (qemuOpenVhostNet(def, net, qemuCaps, vhostfd, &vhostfdSize) < 0) > + goto cleanup; > + } > > - if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd, &vhostfdSize) < 0) > + for (i = 0; i < tapfdSize; i++) { > + virCommandTransferFD(cmd, tapfd[i]); > + if (virAsprintf(&tapfdName[i], "%d", tapfd[i]) < 0) { > + virReportOOMError(); > goto cleanup; > - if (vhostfdSize > 0) { > - virCommandTransferFD(cmd, vhostfd); > - if (virAsprintf(&vhostfdName, "%d", vhostfd) < 0) { > + } > + } > + > + 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; > } > } > } > > - if (tapfd >= 0) { > - virCommandTransferFD(cmd, tapfd); > - if (virAsprintf(&tapfdName, "%d", tapfd) < 0) { > - virReportOOMError(); > - goto cleanup; > - } > - } > - > /* Possible combinations: > * > * 1. Old way: -net nic,model=e1000,vlan=1 -net tap,vlan=1 > @@ -6544,8 +6566,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, > virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { > if (!(host = qemuBuildHostNetStr(net, driver, > ',', vlan, > - &tapfdName, tapfdName ? 1 : 0, > - &vhostfdName, vhostfdName ? 1 : 0))) > + tapfdName, tapfdSize, > + vhostfdName, vhostfdSize))) > goto cleanup; > virCommandAddArgList(cmd, "-netdev", host, NULL); > } > @@ -6562,8 +6584,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, > virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) { > if (!(host = qemuBuildHostNetStr(net, driver, > ',', vlan, > - &tapfdName, tapfdName ? 1 : 0, > - &vhostfdName, vhostfdName ? 1 : 0))) > + tapfdName, tapfdSize, > + vhostfdName, vhostfdSize))) > goto cleanup; > virCommandAddArgList(cmd, "-net", host, NULL); > } > @@ -6572,6 +6594,18 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, > cleanup: > if (ret < 0) > virDomainConfNWFilterTeardown(net); > + for (i = 0; i < tapfdSize; i++) { > + if (ret < 0) > + VIR_FORCE_CLOSE(tapfd[i]); > + VIR_FREE(tapfdName[i]); Okay, tapfdSize isn't set until/unless tapfd and tapfdName are both successfully allocated... > + } > + for (i = 0; i < vhostfdSize; i++) { > + if (ret < 0) > + VIR_FORCE_CLOSE(vhostfd[i]); > + VIR_FREE(vhostfdName[i]); Same here. (I mention this because I was worried about it initially :-) > + } > + VIR_FREE(tapfd); > + VIR_FREE(vhostfd); > VIR_FREE(nic); > VIR_FREE(host); > VIR_FREE(tapfdName); ACK, aside from making tapfdSize an int* (so it can be modified on failure/non-support in qemuIfaceNetworkConnect), and an answer about support for multiple queues when macvtap is used. Oh, actually another problem - in cases where multiple queues are requested and can't be honored due to interface type (or any other reason), I think we should log an error and fail, but it looks like you just ignore it. So I guess that also needs fixing (or an explanation).