[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv4 2/4] qemu: Move interface cmd line construction into a separate function



On 05/21/2013 10:18 AM, Michal Privoznik wrote:
> Currently, we have one huge function to construct qemu command line.
> This is very ineffective esp. if there's a fault somewhere.
> ---
>  src/qemu/qemu_command.c | 224 +++++++++++++++++++++++++-----------------------
>  1 file changed, 117 insertions(+), 107 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 33a1910..6f4028e 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6406,6 +6406,117 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg,
>      return 0;
>  }
>  
> +static int
> +qemuBuildInterfaceCommandLine(virCommandPtr cmd,
> +                              virQEMUDriverPtr driver,
> +                              virConnectPtr conn,
> +                              virDomainDefPtr def,
> +                              virDomainNetDefPtr net,
> +                              virQEMUCapsPtr qemuCaps,
> +                              int vlan,
> +                              int bootindex,
> +                              enum virNetDevVPortProfileOp vmop)
> +{
> +    int ret = -1;
> +    int tapfd = -1;
> +    int vhostfd = -1;
> +    char *nic = NULL, *host = NULL;
> +    char *tapfdName = NULL;
> +    char *vhostfdName = NULL;
> +    int actualType = virDomainNetGetActualType(net);
> +
> +    if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
> +        /* NET_TYPE_HOSTDEV devices are really hostdev devices, so
> +         * their commandlines are constructed with other hostdevs.
> +         */
> +        return 0;
> +    }
> +
> +    if (!bootindex)
> +        bootindex = net->info.bootIndex;
> +
> +    if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> +        actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> +        tapfd = qemuNetworkIfaceConnect(def, conn, driver, net, qemuCaps);
> +        if (tapfd < 0)
> +            goto cleanup;
> +    } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
> +        tapfd = qemuPhysIfaceConnect(def, driver, net, qemuCaps, vmop);
> +        if (tapfd < 0)
> +            goto cleanup;
> +    }
> +
> +    if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> +        actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> +        actualType == VIR_DOMAIN_NET_TYPE_ETHERNET ||
> +        actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
> +        /* Attempt to use vhost-net mode for these types of
> +           network device */
> +        if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0)
> +            goto cleanup;
> +    }
> +
> +    if (tapfd >= 0) {
> +        virCommandTransferFD(cmd, tapfd);
> +        if (virAsprintf(&tapfdName, "%d", tapfd) < 0) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (vhostfd >= 0) {
> +        virCommandTransferFD(cmd, vhostfd);
> +        if (virAsprintf(&vhostfdName, "%d", vhostfd) < 0) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +    }
> +
> +    /* Possible combinations:
> +     *
> +     *  1. Old way:   -net nic,model=e1000,vlan=1 -net tap,vlan=1
> +     *  2. Semi-new:  -device e1000,vlan=1        -net tap,vlan=1
> +     *  3. Best way:  -netdev type=tap,id=netdev1 -device e1000,id=netdev1
> +     *
> +     * NB, no support for -netdev without use of -device
> +     */
> +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
> +        virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> +        if (!(host = qemuBuildHostNetStr(net, driver,
> +                                         ',', vlan, tapfdName,
> +                                         vhostfdName)))
> +            goto cleanup;
> +        virCommandAddArgList(cmd, "-netdev", host, NULL);
> +    }
> +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> +        if (!(nic = qemuBuildNicDevStr(net, vlan, bootindex, qemuCaps)))
> +            goto cleanup;
> +        virCommandAddArgList(cmd, "-device", nic, NULL);
> +    } else {
> +        if (!(nic = qemuBuildNicStr(net, "nic,", vlan)))
> +            goto cleanup;
> +        virCommandAddArgList(cmd, "-net", nic, NULL);
> +    }
> +    if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
> +          virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) {
> +        if (!(host = qemuBuildHostNetStr(net, driver,
> +                                         ',', vlan, tapfdName,
> +                                         vhostfdName)))
> +            goto cleanup;
> +        virCommandAddArgList(cmd, "-net", host, NULL);
> +    }
> +
> +    ret = 0;
> +cleanup:
> +    if (ret < 0)
> +        virDomainConfNWFilterTeardown(net);
> +    VIR_FREE(nic);
> +    VIR_FREE(host);
> +    VIR_FREE(tapfdName);
> +    VIR_FREE(vhostfdName);
> +    return ret;
> +}
> +
>  qemuBuildCommandLineCallbacks buildCommandLineCallbacks = {
>      .qemuGetSCSIDeviceSgName = virSCSIDeviceGetSgName,
>  };
> @@ -7344,118 +7455,17 @@ qemuBuildCommandLine(virConnectPtr conn,
>  
>          for (i = 0 ; i < def->nnets ; i++) {
>              virDomainNetDefPtr net = def->nets[i];
> -            char *nic, *host;
> -            char tapfd_name[50] = "";
> -            char vhostfd_name[50] = "";
> -            int vlan;
> -            int bootindex = bootNet;
> -            int actualType = virDomainNetGetActualType(net);
> -
> -            if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
> -                /* NET_TYPE_HOSTDEV devices are really hostdev devices, so
> -                 * their commandlines are constructed with other hostdevs.
> -                 */
> -                continue;
> -            }
> -
> -            bootNet = 0;
> -            if (!bootindex)
> -                bootindex = net->info.bootIndex;
> -
> +            int vlan = i;
>              /* VLANs are not used with -netdev, so don't record them */
>              if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
>                  virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))
>                  vlan = -1;


