On 20.05.2013 19:15, Laine Stump wrote:
> 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...
Unfortunately not yet. Macvtap is still under development in question of MQ.
>
>
>
>> + 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.
>
Well, I keep modifying patches as you review them. I think I gonna post
one more version - just in the form as you requested.
>
>
>> +
>> + 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).
Yeah, I was thinking about it too.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list