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

[Laine Stump]

On 05/16/2013 08:49 AM, Michal Privoznik wrote:
> ---
>  src/qemu/qemu_command.c | 82 ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 58 insertions(+), 24 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index f2c6d47..5d64705 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6477,11 +6477,15 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>                                enum virNetDevVPortProfileOp vmop)
>  {
>      int ret = -1;
> -    int tapfd = -1;
>      char *nic = NULL, *host = NULL;
> -    char *tapfdName = NULL;
> -    char *vhostfdName = NULL;
> +    int *tapfd = NULL;
> +    int tapfdSize = 0;
> +    int *vhostfd = NULL;
> +    int vhostfdSize = 0;
> +    char **tapfdName = NULL;
> +    char **vhostfdName = NULL;
>      int actualType = virDomainNetGetActualType(net);
> +    int i;
>  
>      if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
>          /* NET_TYPE_HOSTDEV devices are really hostdev devices, so
> @@ -6495,12 +6499,24 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>  
>      if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
>          actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> +        if (VIR_ALLOC(tapfd) < 0 || VIR_ALLOC(tapfdName) < 0) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +
> +        tapfdSize = 1;
>          if (qemuNetworkIfaceConnect(def, conn, driver, net,
> -                                    qemuCaps, &tapfd, 1) < 0)
> +                                    qemuCaps, tapfd, tapfdSize) < 0)


Again, tapfdSize needs to be an int*, but I already mentioned that in
the last patch.

>              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).