A blank line after "int vlan = i;" would be nice.


> -            else
> -                vlan = i;
>  
> -            if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> -                actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> -                int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net,
> -                                                    qemuCaps);
> -                if (tapfd < 0)
> -                    goto error;
> -
> -                last_good_net = i;
> -                virCommandTransferFD(cmd, tapfd);
> -
> -                if (snprintf(tapfd_name, sizeof(tapfd_name), "%d",
> -                             tapfd) >= sizeof(tapfd_name))
> -                    goto no_memory;
> -            } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
> -                int tapfd = qemuPhysIfaceConnect(def, driver, net,
> -                                                 qemuCaps, vmop);
> -                if (tapfd < 0)
> -                    goto error;
> -
> -                last_good_net = i;
> -                virCommandTransferFD(cmd, tapfd);
> -
> -                if (snprintf(tapfd_name, sizeof(tapfd_name), "%d",
> -                             tapfd) >= sizeof(tapfd_name))
> -                    goto no_memory;
> -            }
> -
> -            if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> -                actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> -                actualType == VIR_DOMAIN_NET_TYPE_ETHERNET ||
> -                actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
> -                /* Attempt to use vhost-net mode for these types of
> -                   network device */
> -                int vhostfd;
> -
> -                if (qemuOpenVhostNet(def, net, qemuCaps, &vhostfd) < 0)
> -                    goto error;
> -                if (vhostfd >= 0) {
> -                    virCommandTransferFD(cmd, vhostfd);
> -
> -                    if (snprintf(vhostfd_name, sizeof(vhostfd_name), "%d",
> -                                 vhostfd) >= sizeof(vhostfd_name))
> -                        goto no_memory;
> -                }
> -            }
> -            /* Possible combinations:
> -             *
> -             *  1. Old way:   -net nic,model=e1000,vlan=1 -net tap,vlan=1
> -             *  2. Semi-new:  -device e1000,vlan=1        -net tap,vlan=1
> -             *  3. Best way:  -netdev type=tap,id=netdev1 -device e1000,id=netdev1
> -             *
> -             * NB, no support for -netdev without use of -device
> -             */
> -            if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
> -                virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> -                virCommandAddArg(cmd, "-netdev");
> -                if (!(host = qemuBuildHostNetStr(net, driver,
> -                                                 ',', vlan, tapfd_name,
> -                                                 vhostfd_name)))
> -                    goto error;
> -                virCommandAddArg(cmd, host);
> -                VIR_FREE(host);
> -            }
> -            if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> -                virCommandAddArg(cmd, "-device");
> -                nic = qemuBuildNicDevStr(net, vlan, bootindex, qemuCaps);
> -                if (!nic)
> -                    goto error;
> -                virCommandAddArg(cmd, nic);
> -                VIR_FREE(nic);
> -            } else {
> -                virCommandAddArg(cmd, "-net");
> -                if (!(nic = qemuBuildNicStr(net, "nic,", vlan)))
> -                    goto error;
> -                virCommandAddArg(cmd, nic);
> -                VIR_FREE(nic);
> -            }
> -            if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
> -                  virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) {
> -                virCommandAddArg(cmd, "-net");
> -                if (!(host = qemuBuildHostNetStr(net, driver,
> -                                                 ',', vlan, tapfd_name,
> -                                                 vhostfd_name)))
> -                    goto error;
> -                virCommandAddArg(cmd, host);
> -                VIR_FREE(host);
> -            }
> +            if (qemuBuildInterfaceCommandLine(cmd, driver, conn, def, net,
> +                                              qemuCaps, vlan, bootNet, vmop) < 0)
> +                goto error;
> +            last_good_net = i;
> +            bootNet = 0;
>          }
>      }
>  

ACK, but please add the blank line I mentioned above.



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]